{"thread":{"id":"37987","subject":"git merge a b when a == b but neither == o is always a successful merge?","startedAt":"2014-11-17T18:39:43Z","lastAt":"2014-11-24T19:36:02Z","messageCount":6,"participants":["Daniel Hagerty","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"251994","messageId":"21610.16623.746985.383838@perdition.linnaean.org","threadId":"37987","inReplyTo":null,"subject":"git merge a b when a == b but neither == o is always a successful merge?","fromName":"Daniel Hagerty","fromEmail":"hag@linnaean.org","sentAt":"2014-11-17T18:39:43Z","receivedAt":"2014-11-17T18:39:43Z","isPatch":false,"sender":{"key":"hag@linnaean.org","avatar":null},"body":"    Given a repository setup thusly:\n\n$ git --version\ngit version 2.2.0.rc2\n\ngit init .\n\necho '0.0' > version\ngit add version\ngit commit -m \"master\"\nfor i in a b ; do\n  git checkout -b $i master\n\n  echo '0.1' > version\n  git commit -a -m \"leg $i\"\ndone\n\ngit checkout -b c master\necho '0.2' > version\ngit commit -a -m \"leg c\"\n\ngit checkout --detach a\n\n\n\"git merge c\" produces the expected edit conflict.\n\n\"git merge b\" produces a successful merge, as both branches perform\nthe \"same\" work.\n\nFor the body of content in question, this is a merge conflict.  Git\nseems to have the hard-coded assumption otherwise.  I had to change\nthree source files to get the result I expected, and wasn't seeing\nany indications of parameterization.\n\nAm I missing some means of getting the results I need?  Thanks!\n"},{"id":"252001","messageId":"20141117205304.GA15880@peff.net","threadId":"37987","inReplyTo":"21610.16623.746985.383838@perdition.linnaean.org","subject":"Re: git merge a b when a == b but neither == o is always a successful merge?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-11-17T20:53:04Z","receivedAt":"2014-11-17T20:53:04Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 17, 2014 at 01:39:43PM -0500, Daniel Hagerty wrote:\n\n> \"git merge b\" produces a successful merge, as both branches perform\n> the \"same\" work.\n\nJust to be clear, you were expecting \"git merge b\" to produce a\nconflict?\n\n> For the body of content in question, this is a merge conflict.  Git\n> seems to have the hard-coded assumption otherwise.  I had to change\n> three source files to get the result I expected, and wasn't seeing\n> any indications of parameterization.\n\nI can imagine there might be times you would like to notice this case\nand visit it manually (e.g., even though the conflict would show both\nsides with the same content, you might want the resolution to take the\ntwo sides sequentially, duplicating them).  But there are also cases\nwhere choosing the new content is helpful (e.g., one side cherry-picks a\ncommit from the other, then later merges; you would not want to see a\nconflict there).\n\n> Am I missing some means of getting the results I need?  Thanks!\n\nI don't think there is an easy way to get what you want. You would have\nto write a new merge 3-way strategy that handles this case differently.\nAnd most of the file-level heavy lifting in merge strategies is done by\nthe low-level unpack_trees code, which handles this case. From \"git help\nread-tree\", which describes the index-level 3-way merge:\n\n  ·   stage 2 and 3 are the same; take one or the other (it makes no\n      difference - the same work has been done on our branch in stage 2\n      and their branch in stage 3)\n\nSo I think you would have to add an option to git to handle this, unless\nyou want to reimplement quite a lot of code in your merge strategy.\n\n-Peff\n"},{"id":"252007","messageId":"21610.29903.366230.851787@perdition.linnaean.org","threadId":"37987","inReplyTo":"20141117205304.GA15880@peff.net","subject":"Re: git merge a b when a == b but neither == o is always a successful merge?","fromName":"Daniel Hagerty","fromEmail":"hag@linnaean.org","sentAt":"2014-11-17T22:21:03Z","receivedAt":"2014-11-17T22:21:03Z","isPatch":false,"sender":{"key":"hag@linnaean.org","avatar":null},"body":" > Just to be clear, you were expecting \"git merge b\" to produce a\n > conflict?\n\n    Yessir.\n\n > I can imagine there might be times you would like to notice this case\n > and visit it manually (e.g., even though the conflict would show both\n > sides with the same content, you might want the resolution to take the\n > two sides sequentially, duplicating them).\n\n    This is roughly the case here.  Judgement is needed from the\nperson at the wheel.\n\n > I don't think there is an easy way to get what you want. You would have\n > to write a new merge 3-way strategy that handles this case differently.\n > And most of the file-level heavy lifting in merge strategies is done by\n > the low-level unpack_trees code, which handles this case. From \"git help\n\n    I have a very rough draft that seems to do what I want, exposed\nthrough .gitattributes (below).  Given that this is something you probably\nwant tightly scoped, does it make sense to expose it anywhere else?\n\n    Is it accurate to speak of this as exposing merge minimal?\n\n    Thanks!\n\ncommit 3a8bc89950576c0445167e4f5ee5f42d9d737d2d\nAuthor: Daniel Hagerty <hag@linnaean.org>\nDate:   Mon Nov 17 15:42:04 2014 -0500\n\n    merge: expose XDL_MERGE_MINIMAL behavior:\n\n    When performing a 3-way merge, an identical change to a file in both\n    branches is not considered a conflict by default.  There exist content\n    for which this isn't true; simultaneous, identical edits are a\n    conflict.\n\n    Expose the ability to perform a minimal merge for selective files as\n    directed by gitattributes.\n\ndiff --git a/ll-merge.c b/ll-merge.c\nindex da59738..2a06ac9 100644\n--- a/ll-merge.c\n+++ b/ll-merge.c\n@@ -84,7 +84,16 @@ static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,\n \t}\n\n \tmemset(&xmp, 0, sizeof(xmp));\n-\txmp.level = XDL_MERGE_ZEALOUS;\n+\n+\tstruct git_attr_check check;\n+\tcheck.attr = git_attr(\"merge-minimal\");\n+\t(void) git_check_attr(path, 1, &check);\n+\n+\tif(ATTR_TRUE(check.value))\n+\t  xmp.level = XDL_MERGE_MINIMAL;\n+\telse\n+\t  xmp.level = XDL_MERGE_ZEALOUS;\n+\n \txmp.favor = opts->variant;\n \txmp.xpp.flags = opts->xdl_opts;\n \tif (git_xmerge_style >= 0)\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 3efc04e..dac6252 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -924,7 +924,11 @@ static struct merge_file_info merge_file_1(struct merge_options *o,\n \t\t\t}\n \t\t}\n\n-\t\tif (sha_eq(a->sha1, b->sha1) || sha_eq(a->sha1, one->sha1))\n+\t\tstruct git_attr_check check;\n+\t\tcheck.attr = git_attr(\"merge-minimal\");\n+\t\t(void) git_check_attr(a->path, 1, &check);\n+\n+\t\tif (!ATTR_TRUE(check.value) && (sha_eq(a->sha1, b->sha1) || sha_eq(a->sha1, one->sha1)))\n \t\t\thashcpy(result.sha, b->sha1);\n \t\telse if (sha_eq(b->sha1, one->sha1))\n \t\t\thashcpy(result.sha, a->sha1);\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex cc616c3..627356a 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1597,7 +1597,12 @@ int threeway_merge(struct cache_entry **stages, struct unpack_trees_options *o)\n\n \tif (head) {\n \t\t/* #5ALT, #15 */\n-\t\tif (same(head, remote))\n+\n+\t\tstruct git_attr_check check;\n+\t\tcheck.attr = git_attr(\"merge-minimal\");\n+\t\t(void) git_check_attr(head->name, 1, &check);\n+\n+\t\tif (!ATTR_TRUE(check.value) && same(head, remote))\n \t\t\treturn merged_entry(head, index, o);\n \t\t/* #13, #3ALT */\n \t\tif (!df_conflict_remote && remote_match && !head_match)\n"},{"id":"252355","messageId":"20141121181539.GC26650@peff.net","threadId":"37987","inReplyTo":"21610.29903.366230.851787@perdition.linnaean.org","subject":"Re: git merge a b when a == b but neither == o is always a successful merge?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-11-21T18:15:40Z","receivedAt":"2014-11-21T18:15:40Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 17, 2014 at 05:21:03PM -0500, Daniel Hagerty wrote:\n\n>  > I don't think there is an easy way to get what you want. You would have\n>  > to write a new merge 3-way strategy that handles this case differently.\n>  > And most of the file-level heavy lifting in merge strategies is done by\n>  > the low-level unpack_trees code, which handles this case. From \"git help\n> \n>     I have a very rough draft that seems to do what I want, exposed\n> through .gitattributes (below).  Given that this is something you probably\n> want tightly scoped, does it make sense to expose it anywhere else?\n\nThis is the first use of gitattributes in the unpack-trees code path. I\ncannot think offhand of any philosophical reason that should not be OK,\nbut it is something worth considering (i.e., this code path is deep\nplumbing; are there cases where we would not want to support\ngitattributes?).\n\n>     Is it accurate to speak of this as exposing merge minimal?\n\nI am not 100% sure they are not orthogonal concepts. The tree-diff is\nabout \"just because two separate changes ended at the same place does\nnot mean they should be merged into the same change\". I am actually not\nsure what XDL_MERGE_MINIMAL entails exactly. Is it only about coalescing\noverlapping endpoints in the merge? Documentation is sparse.\n\nThe patch itself doesn't look too bad. It covers the file-level case and\nthe content-level case. Is there additional code needed to handle the\ndirectory-level case? That is, if I have a tree that starts at\nsha1 X, and ends up at sha1 Y in both sides of the merge, do we still\ndescend into it to make the file-level comparisons, or do we optimize\nthat out?\n\n> diff --git a/ll-merge.c b/ll-merge.c\n> index da59738..2a06ac9 100644\n> --- a/ll-merge.c\n> +++ b/ll-merge.c\n> @@ -84,7 +84,16 @@ static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,\n>  \t}\n> \n>  \tmemset(&xmp, 0, sizeof(xmp));\n> -\txmp.level = XDL_MERGE_ZEALOUS;\n> +\n> +\tstruct git_attr_check check;\n> +\tcheck.attr = git_attr(\"merge-minimal\");\n> +\t(void) git_check_attr(path, 1, &check);\n\nPlease void C99 decl-after-statement, as we build on older compilers\nthat do not like it.\n\nWe also do not typically annotate ignored return values.\n\n> +\tif(ATTR_TRUE(check.value))\n> +\t  xmp.level = XDL_MERGE_MINIMAL;\n> +\telse\n> +\t  xmp.level = XDL_MERGE_ZEALOUS;\n\n..and indentations should be a single tab.\n\nOther than those minor style nits, I do not see anything obviously\n_wrong_ with the patch, but I am far from an expert in the area. It\nwould need documentation and tests, of course. The next step may be to\nwrap it up with those things and post it to the list, which will likely\nget more attention.\n\n-Peff\n"},{"id":"252357","messageId":"xmqqwq6oz8sv.fsf@gitster.dls.corp.google.com","threadId":"37987","inReplyTo":"20141121181539.GC26650@peff.net","subject":"Re: git merge a b when a == b but neither == o is always a successful merge?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-21T18:51:28Z","receivedAt":"2014-11-21T18:51:28Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> This is the first use of gitattributes in the unpack-trees code path. I\n> cannot think offhand of any philosophical reason that should not be OK,\n> but it is something worth considering (i.e., this code path is deep\n> plumbing; are there cases where we would not want to support\n> gitattributes?).\n\nIt is not about \"not want to support\", but mroe about \"not want to\nbe affected (by user preference)\", and it is a valid concern.\n\n> ... if I have a tree that starts at\n> sha1 X, and ends up at sha1 Y in both sides of the merge, do we still\n> descend into it to make the file-level comparisons, or do we optimize\n> that out?\n\nThe code to look at is unpack-trees.c::unpack_callback(); even\nthough it does try to be clever to do that exact optimization when\nit is used for \"diff-index --cached\", I do not think we do it during\na real merge.\n>> +\tstruct git_attr_check check;\n>> +\tcheck.attr = git_attr(\"merge-minimal\");\n>> +\t(void) git_check_attr(path, 1, &check);\n>\n> Please void C99 decl-after-statement, as we build on older compilers\n> that do not like it.\n\nAlso I do not think we want to keep calling git_attr() interning\nfunction.  All existing uses (well, at least the ones that were\nwritten by those who know what they are doing ;-) should be avoiding\nrepeated look-ups, and if we are to use an attribute for this, we\nshould do the same.\n\nDo we name our attributes with \"dashes-in-its-name\"?  I am not\nasking if dashes are allowed (I know it is), but if that breaks\nthe pattern already established for the core part of the system\nand makes this new one an oddball.\n\n> We also do not typically annotate ignored return values.\n\nTrue.\n\n>\n>> +\tif(ATTR_TRUE(check.value))\n>> +\t  xmp.level = XDL_MERGE_MINIMAL;\n>> +\telse\n>> +\t  xmp.level = XDL_MERGE_ZEALOUS;\n>\n> ..and indentations should be a single tab.\n\nTrue, too.\n\n> Other than those minor style nits, I do not see anything obviously\n> _wrong_ with the patch, but I am far from an expert in the area.\n\nI agree that the approach taken here is a sensible way to implement\nthe design, _if_ the design and the problem it tries to solve makes\nsense.  I am not sure about that yet myself, though.\n"},{"id":"252442","messageId":"21619.34978.293406.32338@perdition.linnaean.org","threadId":"37987","inReplyTo":"xmqqwq6oz8sv.fsf@gitster.dls.corp.google.com","subject":"Re: git merge a b when a == b but neither == o is always a successful merge?","fromName":"Daniel Hagerty","fromEmail":"hag@linnaean.org","sentAt":"2014-11-24T19:36:02Z","receivedAt":"2014-11-24T19:36:02Z","isPatch":false,"sender":{"key":"hag@linnaean.org","avatar":null},"body":" > I agree that the approach taken here is a sensible way to implement\n > the design, _if_ the design and the problem it tries to solve makes\n > sense.  I am not sure about that yet myself, though.\n\n    This is a \"first things first\".\n\n    What aspect of the problem to be solved is in question?\n\n    From here, it seems obvious that there is textual data where the\n supression of sha1 identical files as non-conflicting isn't always\n proper.  I'm somewhat doubtful that the workflow design is really the\n problem -- I either have to remove a required feature of the broader\n design, or I have to move the problem somewhere else that isn't as\n well suited to handle it.  git seems the right tool for the job, but\n for the parameterization of merge.\n\n    I'd be happy for some route that doesn't involve changing upstream\ngit, but it doesn't sound like that exists.\n\n    Thanks!\n"}]}