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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 21, 2014, 18:51 UTC
Message-ID: <xmqqwq6oz8sv.fsf@gitster.dls.corp.google.com>
In-Reply-To: <20141121181539.GC26650@peff.net>

Jeff King <peff@peff.net> writes:

Show 5 quoted lines

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

Show 6 quoted lines

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

Show 7 quoted lines

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