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