{"thread":{"id":"33310","subject":"[PATCH] merge-tree: fix \"same file added in subdir\"","startedAt":"2013-03-27T21:34:36Z","lastAt":"2013-04-27T12:55:40Z","messageCount":8,"participants":["John Keeping","Junio C Hamano","René Scharfe"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"212460","messageId":"0a6a0c978569906b8c8d9209a85338554e503236.1364419952.git.john@keeping.me.uk","threadId":"33310","inReplyTo":null,"subject":"[PATCH] merge-tree: fix \"same file added in subdir\"","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-03-27T21:34:36Z","receivedAt":"2013-03-27T21:34:36Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"When the same file is added with identical content at the top level,\ngit-merge-tree prints \"added in both\" with the details. But if the file\nis added in an existing subdirectory, threeway_callback() bails out early\nbecause the two trees have been modified identically.\n\nIn order to detect this, we need to fall through and recurse into the\nsubtree in this case.\n\nSigned-off-by: John Keeping \n---\n builtin/merge-tree.c | 9 +++++++--\n t/t4300-merge-tree.sh | 17 +++++++++++++++++\n 2 files changed, 24 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/merge-tree.c b/builtin/merge-tree.c\nindex e0d0b7d..ca97fbd 100644\n--- a/builtin/merge-tree.c\n+++ b/builtin/merge-tree.c\n@@ -298,12 +298,17 @@ static int threeway_callback(int n, unsigned long mask, unsigned long dirmask, s\n {\n \t/* Same in both? */\n \tif (same_entry(entry+1, entry+2)) {\n-\t\tif (entry[0].sha1) {\n+\t\tif (entry[0].sha1 && !S_ISDIR(entry[0].mode)) {\n \t\t\t/* Modified identically */\n \t\t\tresolve(info, NULL, entry+1);\n \t\t\treturn mask;\n \t\t}\n-\t\t/* \"Both added the same\" is left unresolved */\n+\t\t/*\n+\t\t * \"Both added the same\" is left unresolved. We also leave\n+\t\t * \"Both directories modified identically\" unresolved in\n+\t\t * order to catch changes where the same file (with the same\n+\t\t * content) has been added to both directories.\n+\t\t */\n \t}\n \n \tif (same_entry(entry+0, entry+1)) {\ndiff --git a/t/t4300-merge-tree.sh b/t/t4300-merge-tree.sh\nindex d0b2a45..be0737e 100755\n--- a/t/t4300-merge-tree.sh\n+++ b/t/t4300-merge-tree.sh\n@@ -298,4 +298,21 @@ test_expect_success 'turn tree to file' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'add identical files to subdir' '\n+\tcat >expected <<\\EXPECTED &&\n+added in both\n+ our 100644 43d5a8ed6ef6c00ff775008633f95787d088285d sub/ONE\n+ their 100644 43d5a8ed6ef6c00ff775008633f95787d088285d sub/ONE\n+EXPECTED\n+\n+\tgit reset --hard initial &&\n+\tmkdir sub &&\n+\ttest_commit \"sub-initial\" \"sub/initial\" \"initial\" &&\n+\ttest_commit \"sub-add-a-b-same-A\" \"sub/ONE\" \"AAA\" &&\n+\tgit reset --hard sub-initial &&\n+\ttest_commit \"sub-add-a-b-same-B\" \"sub/ONE\" \"AAA\" &&\n+\tgit merge-tree sub-initial sub-add-a-b-same-A sub-add-a-b-same-B >actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_done\n-- \n1.8.2.411.g65a544e\n"},{"id":"212469","messageId":"7vk3osjwgv.fsf@alter.siamese.dyndns.org","threadId":"33310","inReplyTo":"0a6a0c978569906b8c8d9209a85338554e503236.1364419952.git.john@keeping.me.uk","subject":"Re: [PATCH] merge-tree: fix \"same file added in subdir\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-27T22:42:40Z","receivedAt":"2013-03-27T22:42:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Keeping writes:\n\n> When the same file is added with identical content at the top level,\n> git-merge-tree prints \"added in both\" with the details. But if the file\n> is added in an existing subdirectory, threeway_callback() bails out early\n> because the two trees have been modified identically.\n>\n> In order to detect this, we need to fall through and recurse into the\n> subtree in this case.\n>\n> Signed-off-by: John Keeping \n\nThe rationale the above description gives is internally consistent,\nbut it is rather sad to see this optimization go. The primary\nmotivation behind this program, which does not use the usual\nunpack-trees machinery, is to allow us to cull the identical result\nat a shallow level of the traversal when the both sides changed (not\nadded) a file deep in a subdirectory hierarchy.\n\nThe patch makes me wonder if we should go the other way around,\nresolving the \"both added identically\" case at the top cleanly\nwithout complaint.\n\n> builtin/merge-tree.c | 9 +++++++--\n> t/t4300-merge-tree.sh | 17 +++++++++++++++++\n> 2 files changed, 24 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c\n> index e0d0b7d..ca97fbd 100644\n> --- a/builtin/merge-tree.c\n> +++ b/builtin/merge-tree.c\n> @@ -298,12 +298,17 @@ static int threeway_callback(int n, unsigned long mask, unsigned long dirmask, s\n> {\n> \t/* Same in both? */\n> \tif (same_entry(entry+1, entry+2)) {\n> -\t\tif (entry[0].sha1) {\n> +\t\tif (entry[0].sha1 && !S_ISDIR(entry[0].mode)) {\n> \t\t\t/* Modified identically */\n> \t\t\tresolve(info, NULL, entry+1);\n> \t\t\treturn mask;\n> \t\t}\n> -\t\t/* \"Both added the same\" is left unresolved */\n> +\t\t/*\n> +\t\t * \"Both added the same\" is left unresolved. We also leave\n> +\t\t * \"Both directories modified identically\" unresolved in\n> +\t\t * order to catch changes where the same file (with the same\n> +\t\t * content) has been added to both directories.\n> +\t\t */\n> \t}\n> \n> \tif (same_entry(entry+0, entry+1)) {\n> diff --git a/t/t4300-merge-tree.sh b/t/t4300-merge-tree.sh\n> index d0b2a45..be0737e 100755\n> --- a/t/t4300-merge-tree.sh\n> +++ b/t/t4300-merge-tree.sh\n> @@ -298,4 +298,21 @@ test_expect_success 'turn tree to file' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success 'add identical files to subdir' '\n> +\tcat >expected <<\\EXPECTED &&\n> +added in both\n> + our 100644 43d5a8ed6ef6c00ff775008633f95787d088285d sub/ONE\n> + their 100644 43d5a8ed6ef6c00ff775008633f95787d088285d sub/ONE\n> +EXPECTED\n> +\n> +\tgit reset --hard initial &&\n> +\tmkdir sub &&\n> +\ttest_commit \"sub-initial\" \"sub/initial\" \"initial\" &&\n> +\ttest_commit \"sub-add-a-b-same-A\" \"sub/ONE\" \"AAA\" &&\n> +\tgit reset --hard sub-initial &&\n> +\ttest_commit \"sub-add-a-b-same-B\" \"sub/ONE\" \"AAA\" &&\n> +\tgit merge-tree sub-initial sub-add-a-b-same-A sub-add-a-b-same-B >actual &&\n> +\ttest_cmp expected actual\n> +'\n> +\n> test_done\n"},{"id":"212476","messageId":"20130327225739.GT2286@serenity.lan","threadId":"33310","inReplyTo":"7vk3osjwgv.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] merge-tree: fix \"same file added in subdir\"","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-03-27T22:57:39Z","receivedAt":"2013-03-27T22:57:39Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Wed, Mar 27, 2013 at 03:42:40PM -0700, Junio C Hamano wrote:\n> John Keeping writes:\n> \n> > When the same file is added with identical content at the top level,\n> > git-merge-tree prints \"added in both\" with the details. But if the file\n> > is added in an existing subdirectory, threeway_callback() bails out early\n> > because the two trees have been modified identically.\n> >\n> > In order to detect this, we need to fall through and recurse into the\n> > subtree in this case.\n> >\n> > Signed-off-by: John Keeping \n> \n> The rationale the above description gives is internally consistent,\n> but it is rather sad to see this optimization go. The primary\n> motivation behind this program, which does not use the usual\n> unpack-trees machinery, is to allow us to cull the identical result\n> at a shallow level of the traversal when the both sides changed (not\n> added) a file deep in a subdirectory hierarchy.\n> \n> The patch makes me wonder if we should go the other way around,\n> resolving the \"both added identically\" case at the top cleanly\n> without complaint.\n\nI don't use merge-tree so I have no opinion on this, just wanted to fix\nan inconsistency :-)\n\nI'll try to have a look at doing the other change tomorrow if no one\nelse gets there first.\n\n> > builtin/merge-tree.c | 9 +++++++--\n> > t/t4300-merge-tree.sh | 17 +++++++++++++++++\n> > 2 files changed, 24 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c\n> > index e0d0b7d..ca97fbd 100644\n> > --- a/builtin/merge-tree.c\n> > +++ b/builtin/merge-tree.c\n> > @@ -298,12 +298,17 @@ static int threeway_callback(int n, unsigned long mask, unsigned long dirmask, s\n> > {\n> > \t/* Same in both? */\n> > \tif (same_entry(entry+1, entry+2)) {\n> > -\t\tif (entry[0].sha1) {\n> > +\t\tif (entry[0].sha1 && !S_ISDIR(entry[0].mode)) {\n> > \t\t\t/* Modified identically */\n> > \t\t\tresolve(info, NULL, entry+1);\n> > \t\t\treturn mask;\n> > \t\t}\n> > -\t\t/* \"Both added the same\" is left unresolved */\n> > +\t\t/*\n> > +\t\t * \"Both added the same\" is left unresolved. We also leave\n> > +\t\t * \"Both directories modified identically\" unresolved in\n> > +\t\t * order to catch changes where the same file (with the same\n> > +\t\t * content) has been added to both directories.\n> > +\t\t */\n> > \t}\n> > \n> > \tif (same_entry(entry+0, entry+1)) {\n> > diff --git a/t/t4300-merge-tree.sh b/t/t4300-merge-tree.sh\n> > index d0b2a45..be0737e 100755\n> > --- a/t/t4300-merge-tree.sh\n> > +++ b/t/t4300-merge-tree.sh\n> > @@ -298,4 +298,21 @@ test_expect_success 'turn tree to file' '\n> > \ttest_cmp expect actual\n> > '\n> > \n> > +test_expect_success 'add identical files to subdir' '\n> > +\tcat >expected <<\\EXPECTED &&\n> > +added in both\n> > + our 100644 43d5a8ed6ef6c00ff775008633f95787d088285d sub/ONE\n> > + their 100644 43d5a8ed6ef6c00ff775008633f95787d088285d sub/ONE\n> > +EXPECTED\n> > +\n> > +\tgit reset --hard initial &&\n> > +\tmkdir sub &&\n> > +\ttest_commit \"sub-initial\" \"sub/initial\" \"initial\" &&\n> > +\ttest_commit \"sub-add-a-b-same-A\" \"sub/ONE\" \"AAA\" &&\n> > +\tgit reset --hard sub-initial &&\n> > +\ttest_commit \"sub-add-a-b-same-B\" \"sub/ONE\" \"AAA\" &&\n> > +\tgit merge-tree sub-initial sub-add-a-b-same-A sub-add-a-b-same-B >actual &&\n> > +\ttest_cmp expected actual\n> > +'\n> > +\n> > test_done\n"},{"id":"212498","messageId":"20130328093458.GV2286@serenity.lan","threadId":"33310","inReplyTo":"20130327225739.GT2286@serenity.lan","subject":"Re: [PATCH] merge-tree: fix \"same file added in subdir\"","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-03-28T09:34:58Z","receivedAt":"2013-03-28T09:34:58Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Wed, Mar 27, 2013 at 10:57:39PM +0000, John Keeping wrote:\n> On Wed, Mar 27, 2013 at 03:42:40PM -0700, Junio C Hamano wrote:\n> > John Keeping writes:\n> > \n> > > When the same file is added with identical content at the top level,\n> > > git-merge-tree prints \"added in both\" with the details. But if the file\n> > > is added in an existing subdirectory, threeway_callback() bails out early\n> > > because the two trees have been modified identically.\n> > >\n> > > In order to detect this, we need to fall through and recurse into the\n> > > subtree in this case.\n> > >\n> > > Signed-off-by: John Keeping \n> > \n> > The rationale the above description gives is internally consistent,\n> > but it is rather sad to see this optimization go. The primary\n> > motivation behind this program, which does not use the usual\n> > unpack-trees machinery, is to allow us to cull the identical result\n> > at a shallow level of the traversal when the both sides changed (not\n> > added) a file deep in a subdirectory hierarchy.\n> > \n> > The patch makes me wonder if we should go the other way around,\n> > resolving the \"both added identically\" case at the top cleanly\n> > without complaint.\n> \n> I don't use merge-tree so I have no opinion on this, just wanted to fix\n> an inconsistency :-)\n\nHaving re-read the manpage, I think you're right that we should just\nresolve the \"both added identically\" case cleanly, but I wonder whether\nsome of the other cases should also be resolved cleanly.\n\ngit-merge-tree(1) says:\n\n the output from the command omits entries that match the \n tree.\n\nso you could argue that \"added in branch1\", \"changed in branch1,\nunmodified in branch2\" and \"removed in branch1, unchanged in branch2\"\nshould also print no output.\n\nBut as I said above I don't use git-merge-tree so perhaps people who do\nwould like to explain what they expect in these cases.\n"},{"id":"213461","messageId":"7vk3oe2j85.fsf@alter.siamese.dyndns.org","threadId":"33310","inReplyTo":"20130328093458.GV2286@serenity.lan","subject":"Re: [PATCH] merge-tree: fix \"same file added in subdir\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-07T20:12:26Z","receivedAt":"2013-04-07T20:12:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Keeping writes:\n\n> Having re-read the manpage, I think you're right that we should just\n> resolve the \"both added identically\" case cleanly, but I wonder whether\n> some of the other cases should also be resolved cleanly.\n>\n> git-merge-tree(1) says:\n>\n> the output from the command omits entries that match the \n> tree.\n>\n> so you could argue that \"added in branch1\", \"changed in branch1,\n> unmodified in branch2\" and \"removed in branch1, unchanged in branch2\"\n> should also print no output.\n\nYes, that description explains what we wanted to achieve with\nmerge-tree that is still an unfinished experiment and fixing these\ncases you identified above to match the stated goal is a good thing\nto do. I've been meaning to find enough time to do a new merge\nstrategy backend based on the logic of this program, and the few\npatches I sent out on it recently were fallout from preparatory\nsteps for it.\n"},{"id":"213473","messageId":"20130407210751.GM2222@serenity.lan","threadId":"33310","inReplyTo":"7vk3oe2j85.fsf@alter.siamese.dyndns.org","subject":"[PATCH v2] merge-tree: don't print entries that match \"local\"","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-04-07T21:07:51Z","receivedAt":"2013-04-07T21:07:51Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"The documentation says:\n\n\tthe output from the command omits entries that match the\n\t tree.\n\nBut currently \"added in branch1\" and \"removed in branch1\" (both while\nunchanged in branch2) do print output. Change this so that the\nbehaviour matches the documentation.\n\nSigned-off-by: John Keeping \n---\n builtin/merge-tree.c | 26 +++++++++++++-------------\n t/t4300-merge-tree.sh | 10 ----------\n 2 files changed, 13 insertions(+), 23 deletions(-)\n\ndiff --git a/builtin/merge-tree.c b/builtin/merge-tree.c\nindex bc912e3..ed25d81 100644\n--- a/builtin/merge-tree.c\n+++ b/builtin/merge-tree.c\n@@ -155,6 +155,11 @@ static int same_entry(struct name_entry *a, struct name_entry *b)\n \t\ta->mode == b->mode;\n }\n \n+static int both_empty(struct name_entry *a, struct name_entry *b)\n+{\n+\treturn !(a->sha1 || b->sha1);\n+}\n+\n static struct merge_list *create_entry(unsigned stage, unsigned mode, const unsigned char *sha1, const char *path)\n {\n \tstruct merge_list *res = xcalloc(1, sizeof(*res));\n@@ -297,13 +302,10 @@ static void unresolved(const struct traverse_info *info, struct name_entry n[3])\n static int threeway_callback(int n, unsigned long mask, unsigned long dirmask, struct name_entry *entry, struct traverse_info *info)\n {\n \t/* Same in both? */\n-\tif (same_entry(entry+1, entry+2)) {\n-\t\tif (entry[0].sha1) {\n-\t\t\t/* Modified identically */\n-\t\t\tresolve(info, NULL, entry+1);\n-\t\t\treturn mask;\n-\t\t}\n-\t\t/* \"Both added the same\" is left unresolved */\n+\tif (same_entry(entry+1, entry+2) || both_empty(entry+0, entry+2)) {\n+\t\t/* Modified, added or removed identically */\n+\t\tresolve(info, NULL, entry+1);\n+\t\treturn mask;\n \t}\n \n \tif (same_entry(entry+0, entry+1)) {\n@@ -319,12 +321,10 @@ static int threeway_callback(int n, unsigned long mask, unsigned long dirmask, s\n \t\t */\n \t}\n \n-\tif (same_entry(entry+0, entry+2)) {\n-\t\tif (entry[1].sha1 && !S_ISDIR(entry[1].mode)) {\n-\t\t\t/* We modified, they did not touch -- take ours */\n-\t\t\tresolve(info, NULL, entry+1);\n-\t\t\treturn mask;\n-\t\t}\n+\tif (same_entry(entry+0, entry+2) || both_empty(entry+0, entry+2)) {\n+\t\t/* We added, modified or removed, they did not touch -- take ours */\n+\t\tresolve(info, NULL, entry+1);\n+\t\treturn mask;\n \t}\n \n \tunresolved(info, entry);\ndiff --git a/t/t4300-merge-tree.sh b/t/t4300-merge-tree.sh\nindex d0b2a45..bd43b3d 100755\n--- a/t/t4300-merge-tree.sh\n+++ b/t/t4300-merge-tree.sh\n@@ -26,8 +26,6 @@ EXPECTED\n \n test_expect_success 'file add !A, B' '\n \tcat >expected <<\\EXPECTED &&\n-added in local\n- our 100644 43d5a8ed6ef6c00ff775008633f95787d088285d ONE\n EXPECTED\n \n \tgit reset --hard initial &&\n@@ -38,9 +36,6 @@ EXPECTED\n \n test_expect_success 'file add A, B (same)' '\n \tcat >expected <<\\EXPECTED &&\n-added in both\n- our 100644 43d5a8ed6ef6c00ff775008633f95787d088285d ONE\n- their 100644 43d5a8ed6ef6c00ff775008633f95787d088285d ONE\n EXPECTED\n \n \tgit reset --hard initial &&\n@@ -181,9 +176,6 @@ AAA\" &&\n \n test_expect_success 'file remove A, !B' '\n \tcat >expected <<\\EXPECTED &&\n-removed in local\n- base 100644 43d5a8ed6ef6c00ff775008633f95787d088285d ONE\n- their 100644 43d5a8ed6ef6c00ff775008633f95787d088285d ONE\n EXPECTED\n \n \tgit reset --hard initial &&\n@@ -283,8 +275,6 @@ test_expect_success 'turn tree to file' '\n \ttest_commit \"make-file\" \"dir\" \"CCC\" &&\n \tgit merge-tree add-tree add-another-tree make-file >actual &&\n \tcat >expect <<-\\EOF &&\n-\tadded in local\n-\t our 100644 ba629238ca89489f2b350e196ca445e09d8bb834 dir/another\n \tremoved in remote\n \t base 100644 43d5a8ed6ef6c00ff775008633f95787d088285d dir/path\n \t our 100644 43d5a8ed6ef6c00ff775008633f95787d088285d dir/path\n-- \n1.8.2.694.ga76e9c3.dirty\n"},{"id":"215696","messageId":"517BB81D.10909@lsrfire.ath.cx","threadId":"33310","inReplyTo":"20130407210751.GM2222@serenity.lan","subject":"Re: [PATCH v2] merge-tree: don't print entries that match \"local\"","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2013-04-27T11:35:57Z","receivedAt":"2013-04-27T11:35:57Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 07.04.2013 23:07, schrieb John Keeping:\n> The documentation says:\n>\n> \tthe output from the command omits entries that match the\n> \t tree.\n>\n> But currently \"added in branch1\" and \"removed in branch1\" (both while\n> unchanged in branch2) do print output. Change this so that the\n> behaviour matches the documentation.\n>\n> Signed-off-by: John Keeping \n> ---\n> builtin/merge-tree.c | 26 +++++++++++++-------------\n> t/t4300-merge-tree.sh | 10 ----------\n> 2 files changed, 13 insertions(+), 23 deletions(-)\n>\n> diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c\n> index bc912e3..ed25d81 100644\n> --- a/builtin/merge-tree.c\n> +++ b/builtin/merge-tree.c\n> @@ -155,6 +155,11 @@ static int same_entry(struct name_entry *a, struct name_entry *b)\n> \t\ta->mode == b->mode;\n> }\n>\n> +static int both_empty(struct name_entry *a, struct name_entry *b)\n> +{\n> +\treturn !(a->sha1 || b->sha1);\n> +}\n> +\n> static struct merge_list *create_entry(unsigned stage, unsigned mode, const unsigned char *sha1, const char *path)\n> {\n> \tstruct merge_list *res = xcalloc(1, sizeof(*res));\n> @@ -297,13 +302,10 @@ static void unresolved(const struct traverse_info *info, struct name_entry n[3])\n> static int threeway_callback(int n, unsigned long mask, unsigned long dirmask, struct name_entry *entry, struct traverse_info *info)\n> {\n> \t/* Same in both? */\n> -\tif (same_entry(entry+1, entry+2)) {\n> -\t\tif (entry[0].sha1) {\n> -\t\t\t/* Modified identically */\n> -\t\t\tresolve(info, NULL, entry+1);\n> -\t\t\treturn mask;\n> -\t\t}\n> -\t\t/* \"Both added the same\" is left unresolved */\n> +\tif (same_entry(entry+1, entry+2) || both_empty(entry+0, entry+2)) {\n\nShouldn't this zero be a one instead?\n\n> +\t\t/* Modified, added or removed identically */\n> +\t\tresolve(info, NULL, entry+1);\n> +\t\treturn mask;\n> \t}\n>\n> \tif (same_entry(entry+0, entry+1)) {\n> @@ -319,12 +321,10 @@ static int threeway_callback(int n, unsigned long mask, unsigned long dirmask, s\n> \t\t */\n> \t}\n>\n> -\tif (same_entry(entry+0, entry+2)) {\n> -\t\tif (entry[1].sha1 && !S_ISDIR(entry[1].mode)) {\n> -\t\t\t/* We modified, they did not touch -- take ours */\n> -\t\t\tresolve(info, NULL, entry+1);\n> -\t\t\treturn mask;\n> -\t\t}\n> +\tif (same_entry(entry+0, entry+2) || both_empty(entry+0, entry+2)) {\n\nOtherwise the both_empty check here can be removed because we'd have \nalready returned above if it was true.\n\nBut do we actually want to resolve the removal of a file on both sides \nsilently? The added comment above says so, but the commit message \ndoesn't mention it.\n\n> +\t\t/* We added, modified or removed, they did not touch -- take ours */\n> +\t\tresolve(info, NULL, entry+1);\n> +\t\treturn mask;\n> \t}\n>\n> \tunresolved(info, entry);\n> diff --git a/t/t4300-merge-tree.sh b/t/t4300-merge-tree.sh\n> index d0b2a45..bd43b3d 100755\n> --- a/t/t4300-merge-tree.sh\n> +++ b/t/t4300-merge-tree.sh\n> @@ -26,8 +26,6 @@ EXPECTED\n>\n> test_expect_success 'file add !A, B' '\n> \tcat >expected <<\\EXPECTED &&\n> -added in local\n> - our 100644 43d5a8ed6ef6c00ff775008633f95787d088285d ONE\n> EXPECTED\n>\n> \tgit reset --hard initial &&\n> @@ -38,9 +36,6 @@ EXPECTED\n>\n> test_expect_success 'file add A, B (same)' '\n> \tcat >expected <<\\EXPECTED &&\n> -added in both\n> - our 100644 43d5a8ed6ef6c00ff775008633f95787d088285d ONE\n> - their 100644 43d5a8ed6ef6c00ff775008633f95787d088285d ONE\n> EXPECTED\n>\n> \tgit reset --hard initial &&\n> @@ -181,9 +176,6 @@ AAA\" &&\n>\n> test_expect_success 'file remove A, !B' '\n> \tcat >expected <<\\EXPECTED &&\n> -removed in local\n> - base 100644 43d5a8ed6ef6c00ff775008633f95787d088285d ONE\n> - their 100644 43d5a8ed6ef6c00ff775008633f95787d088285d ONE\n> EXPECTED\n>\n> \tgit reset --hard initial &&\n> @@ -283,8 +275,6 @@ test_expect_success 'turn tree to file' '\n> \ttest_commit \"make-file\" \"dir\" \"CCC\" &&\n> \tgit merge-tree add-tree add-another-tree make-file >actual &&\n> \tcat >expect <<-\\EOF &&\n> -\tadded in local\n> -\t our 100644 ba629238ca89489f2b350e196ca445e09d8bb834 dir/another\n> \tremoved in remote\n> \t base 100644 43d5a8ed6ef6c00ff775008633f95787d088285d dir/path\n> \t our 100644 43d5a8ed6ef6c00ff775008633f95787d088285d dir/path\n>\n"},{"id":"215698","messageId":"20130427125540.GD472@serenity.lan","threadId":"33310","inReplyTo":"517BB81D.10909@lsrfire.ath.cx","subject":"Re: [PATCH v2] merge-tree: don't print entries that match \"local\"","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-04-27T12:55:40Z","receivedAt":"2013-04-27T12:55:40Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Sat, Apr 27, 2013 at 01:35:57PM +0200, René Scharfe wrote:\n> Am 07.04.2013 23:07, schrieb John Keeping:\n> > The documentation says:\n> >\n> > \tthe output from the command omits entries that match the\n> > \t tree.\n> >\n> > But currently \"added in branch1\" and \"removed in branch1\" (both while\n> > unchanged in branch2) do print output. Change this so that the\n> > behaviour matches the documentation.\n> >\n> > Signed-off-by: John Keeping \n> > ---\n> > builtin/merge-tree.c | 26 +++++++++++++-------------\n> > t/t4300-merge-tree.sh | 10 ----------\n> > 2 files changed, 13 insertions(+), 23 deletions(-)\n> >\n> > diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c\n> > index bc912e3..ed25d81 100644\n> > --- a/builtin/merge-tree.c\n> > +++ b/builtin/merge-tree.c\n> > @@ -155,6 +155,11 @@ static int same_entry(struct name_entry *a, struct name_entry *b)\n> > \t\ta->mode == b->mode;\n> > }\n> >\n> > +static int both_empty(struct name_entry *a, struct name_entry *b)\n> > +{\n> > +\treturn !(a->sha1 || b->sha1);\n> > +}\n> > +\n> > static struct merge_list *create_entry(unsigned stage, unsigned mode, const unsigned char *sha1, const char *path)\n> > {\n> > \tstruct merge_list *res = xcalloc(1, sizeof(*res));\n> > @@ -297,13 +302,10 @@ static void unresolved(const struct traverse_info *info, struct name_entry n[3])\n> > static int threeway_callback(int n, unsigned long mask, unsigned long dirmask, struct name_entry *entry, struct traverse_info *info)\n> > {\n> > \t/* Same in both? */\n> > -\tif (same_entry(entry+1, entry+2)) {\n> > -\t\tif (entry[0].sha1) {\n> > -\t\t\t/* Modified identically */\n> > -\t\t\tresolve(info, NULL, entry+1);\n> > -\t\t\treturn mask;\n> > -\t\t}\n> > -\t\t/* \"Both added the same\" is left unresolved */\n> > +\tif (same_entry(entry+1, entry+2) || both_empty(entry+0, entry+2)) {\n> \n> Shouldn't this zero be a one instead?\n\nYes, that's a typo.\n\n> > +\t\t/* Modified, added or removed identically */\n> > +\t\tresolve(info, NULL, entry+1);\n> > +\t\treturn mask;\n> > \t}\n> >\n> > \tif (same_entry(entry+0, entry+1)) {\n> > @@ -319,12 +321,10 @@ static int threeway_callback(int n, unsigned long mask, unsigned long dirmask, s\n> > \t\t */\n> > \t}\n> >\n> > -\tif (same_entry(entry+0, entry+2)) {\n> > -\t\tif (entry[1].sha1 && !S_ISDIR(entry[1].mode)) {\n> > -\t\t\t/* We modified, they did not touch -- take ours */\n> > -\t\t\tresolve(info, NULL, entry+1);\n> > -\t\t\treturn mask;\n> > -\t\t}\n> > +\tif (same_entry(entry+0, entry+2) || both_empty(entry+0, entry+2)) {\n> \n> Otherwise the both_empty check here can be removed because we'd have \n> already returned above if it was true.\n> \n> But do we actually want to resolve the removal of a file on both sides \n> silently? The added comment above says so, but the commit message \n> doesn't mention it.\n\nI think so. The rule is that we omit things that match the branch1\ntree. So if it is removed in branch1 and there isn't a conflict with\nbranch2 then we shouldn't be printing any output.\n\n> > +\t\t/* We added, modified or removed, they did not touch -- take ours */\n> > +\t\tresolve(info, NULL, entry+1);\n> > +\t\treturn mask;\n> > \t}\n> >\n> > \tunresolved(info, entry);\n> > diff --git a/t/t4300-merge-tree.sh b/t/t4300-merge-tree.sh\n> > index d0b2a45..bd43b3d 100755\n> > --- a/t/t4300-merge-tree.sh\n> > +++ b/t/t4300-merge-tree.sh\n> > @@ -26,8 +26,6 @@ EXPECTED\n> >\n> > test_expect_success 'file add !A, B' '\n> > \tcat >expected <<\\EXPECTED &&\n> > -added in local\n> > - our 100644 43d5a8ed6ef6c00ff775008633f95787d088285d ONE\n> > EXPECTED\n> >\n> > \tgit reset --hard initial &&\n> > @@ -38,9 +36,6 @@ EXPECTED\n> >\n> > test_expect_success 'file add A, B (same)' '\n> > \tcat >expected <<\\EXPECTED &&\n> > -added in both\n> > - our 100644 43d5a8ed6ef6c00ff775008633f95787d088285d ONE\n> > - their 100644 43d5a8ed6ef6c00ff775008633f95787d088285d ONE\n> > EXPECTED\n> >\n> > \tgit reset --hard initial &&\n> > @@ -181,9 +176,6 @@ AAA\" &&\n> >\n> > test_expect_success 'file remove A, !B' '\n> > \tcat >expected <<\\EXPECTED &&\n> > -removed in local\n> > - base 100644 43d5a8ed6ef6c00ff775008633f95787d088285d ONE\n> > - their 100644 43d5a8ed6ef6c00ff775008633f95787d088285d ONE\n> > EXPECTED\n> >\n> > \tgit reset --hard initial &&\n> > @@ -283,8 +275,6 @@ test_expect_success 'turn tree to file' '\n> > \ttest_commit \"make-file\" \"dir\" \"CCC\" &&\n> > \tgit merge-tree add-tree add-another-tree make-file >actual &&\n> > \tcat >expect <<-\\EOF &&\n> > -\tadded in local\n> > -\t our 100644 ba629238ca89489f2b350e196ca445e09d8bb834 dir/another\n> > \tremoved in remote\n> > \t base 100644 43d5a8ed6ef6c00ff775008633f95787d088285d dir/path\n> > \t our 100644 43d5a8ed6ef6c00ff775008633f95787d088285d dir/path\n> >\n> \n"}]}