Re: git merge a b when a == b but neither == o is always a successful merge?

From: Jeff King <peff@peff.net>
Date: Nov 21, 2014, 18:15 UTC
Message-ID: <20141121181539.GC26650@peff.net>
In-Reply-To: <21610.29903.366230.851787@perdition.linnaean.org>

On Mon, Nov 17, 2014 at 05:21:03PM -0500, Daniel Hagerty wrote:

Show 8 quoted lines

>  > I don't think there is an easy way to get what you want. You would have
>  > to write a new merge 3-way strategy that handles this case differently.
>  > And most of the file-level heavy lifting in merge strategies is done by
>  > the low-level unpack_trees code, which handles this case. From "git help
> 
>     I have a very rough draft that seems to do what I want, exposed
> through .gitattributes (below).  Given that this is something you probably
> want tightly scoped, does it make sense to expose it anywhere else?

This is the first use of gitattributes in the unpack-trees code path. I cannot think offhand of any philosophical reason that should not be OK, but it is something worth considering (i.e., this code path is deep plumbing; are there cases where we would not want to support gitattributes?).

>     Is it accurate to speak of this as exposing merge minimal?

I am not 100% sure they are not orthogonal concepts. The tree-diff is about "just because two separate changes ended at the same place does not mean they should be merged into the same change". I am actually not sure what XDL_MERGE_MINIMAL entails exactly. Is it only about coalescing overlapping endpoints in the merge? Documentation is sparse.

The patch itself doesn't look too bad. It covers the file-level case and the content-level case. Is there additional code needed to handle the directory-level case? That is, if I have a tree that starts at sha1 X, and ends up at sha1 Y in both sides of the merge, do we still descend into it to make the file-level comparisons, or do we optimize that out?

Show 13 quoted lines

> diff --git a/ll-merge.c b/ll-merge.c
> index da59738..2a06ac9 100644
> --- a/ll-merge.c
> +++ b/ll-merge.c
> @@ -84,7 +84,16 @@ static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,
>  	}
> 
>  	memset(&xmp, 0, sizeof(xmp));
> -	xmp.level = XDL_MERGE_ZEALOUS;
> +
> +	struct git_attr_check check;
> +	check.attr = git_attr("merge-minimal");
> +	(void) git_check_attr(path, 1, &check);

Please void C99 decl-after-statement, as we build on older compilers that do not like it.

We also do not typically annotate ignored return values.

> +	if(ATTR_TRUE(check.value))
> +	  xmp.level = XDL_MERGE_MINIMAL;
> +	else
> +	  xmp.level = XDL_MERGE_ZEALOUS;

..and indentations should be a single tab.

Other than those minor style nits, I do not see anything obviously _wrong_ with the patch, but I am far from an expert in the area. It would need documentation and tests, of course. The next step may be to wrap it up with those things and post it to the list, which will likely get more attention.

-Peff