5

When you have docblocks above multiple functions removing a function creates a sub-optimal patch.

index.js:

/**
 * Function foo description.
 */
function foo() {}

/**
 * Function bar description.
 */
function bar() {}

Removing function foo with its docblock generates the following patch:

diff --git a/index.js b/index.js
index f4e18ef..933004f 100644
--- a/index.js
+++ b/index.js
@@ -1,9 +1,4 @@
 /**
- * Function foo description.
- */
-function foo() {}
-
-/**
  * Function bar description.
  */
 function bar() {}

This means that any merge that brings with it commits that touch the space between function foo and function bar now result in a conflict. For example imagine we created a branch feature-1 before removing foo, and in index.js added a function foobar between the two. The conflict would look as follows:

/**
<<<<<<< HEAD
=======
 * Function foo description.
 */
function foo() {}

/**
 * Function foobar description.
 */
function foobar() {}

/**
>>>>>>> feature-1
 * Function bar description.
 */
function bar() {}

I imagine there would be no issue if the /** was grabbed from the top instead. I'm sure there's a good reason for git to prefer removing from the end but I'd like to force it to grab it from the start. Is there a way to easily do this? Or is manual patch editing the only way?

alextes
  • 1,817
  • 2
  • 15
  • 22
  • I would expect that a "smarter" diff will not avoid the merge conflict in this hypothetical situation. If only foo() and its preceding comment block gets removed by the diff, a merge with a patch that adds a function after foo() will still have a conflict, since it will have enough context to collide with the removed code. – Sam Varshavchik Oct 19 '16 at 12:29
  • Possible, but at the very least the conflict would be about empty space below `foo` vs your added function. Which would clearly leave the whole `foo` function excluded. Whereas now I keep getting confused by the fact `foo` is somehow a part of the conflict. – alextes Oct 19 '16 at 12:36
  • I though that `git diff --patience` would solve your problem, but it turned out that it doesn't, which seems to me like a bug and `git diff`. I posted a corresponding question: http://stackoverflow.com/questions/40133534/is-gits-implementation-of-the-patience-diff-algorithm-correct – Leon Oct 19 '16 at 14:02

1 Answers1

3

It's far from perfect, but the new --compaction-heuristic in Git 2.9 often does what you want. See this blog post for details. You can configure it to be on by default, but given that it sometimes makes things worse, I have not done so:

git config --global diff.compactionHeuristic true

Your Git version must be at least 2.9 for this to have any effect.

One flaw in the current implementation is that it quite literally requires a blank line above the modified section. Starting at the top of the file is not sufficient. For instance, suppose we start with:

block:
This file has
three blocks.

block:
There is a blank line
between each.

block:
This is the third
block.

If we delete the middle block, the default diff retains the second block: line and deletes the third block: line. Turning on compaction moves the diff hunk up until it reaches the blank line above the second block:, which is what we want.

Unfortunately, if we delete the first block, the compaction heuristic attempts to move the diff hunk up to include the first block: line, but fails because it hits the top of the file, where there is no blank line above it (as there is no line at all above it). It therefore leaves the first word block: in place and deletes the second.

(Fixing this would require only that the compaction algorithm provide a "virtual line zero" that is blank. Note that the problem itself never occurs at the end of the file since the default diff favors deletion of later lines. Meanwhile, an ugly workaround is to leave a blank line at the top of each file, just so that compaction can see it.)

torek
  • 448,244
  • 59
  • 642
  • 775
  • The blog posts seems to describe exactly my problem in detail, as its git's proposed solution to exactly my problem I'll happily accept the answer. – alextes Oct 19 '16 at 14:56
  • Hmm, retracted my acceptance. It doesn't actually work for me. That's using git version 2.10.1 adding the --compaction-heuristic option. Can you confirm it works for you. http://stackoverflow.com/questions/40133534/is-gits-implementation-of-the-patience-diff-algorithm-correct lets you easily test it. – alextes Oct 19 '16 at 15:22
  • I did say "often" does what you want: one problem is that it requires a blank line *above* the section you're deleting. When deleting the section at the top of the file, the compaction heuristic does nothing. Add a blank line at the top before making any commits, and it should work. (I believe it would be best to modify the heuristic to *also* accept "jamming up at the top of the file" as equivalent to "reaching a blank line", which would fix this case.) I'll edit the answer to note the restriction, too. – torek Oct 19 '16 at 15:28
  • Fair. If its just the whiteline I'm totally fine with your answer even if technically it doesn't solve this case. – alextes Oct 19 '16 at 16:04