Jeff King <peff@xxxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html