{"thread":{"id":"24368","subject":"[PATCHv2 2/2] fix merge-tree where two branches share no changes","startedAt":"2010-07-11T13:16:53Z","lastAt":"2010-07-14T16:26:46Z","messageCount":6,"participants":["Will Palmer","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"145326","messageId":"1278854215-9022-1-git-send-email-wmpalmer@gmail.com","threadId":"24368","inReplyTo":null,"subject":"[PATCHv2 0/2] merge-tree: fix (merge-base a b) b a","fromName":"Will Palmer","fromEmail":"wmpalmer@gmail.com","sentAt":"2010-07-11T13:16:53Z","receivedAt":"2010-07-11T13:16:53Z","isPatch":false,"sender":{"key":"wmpalmer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/357044?v=4"},"body":"This series notes, then fixes, a regression introduced by\n15b4f7a68d8c3c8ee28424415b203f61202d65d1 /\n\tmerge-tree: use ll_merge() not xdl_merge()\n\nI don't know the proper terminology to describe what's being fixed here.\nThis seems to most-easily be triggered by (for example):\n\tgit merge-tree $(git merge-base HEAD @{u}) HEAD @{u}\n\nIn the git repository at the moment, this could be triggered with:\n\tgit merge-tree $(git merge-base origin/next origin/master) \\\n\t\torigin/next origin/master\n\nThough as I write this, next has only just been merged with master, so\nthat is not the case. For an example which is less likely to go away,\ntry:\n\tgit merge-tree c9eaaab4165d8f402930d12899ec097495b599e6 \\\n\t\tbe16ac8cc8ce693c6adf37b80db65d10a41b4eb9 \\\n\t\t9918285fb10d81af9021dae99c5f4de88ded497c\n\nIt's actually very trivial to reproduce this, to the point where I\ncan't help but wonder how much merge-tree is actually being used. As\nI narrowed the test-case more and more, I was surprised by how little\nit took to trigger it. The first patch in this series includes some\nvery basic tests for merge-tree, the last of which demonstrates the\nregression.\n\nThe second patch implements the trivial fix for it.\n\nThis is the second version of the patch. The first version of the patch\nincluded very trivial tests for merge-tree, which were really only\nintended to demonstrate the breakage being fixed. This version of the\npatch includes proper tests, with real \"expected vs actual\" testing,\nrather than dumb \"did it return successfully?\" checks.\n\nWill Palmer (2):\n add basic tests for merge-tree\n fix merge-tree where two branches share no changes\n\n builtin/merge-tree.c | 3 +-\n t/t4300-merge-tree.sh | 277 +++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 279 insertions(+), 1 deletions(-)\n create mode 100755 t/t4300-merge-tree.sh\n\n-- \n1.7.1.703.g42c01\n"},{"id":"145327","messageId":"1278854215-9022-2-git-send-email-wmpalmer@gmail.com","threadId":"24368","inReplyTo":"1278854215-9022-1-git-send-email-wmpalmer@gmail.com","subject":"[PATCHv2 1/2] add basic tests for merge-tree","fromName":"Will Palmer","fromEmail":"wmpalmer@gmail.com","sentAt":"2010-07-11T13:16:54Z","receivedAt":"2010-07-11T13:16:54Z","isPatch":false,"sender":{"key":"wmpalmer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/357044?v=4"},"body":"merge-tree had no test cases, so here we add some very basic tests for\nit, including some known-breakages.\n\nSigned-off-by: Will Palmer \n---\n t/t4300-merge-tree.sh | 277 +++++++++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 277 insertions(+), 0 deletions(-)\n create mode 100755 t/t4300-merge-tree.sh\n\ndiff --git a/t/t4300-merge-tree.sh b/t/t4300-merge-tree.sh\nnew file mode 100755\nindex 0000000..b2ae3a1\n--- /dev/null\n+++ b/t/t4300-merge-tree.sh\n@@ -0,0 +1,277 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2010 Will Palmer\n+#\n+\n+test_description='git merge-tree'\n+. ./test-lib.sh\n+\n+test_expect_success setup '\n+\ttest_commit \"initial\" \"initial-file\" \"initial\"\n+'\n+\n+test_expect_'file add A, !B' '\n+\tcat >expected <actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_failure 'file add !A, B' '\n+\tcat >expected <actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'file add A, B (same)' '\n+\tcat >expected <actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'file add A, B (different)' '\n+\tcat >expected <>>>>>> .their\n+EXPECTED\n+\n+\tgit reset --hard initial\n+\ttest_commit \"add-a-b-diff-A\" \"ONE\" \"AAA\"\n+\n+\tgit reset --hard initial\n+\ttest_commit \"add-a-b-diff-B\" \"ONE\" \"BBB\"\n+\n+\tgit merge-tree initial add-a-b-diff-A add-a-b-diff-B >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'file change A, !B' '\n+\tcat >expected <actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'file change !A, B' '\n+\tcat >expected <actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'file change A, B (same)' '\n+\tcat >expected <actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'file change A, B (different)' '\n+\tcat >expected <>>>>>> .their\n+EXPECTED\n+\n+\tgit reset --hard initial\n+\ttest_commit \"change-a-b-diff-A\" \"initial-file\" \"AAA\"\n+\n+\tgit reset --hard initial\n+\ttest_commit \"change-a-b-diff-B\" \"initial-file\" \"BBB\"\n+\n+\tgit merge-tree initial change-a-b-diff-A change-a-b-diff-B >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'file change A, B (mixed)' '\n+\tcat >expected <>>>>>> .their\n+ AAA\n+ AAA\n+ AAA\n+EXPECTED\n+\n+\tgit reset --hard initial\n+\ttest_commit \"change-a-b-mix-base\" \"ONE\" \"\n+AAA\n+AAA\n+AAA\n+AAA\n+AAA\n+AAA\n+AAA\n+AAA\n+AAA\n+AAA\n+AAA\n+AAA\n+AAA\n+AAA\n+AAA\"\n+\n+\ttest_commit \"change-a-b-mix-A\" \"ONE\" \\\n+\t\t\"$(sed -e \"1{s/AAA/BBB/;}\" -e \"10{s/AAA/BBB/;}\" actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'file remove A, !B' '\n+\tcat >expected <actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_failure 'file remove !A, B' '\n+\tcat >expected <actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_failure 'file change A, remove B' '\n+\tcat >expected <actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'file remove A, change B' '\n+\tcat >expected <actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_done\n-- \n1.7.1.703.g42c01\n"},{"id":"145325","messageId":"1278854215-9022-3-git-send-email-wmpalmer@gmail.com","threadId":"24368","inReplyTo":"1278854215-9022-1-git-send-email-wmpalmer@gmail.com","subject":"[PATCHv2 2/2] fix merge-tree where two branches share no changes","fromName":"Will Palmer","fromEmail":"wmpalmer@gmail.com","sentAt":"2010-07-11T13:16:55Z","receivedAt":"2010-07-11T13:16:55Z","isPatch":false,"sender":{"key":"wmpalmer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/357044?v=4"},"body":"Here we fix a regression which was introduced by\n15b4f7a68d8c3c8ee28424415b203f61202d65d1 /\n\tmerge-tree: use ll_merge() not xdl_merge()\n\nWhich caused merge-tree to segfault in particular combinations of\nmerging files which existed in one branch, but not in the other or in\nthe merge-base. This was caused by referencing entry->path at a time\nwhen entry was known to be possibly-NULL.\n\nTo correct the problem, we save the path of the entry we came in with,\nas the path should be the same among all the stages no matter which\nsides are involved in the merge.\n\nSigned-off-by: Will Palmer \n---\n builtin/merge-tree.c | 3 ++-\n t/t4300-merge-tree.sh | 6 +++---\n 2 files changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/merge-tree.c b/builtin/merge-tree.c\nindex fc00d79..9b25ddc 100644\n--- a/builtin/merge-tree.c\n+++ b/builtin/merge-tree.c\n@@ -60,6 +60,7 @@ static void *result(struct merge_list *entry, unsigned long *size)\n {\n \tenum object_type type;\n \tstruct blob *base, *our, *their;\n+\tconst char *path = entry->path;\n \n \tif (!entry->stage)\n \t\treturn read_sha1_file(entry->blob->object.sha1, &type, size);\n@@ -76,7 +77,7 @@ static void *result(struct merge_list *entry, unsigned long *size)\n \ttheir = NULL;\n \tif (entry)\n \t\ttheir = entry->blob;\n-\treturn merge_file(entry->path, base, our, their, size);\n+\treturn merge_file(path, base, our, their, size);\n }\n \n static void *origin(struct merge_list *entry, unsigned long *size)\ndiff --git a/t/t4300-merge-tree.sh b/t/t4300-merge-tree.sh\nindex b2ae3a1..5feff91 100755\n--- a/t/t4300-merge-tree.sh\n+++ b/t/t4300-merge-tree.sh\n@@ -25,7 +25,7 @@ EXPECTED\n \ttest_cmp expected actual\n '\n \n-test_expect_failure 'file add !A, B' '\n+test_expect_success 'file add !A, B' '\n \tcat >expected <expected <expected < writes:\n\n> merge-tree had no test cases, so here we add some very basic tests for\n> it, including some known-breakages.\n>\n> Signed-off-by: Will Palmer \n> ---\n> t/t4300-merge-tree.sh | 277 +++++++++++++++++++++++++++++++++++++++++++++++++\n> 1 files changed, 277 insertions(+), 0 deletions(-)\n> create mode 100755 t/t4300-merge-tree.sh\n\nThis does not sound like part of \"the diff commands\", unlike apply/diff/log.\nPerhaps somewhere in t3xxx \"other basic commands\" series?\n\n> diff --git a/t/t4300-merge-tree.sh b/t/t4300-merge-tree.sh\n> new file mode 100755\n> index 0000000..b2ae3a1\n> --- /dev/null\n> +++ b/t/t4300-merge-tree.sh\n> @@ -0,0 +1,277 @@\n> ...\n> +test_expect_'file add A, !B' '\n> +\tcat >expected < +added in remote\n> + their 100644 43d5a8ed6ef6c00ff775008633f95787d088285d ONE\n> +@@ -0,0 +1 @@\n> ++AAA\n> +EXPECTED\n\nSince you do not use any variable substitution in the here text, please\nmake that clear by quoting EXPECTED like this:\n\n\tcat >expected <<\\EXPECTED\n... your here text goes here ...\nEXPECTED\n\nto help the readers, who otherwise have to scan the here text to find if\nthere is any interesting substitution going on.\n\n> +\tgit reset --hard initial\n> +\ttest_commit \"add-a-not-b\" \"ONE\" \"AAA\"\n\nPlease fix missing && cascades.\n\nThanks.\n"},{"id":"145517","messageId":"1279090893.2789.21.camel@dreddbeard","threadId":"24368","inReplyTo":"7vaapv0xin.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv2 1/2] add basic tests for merge-tree","fromName":"Will Palmer","fromEmail":"wmpalmer@gmail.com","sentAt":"2010-07-14T07:01:33Z","receivedAt":"2010-07-14T07:01:33Z","isPatch":false,"sender":{"key":"wmpalmer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/357044?v=4"},"body":"On Tue, 2010-07-13 at 16:31 -0700, Junio C Hamano wrote:\n> Will Palmer writes:\n> \n> > merge-tree had no test cases, so here we add some very basic tests for\n> > it, including some known-breakages.\n> >\n> > Signed-off-by: Will Palmer \n> > ---\n> > t/t4300-merge-tree.sh | 277 +++++++++++++++++++++++++++++++++++++++++++++++++\n> > 1 files changed, 277 insertions(+), 0 deletions(-)\n> > create mode 100755 t/t4300-merge-tree.sh\n> \n> This does not sound like part of \"the diff commands\", unlike apply/diff/log.\n> Perhaps somewhere in t3xxx \"other basic commands\" series?\n\nI was a bit iffy on this one. Part of this was, admittedly, because\nt3[0-9] were all taken, and I'm not sure about what number should be\nused after that (perhaps t/README needs to be updated? It only mentions\nthree digits, but lists the template as tNNNN)\n\nHowever, merge-tree has a function similar to diff (it shows differences\nbetween trees) and apply (it creates the \"result\" object when there are\nno conflicts), and its purpose seems to be more of a \"shower\" than a\n\"do-er\" (contrasted with something like git-merge, which actually\ncreates a commit object, for example), so I thought that diff-commands\nwould be a good classification.\n\nIf you can tell me what number it should be, I'll change it.\n\n\n-- \n-- Will\n"},{"id":"145554","messageId":"7vwrsyxc55.fsf@alter.siamese.dyndns.org","threadId":"24368","inReplyTo":"1279090893.2789.21.camel@dreddbeard","subject":"Re: [PATCHv2 1/2] add basic tests for merge-tree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-07-14T16:26:46Z","receivedAt":"2010-07-14T16:26:46Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Will Palmer writes:\n\n> However, merge-tree has a function similar to diff (it shows differences\n> between trees) and apply (it creates the \"result\" object when there are\n> no conflicts), and its purpose seems to be more of a \"shower\" than a\n> \"do-er\" (contrasted with something like git-merge, which actually\n> creates a commit object, for example), so I thought that diff-commands\n> would be a good classification.\n\nOk, that makes sense (I haven't heard anybody use merge-tree in real life,\nthough). Let's keep the numbering as-is.\n"}]}