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

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

[Daniel Hagerty]

    Given a repository setup thusly:

$ git --version
git version 2.2.0.rc2

git init .

echo '0.0' > version
git add version
git commit -m "master"
for i in a b ; do
  git checkout -b $i master

  echo '0.1' > version
  git commit -a -m "leg $i"
done

git checkout -b c master
echo '0.2' > version
git commit -a -m "leg c"

git checkout --detach a


"git merge c" produces the expected edit conflict.

"git merge b" produces a successful merge, as both branches perform
the "same" work.

For the body of content in question, this is a merge conflict.  Git
seems to have the hard-coded assumption otherwise.  I had to change
three source files to get the result I expected, and wasn't seeing
any indications of parameterization.

Am I missing some means of getting the results I need?  Thanks!

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

[Jeff King]

On Mon, Nov 17, 2014 at 01:39:43PM -0500, Daniel Hagerty wrote:

> "git merge b" produces a successful merge, as both branches perform
> the "same" work.

Just to be clear, you were expecting "git merge b" to produce a
conflict?

> For the body of content in question, this is a merge conflict.  Git
> seems to have the hard-coded assumption otherwise.  I had to change
> three source files to get the result I expected, and wasn't seeing
> any indications of parameterization.

I can imagine there might be times you would like to notice this case
and visit it manually (e.g., even though the conflict would show both
sides with the same content, you might want the resolution to take the
two sides sequentially, duplicating them).  But there are also cases
where choosing the new content is helpful (e.g., one side cherry-picks a
commit from the other, then later merges; you would not want to see a
conflict there).

> Am I missing some means of getting the results I need?  Thanks!

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
read-tree", which describes the index-level 3-way merge:

  ·   stage 2 and 3 are the same; take one or the other (it makes no
      difference - the same work has been done on our branch in stage 2
      and their branch in stage 3)

So I think you would have to add an option to git to handle this, unless
you want to reimplement quite a lot of code in your merge strategy.

-Peff

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

[Daniel Hagerty]

 > Just to be clear, you were expecting "git merge b" to produce a
 > conflict?

    Yessir.

 > I can imagine there might be times you would like to notice this case
 > and visit it manually (e.g., even though the conflict would show both
 > sides with the same content, you might want the resolution to take the
 > two sides sequentially, duplicating them).

    This is roughly the case here.  Judgement is needed from the
person at the wheel.

 > 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?

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

    Thanks!

commit 3a8bc89950576c0445167e4f5ee5f42d9d737d2d
Author: Daniel Hagerty <hag@linnaean.org>
Date:   Mon Nov 17 15:42:04 2014 -0500

    merge: expose XDL_MERGE_MINIMAL behavior:

    When performing a 3-way merge, an identical change to a file in both
    branches is not considered a conflict by default.  There exist content
    for which this isn't true; simultaneous, identical edits are a
    conflict.

    Expose the ability to perform a minimal merge for selective files as
    directed by gitattributes.

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);
+
+	if(ATTR_TRUE(check.value))
+	  xmp.level = XDL_MERGE_MINIMAL;
+	else
+	  xmp.level = XDL_MERGE_ZEALOUS;
+
 	xmp.favor = opts->variant;
 	xmp.xpp.flags = opts->xdl_opts;
 	if (git_xmerge_style >= 0)
diff --git a/merge-recursive.c b/merge-recursive.c
index 3efc04e..dac6252 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -924,7 +924,11 @@ static struct merge_file_info merge_file_1(struct merge_options *o,
 			}
 		}

-		if (sha_eq(a->sha1, b->sha1) || sha_eq(a->sha1, one->sha1))
+		struct git_attr_check check;
+		check.attr = git_attr("merge-minimal");
+		(void) git_check_attr(a->path, 1, &check);
+
+		if (!ATTR_TRUE(check.value) && (sha_eq(a->sha1, b->sha1) || sha_eq(a->sha1, one->sha1)))
 			hashcpy(result.sha, b->sha1);
 		else if (sha_eq(b->sha1, one->sha1))
 			hashcpy(result.sha, a->sha1);
diff --git a/unpack-trees.c b/unpack-trees.c
index cc616c3..627356a 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1597,7 +1597,12 @@ int threeway_merge(struct cache_entry **stages, struct unpack_trees_options *o)

 	if (head) {
 		/* #5ALT, #15 */
-		if (same(head, remote))
+
+		struct git_attr_check check;
+		check.attr = git_attr("merge-minimal");
+		(void) git_check_attr(head->name, 1, &check);
+
+		if (!ATTR_TRUE(check.value) && same(head, remote))
 			return merged_entry(head, index, o);
 		/* #13, #3ALT */
 		if (!df_conflict_remote && remote_match && !head_match)

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

[Jeff King]

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

>  > 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?

> 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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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?).

It is not about "not want to support", but mroe about "not want to
be affected (by user preference)", and it is a valid concern.

> ... 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?

The code to look at is unpack-trees.c::unpack_callback(); even
though it does try to be clever to do that exact optimization when
it is used for "diff-index --cached", I do not think we do it during
a real merge.
>> +	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.

Also I do not think we want to keep calling git_attr() interning
function.  All existing uses (well, at least the ones that were
written by those who know what they are doing ;-) should be avoiding
repeated look-ups, and if we are to use an attribute for this, we
should do the same.

Do we name our attributes with "dashes-in-its-name"?  I am not
asking if dashes are allowed (I know it is), but if that breaks
the pattern already established for the core part of the system
and makes this new one an oddball.

> We also do not typically annotate ignored return values.

True.

>
>> +	if(ATTR_TRUE(check.value))
>> +	  xmp.level = XDL_MERGE_MINIMAL;
>> +	else
>> +	  xmp.level = XDL_MERGE_ZEALOUS;
>
> ..and indentations should be a single tab.

True, too.

> 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.

I agree that the approach taken here is a sensible way to implement
the design, _if_ the design and the problem it tries to solve makes
sense.  I am not sure about that yet myself, though.

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

[Daniel Hagerty]

 > I agree that the approach taken here is a sensible way to implement
 > the design, _if_ the design and the problem it tries to solve makes
 > sense.  I am not sure about that yet myself, though.

    This is a "first things first".

    What aspect of the problem to be solved is in question?

    From here, it seems obvious that there is textual data where the
 supression of sha1 identical files as non-conflicting isn't always
 proper.  I'm somewhat doubtful that the workflow design is really the
 problem -- I either have to remove a required feature of the broader
 design, or I have to move the problem somewhere else that isn't as
 well suited to handle it.  git seems the right tool for the job, but
 for the parameterization of merge.

    I'd be happy for some route that doesn't involve changing upstream
git, but it doesn't sound like that exists.

    Thanks!