Re: [PATCH 3/4] Allow specifying specialized merge-backend per path.

From: Junio C Hamano <junkio@cox.net>
Date: Apr 17, 2007, 09:20 UTC
Message-ID: <7vabx7qrvl.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <11767973191314-git-send-email-junkio@cox.net>

Junio C Hamano <junkio@cox.net> writes:

I hate to nitpick my own patches, but...

Show 7 quoted lines

> There is one caveat, though.  ll_merge() is called for both
> internal ancestor merge and the outer "final" merge.  I think an
> interactive custom per-path merge backend should refrain from
> going interactive when performing an internal merge (you can
> tell it by checking call_depth) and instead just call either
> ll_xdl_merge() if the content is text, or call ll_ours_merge()
> otherwise.

After having thought about this a bit more, I think a conflict in *internal* merge for a binary file should probably take the ancestor's, instead of ours.

A conflict during the internal merge happens when there are multiple common ancestors, and the development lines continuing from these ancestores may have disagreed on the result of this conflicting merge earlier.

             o---A---C---o---Y
            /     \ /         \
          GP       .           M
            \     / \         /
             o---B---D---o---Z

When we are trying to do the merge M between Y and Z, we find two closest common ancestors, A and B. An internal merge to merge them, using the grandparent ancestor, GP, is attempted, and we will use the result as the virtual common ancestor to create a merge between Y and Z.

The fact the internal merge between A and B conflicted means that the development lines leading to Y and Z already saw that same conflict before reaching Y and Z (in the above picture, at C and D, respectively), and they might have resolved it differently.

When it is a text file, Fredrik's clever "recursive merge" leaves conflicted merge as the common ancestor, which is different from either C or D's resolution (perhaps we can think of it being somewhere areound sign X in the above picture), and merge between Y and Z using that as the common ancestor cancels the conflict out if C and D resolved it the same way.

However, when the file is a binary guck, the result of such a half-merge with conflict markers is simply a corrupt binary data, and is unusable even for inspection. That was the reason why I initially suggested to use 'ours'. But that means, when we are on the branch that leads to Y, we essentially favor C's resolution, defeating the "accidental revert avoidance" the recursive merge strategy offers. If instead we take the version from GP as the common ancestor, we have a usable binary guck as an ancestor as well, and also we are being neutral between the branches, giving the end user a chance to make an unbiased decision on his own.

Show 22 quoted lines

> diff --git a/merge-recursive.c b/merge-recursive.c
> index 4eb62cf..a6c08a1 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -659,6 +660,124 @@ static void fill_mm(const unsigned char *sha1, mmfile_t *mm)
>  	mm->size = size;
>  }
>  
> +/* Low-level merge functions */
> +typedef int (*ll_merge_fn)(mmfile_t *orig,
> +			   mmfile_t *src1, const char *name1,
> +			   mmfile_t *src2, const char *name2,
> +			   mmbuffer_t *result);
> +
> +static int ll_xdl_merge(mmfile_t *orig,
> +			mmfile_t *src1, const char *name1,
> +			mmfile_t *src2, const char *name2,
> +			mmbuffer_t *result)
> +{
> +	xpparam_t xpp;
> +	memset(&xpp, 0, sizeof(xpp));
> +	memset(&xpp, 0, sizeof(xpp));

I caught this after sending this message, and already fixed it in the version I pushed out on 'pu'.