feature suggestion: optimize common parts for checkout --conflict=diff3

Hello,

Here comes another recipe for a different suggestion:

	git init
	echo 1 > file
	git add file
	git commit -m 'base'
	git branch branch
	seq 1 30 | grep -v 15 > file
	git commit -m 'add 2-30 without 15' file
	git checkout branch
	seq 1 30 | grep -v 16 > file
	git commit -m 'add 2-30 without 16' file
	git merge master
	git diff

This yields:

	diff --cc file
	index a07e697,5080129..0000000
	--- a/file
	+++ b/file
	@@@ -12,7 -12,7 +12,11 @@@
	  12
	  13
	  14
	++<<<<<<< HEAD
	 +15
	++=======
	+ 16
	++>>>>>>> master
	  17
	  18
	  19

as expected; nice and sweet. After

	git checkout --conflict=diff3 file

however the difference isn't that easy to spot any more. I expected

	diff --cc file
	index a07e697,5080129..0000000
	--- a/file
	+++ b/file
	@@@ -12,7 -12,7 +12,12 @@@
	  12
	  13
	  14
	++<<<<<<< ours
	 +15
	++||||||| base
	++=======
	+ 16
	++>>>>>>> theirs
	  17
	  18
	  19

But instead I get

	diff --cc file
	index a07e697,5080129..0000000
	--- a/file
	+++ b/file
	@@@ -1,29 -1,29 +1,61 @@@
	  1
	++<<<<<<< ours
	 +2
	 +3
	 +4
	 +5
	 +6
	 +7
	 +8
	 +9
	 +10
	 +11
	 +12
	 +13
	 +14
	 +15
	 +17
	 +18
	 +19
	 +20
	 +21
	 +22
	 +23
	 +24
	 +25
	 +26
	 +27
	 +28
	 +29
	 +30
	++||||||| base
	++=======
	+ 2
	+ 3
	+ 4
	+ 5
	+ 6
	+ 7
	+ 8
	+ 9
	+ 10
	+ 11
	+ 12
	+ 13
	+ 14
	+ 16
	+ 17
	+ 18
	+ 19
	+ 20
	+ 21
	+ 22
	+ 23
	+ 24
	+ 25
	+ 26
	+ 27
	+ 28
	+ 29
	+ 30
	++>>>>>>> theirs

Of course this is technically correct, just not maximally helpful.

Is this a missing optimisation for the diff3 case or did I miss a detail
that makes my expectation wrong?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

        git checkout --conflict=diff3 file

That's somehow unrelated, but shouldn't we have a "conflict" option to
git-merge as we have for git-checkout ?

With something like this (pasted into gmail):

diff --git a/builtin/merge.c b/builtin/merge.c
index 7c8922c..edad742 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c

@@ -65,6 +65,7 @@ static int abort_current_merge;
 static int show_progress = -1;
 static int default_to_upstream;
 static const char *sign_commit;
+static char *conflict_style;

 static struct strategy all_strategy[] = {
  { "recursive",  DEFAULT_TWOHEAD | NO_TRIVIAL },

@@ -213,6 +214,7 @@ static struct option builtin_merge_options[] = {
  { OPTION_STRING, 'S', "gpg-sign", &sign_commit, N_("key id"),
   N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
  OPT_BOOLEAN(0, "overwrite-ignore", &overwrite_ignore, N_("update

ignored files (default)")),
+ OPT_STRING(0, "conflict", &conflict_style, N_("style"), N_("conflict
style (merge or diff3)")),
  OPT_END()
 };

@@ -1102,6 +1104,9 @@ int cmd_merge(int argc, const char **argv, const

char *prefix)
  if (verbosity < 0 && show_progress == -1)
  show_progress = 0;

+ if (conflict_style)
+ git_xmerge_config("merge.conflictstyle", conflict_style, NULL);
+
  if (abort_current_merge) {
  int nargc = 2;
  const char *nargv[] = {"reset", "--merge", NULL};

however the difference isn't that easy to spot any more. I expected

        diff --cc file
        index a07e697,5080129..0000000
        --- a/file
        +++ b/file
        @@@ -12,7 -12,7 +12,12 @@@
          12
          13
          14
        ++<<<<<<< ours
         +15
        ++||||||| base
        ++=======
        + 16
        ++>>>>>>> theirs
          17
          18
          19

This is not correct, it would mean that 12, 13, 14, 17, 18, 19 are in
base, while they are not.

Cheers,
Antoine

On Wed, Mar 06, 2013 at 08:26:57PM +0100, Antoine Pelisse wrote:

quoted

however the difference isn't that easy to spot any more. I expected

        diff --cc file
        index a07e697,5080129..0000000
        --- a/file
        +++ b/file
        @@@ -12,7 -12,7 +12,12 @@@
          12
          13
          14
        ++<<<<<<< ours
         +15
        ++||||||| base
        ++=======
        + 16
        ++>>>>>>> theirs
          17
          18
          19

This is not correct, it would mean that 12, 13, 14, 17, 18, 19 are in
base, while they are not.

Yeah, I agree it is a bit of a lie, as you are not seeing the full
picture of what was in the base. That is why we intentionally dial down
the conflict simplification level when using diff3. If you apply this
patch to git:

diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c
index 9e13b25..f381e0c 100644
--- a/xdiff/xmerge.c
+++ b/xdiff/xmerge.c

@@ -420,15 +420,6 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1,
 	int style = xmp->style;
 	int favor = xmp->favor;
 
-	if (style == XDL_MERGE_DIFF3) {
-		/*
-		 * "diff3 -m" output does not make sense for anything
-		 * more aggressive than XDL_MERGE_EAGER.
-		 */
-		if (XDL_MERGE_EAGER < level)
-			level = XDL_MERGE_EAGER;
-	}
-
 	c = changes = NULL;
 
 	while (xscr1 && xscr2) {

then it will produce the output that Uwe expects. While it can be
misleading, I also think it can make some conflicts (like this one) much
easier to understand. I don't see any reason we can't have a "zealous
diff3" mode to let people view this output, as long as it is not the
default.

-Peff

"zdiff3" is identical to ordinary diff3, only it allows more aggressive
compaction than diff3. This way the displayed base isn't necessary
technically correct, but still this mode might help resolving merge
conflicts between two near identical additions.

Signed-off-by: Uwe Kleine-König <redacted>
---
Hello,

this patch implements what I want. Thanks to Jeff for pointing me to the
right code to modify.

Best regards
Uwe

 builtin/merge-file.c                   | 2 ++
 contrib/completion/git-completion.bash | 2 +-
 xdiff-interface.c                      | 2 ++
 xdiff/xdiff.h                          | 1 +
 xdiff/xmerge.c                         | 8 +++++++-
 5 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/builtin/merge-file.c b/builtin/merge-file.c
index c0570f2..4ef86aa 100644
--- a/builtin/merge-file.c
+++ b/builtin/merge-file.c

@@ -32,6 +32,8 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
 	struct option options[] = {
 		OPT_BOOLEAN('p', "stdout", &to_stdout, N_("send results to standard output")),
 		OPT_SET_INT(0, "diff3", &xmp.style, N_("use a diff3 based merge"), XDL_MERGE_DIFF3),
+		OPT_SET_INT(0, "zdiff3", &xmp.style, N_("use a zealous diff3 based merge"),
+				XDL_MERGE_ZEALOUS_DIFF3),
 		OPT_SET_INT(0, "ours", &xmp.favor, N_("for conflicts, use our version"),
 			    XDL_MERGE_FAVOR_OURS),
 		OPT_SET_INT(0, "theirs", &xmp.favor, N_("for conflicts, use their version"),

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index b62bec0..a0d887e 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash

@@ -1091,7 +1091,7 @@ _git_checkout ()
 
 	case "$cur" in
 	--conflict=*)
-		__gitcomp "diff3 merge" "" "${cur##--conflict=}"
+		__gitcomp "diff3 merge zdiff3" "" "${cur##--conflict=}"
 		;;
 	--*)
 		__gitcomp "

diff --git a/xdiff-interface.c b/xdiff-interface.c
index ecfa05f..a911c25 100644
--- a/xdiff-interface.c
+++ b/xdiff-interface.c

@@ -308,6 +308,8 @@ int git_xmerge_config(const char *var, const char *value, void *cb)
 			die("'%s' is not a boolean", var);
 		if (!strcmp(value, "diff3"))
 			git_xmerge_style = XDL_MERGE_DIFF3;
+		else if (!strcmp(value, "zdiff3"))
+			git_xmerge_style = XDL_MERGE_ZEALOUS_DIFF3;
 		else if (!strcmp(value, "merge"))
 			git_xmerge_style = 0;
 		else

diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h
index 219a3bb..9730c63 100644
--- a/xdiff/xdiff.h
+++ b/xdiff/xdiff.h

@@ -64,6 +64,7 @@ extern "C" {
 
 /* merge output styles */
 #define XDL_MERGE_DIFF3 1
+#define XDL_MERGE_ZEALOUS_DIFF3 2
 
 typedef struct s_mmfile {
 	char *ptr;

diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c
index 9e13b25..4772707 100644
--- a/xdiff/xmerge.c
+++ b/xdiff/xmerge.c

@@ -177,7 +177,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
 	size += xdl_recs_copy(xe1, m->i1, m->chg1, 1,
 			      dest ? dest + size : NULL);
 
-	if (style == XDL_MERGE_DIFF3) {
+	if (style == XDL_MERGE_DIFF3 || style == XDL_MERGE_ZEALOUS_DIFF3) {
 		/* Shared preimage */
 		if (!dest) {
 			size += marker_size + 1 + marker3_size;

@@ -420,6 +420,12 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1,
 	int style = xmp->style;
 	int favor = xmp->favor;
 
+	/*
+	 * This is the only change between XDL_MERGE_DIFF3 and
+	 * XDL_MERGE_ZEALOUS_DIFF3. "zdiff3" isn't 100% technically correct (as
+	 * the base might be considerably simplified), but still it might help
+	 * interpreting conflicts between two big and near identical additions.
+	 */
 	if (style == XDL_MERGE_DIFF3) {
 		/*
 		 * "diff3 -m" output does not make sense for anything

-- 
1.8.2.rc2

On Wed, Mar 06, 2013 at 09:36:42PM +0100, Uwe Kleine-König wrote:

"zdiff3" is identical to ordinary diff3, only it allows more aggressive
compaction than diff3. This way the displayed base isn't necessary
technically correct, but still this mode might help resolving merge
conflicts between two near identical additions.

Signed-off-by: Uwe Kleine-König <redacted>

I think the patch is correct, assuming this is the interface we want.

It would be more flexible instead to have:

  1. user can configure zealous-level of merge via command-line or
     config (they cannot control it at all right now)

  2. when diff3 is used and no level is explicitly given, do not go
     above EAGER

  3. otherwise, respect the level given by the user, even if it is
     ZEALOUS

But that would involve a lot of refactoring. I don't know if it is worth
the effort (the bonus is that people can then set the level
independently, but I do not know that anyone ever wants to do that).

 builtin/merge-file.c                   | 2 ++
 contrib/completion/git-completion.bash | 2 +-
 xdiff-interface.c                      | 2 ++
 xdiff/xdiff.h                          | 1 +
 xdiff/xmerge.c                         | 8 +++++++-
 5 files changed, 13 insertions(+), 2 deletions(-)

I think this would need documentation not only to let users know about
the feature, but also to warn them of the caveats.

-Peff