{"thread":{"id":"24338","subject":"[PATCH] fix read-tree storing wrong tree reference with modified index","startedAt":"2010-07-08T22:46:53Z","lastAt":"2010-07-09T10:04:34Z","messageCount":4,"participants":["Heiko Voigt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"145167","messageId":"20100708224653.GB50404@book.hvoigt.net","threadId":"24338","inReplyTo":null,"subject":"[PATCH] fix read-tree storing wrong tree reference with modified index","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2010-07-08T22:46:53Z","receivedAt":"2010-07-08T22:46:53Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"In 456156d a shortcut to priming the index tree reference was\nintroduced.\n\nWhen read-tree is called with -m one would expect it to merge the\ntree read and the existing one stored in the index. It did that\ncorrectly but the index referenced the wrong tree. This sometimes lead\nto a situation that the diff of\n\n index <-> HEAD (git diff)\n index <-> worktree (git diff --cached)\n\nwas empty but\n\n HEAD <-> worktree (git diff HEAD)\n\ndid show changes because the SHA1 of the worktree files were still in\nthe index but the referenced tree came from HEAD. The test demonstrates\nhow to create such a situation.\n\nSigned-off-by: Heiko Voigt \n---\n\nI stumbled upon this bug when using git gui. For some reason it does not\nalways result in the situation described above. I am not sure whether\nthis is the correct fix or if we expect a different behavior from\nread-tree. So another set of eyes is appreciated.\n\n builtin/read-tree.c | 6 ++++--\n t/t1001-read-tree-m-2way.sh | 16 ++++++++++++++++\n 2 files changed, 20 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/read-tree.c b/builtin/read-tree.c\nindex 8bdcab1..3cb6cdd 100644\n--- a/builtin/read-tree.c\n+++ b/builtin/read-tree.c\n@@ -221,11 +221,13 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)\n \t * what came from the tree.\n \t *\n \t * The same holds true if we are switching between two trees\n-\t * using read-tree -m A B. The index must match B after that.\n+\t * using read-tree A B (without -m). The index must match B\n+\t * after that. With given -m it can be a mix of the old index\n+\t * and the read one.\n \t */\n \tif (nr_trees == 1 && !opts.prefix)\n \t\tprime_cache_tree(&active_cache_tree, trees[0]);\n-\telse if (nr_trees == 2 && opts.merge)\n+\telse if (nr_trees == 2 && !opts.merge)\n \t\tprime_cache_tree(&active_cache_tree, trees[1]);\n \n \tif (write_cache(newfd, active_cache, active_nr) ||\ndiff --git a/t/t1001-read-tree-m-2way.sh b/t/t1001-read-tree-m-2way.sh\nindex 6327d20..a5ad3c1 100755\n--- a/t/t1001-read-tree-m-2way.sh\n+++ b/t/t1001-read-tree-m-2way.sh\n@@ -390,4 +390,20 @@ test_expect_success \\\n git ls-files --stage | tee >treeMcheck.out &&\n test_cmp treeM.out treeMcheck.out'\n \n+test_expect_success '-m references the correct modified tree' '\n+\techo > file-a &&\n+\techo > file-b &&\n+\tgit add file-a file-b &&\n+\tgit commit -a -m \"test for correct modified tree\"\n+\tgit branch initial-mod &&\n+\techo b > file-b &&\n+\tgit commit -a -m \"B\" &&\n+\techo a > file-a &&\n+\tgit add file-a &&\n+\tgit ls-tree $(git write-tree) file-a > expect &&\n+\tgit read-tree -m HEAD initial-mod &&\n+\tgit ls-tree $(git write-tree) file-a > actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n1.7.2.rc2.1.g8c934\n"},{"id":"145172","messageId":"7vvd8po804.fsf@alter.siamese.dyndns.org","threadId":"24338","inReplyTo":"20100708224653.GB50404@book.hvoigt.net","subject":"Re: [PATCH] fix read-tree storing wrong tree reference with modified index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-07-08T23:42:51Z","receivedAt":"2010-07-08T23:42:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Heiko Voigt writes:\n\n> \t * The same holds true if we are switching between two trees\n> -\t * using read-tree -m A B. The index must match B after that.\n> +\t * using read-tree A B (without -m). The index must match B\n> +\t * after that. With given -m it can be a mix of the old index\n> +\t * and the read one.\n\nI think the justification of the original patch is completely bogus. Why\nnot just drop the priming instead? Two-tree read-tree without -m does not\nmake much sense but the result would look like an overlay of two trees,\nand is not likely to match either of the trees.\n"},{"id":"145176","messageId":"7v630po53r.fsf@alter.siamese.dyndns.org","threadId":"24338","inReplyTo":"7vvd8po804.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] fix read-tree storing wrong tree reference with modified index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-07-09T00:45:28Z","receivedAt":"2010-07-09T00:45:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Heiko Voigt writes:\n>\n>> \t * The same holds true if we are switching between two trees\n>> -\t * using read-tree -m A B. The index must match B after that.\n>> +\t * using read-tree A B (without -m). The index must match B\n>> +\t * after that. With given -m it can be a mix of the old index\n>> +\t * and the read one.\n>\n> I think the justification of the original patch is completely bogus. Why\n> not just drop the priming instead? Two-tree read-tree without -m does not\n> make much sense but the result would look like an overlay of two trees,\n> and is not likely to match either of the trees.\n\nIOW, how about doing this (backported to 1.6.4 codebase) instead?\n\n-- >8 --\nSubject: [PATCH] Fix \"read-tree -m A B\" priming the cache-tree\n\nIn 456156d a shortcut to priming the index tree reference was\nintroduced, but the justification for it was completely bogus.\n\n\"read-tree -m A B\" is to take the index (and the working tree)\nthat is largely based on (but does not have to match exactly) A\nand update it to B, while carrying the local change that does\nnot overlap the difference between A and B, so there is no reason\nto expect that the resulting index should match the tree B.\n\nNoticed and test provided by Heiko Voigt.\n\nSigned-off-by: Junio C Hamano \n---\n builtin-read-tree.c | 5 -----\n t/t1001-read-tree-m-2way.sh | 16 ++++++++++++++++\n 2 files changed, 16 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin-read-tree.c b/builtin-read-tree.c\nindex 82e25ea..4fbf5f8 100644\n--- a/builtin-read-tree.c\n+++ b/builtin-read-tree.c\n@@ -199,14 +199,9 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)\n \t * \"-m ent\" or \"--reset ent\" form), we can obtain a fully\n \t * valid cache-tree because the index must match exactly\n \t * what came from the tree.\n-\t *\n-\t * The same holds true if we are switching between two trees\n-\t * using read-tree -m A B. The index must match B after that.\n \t */\n \tif (nr_trees == 1 && !opts.prefix)\n \t\tprime_cache_tree(&active_cache_tree, trees[0]);\n-\telse if (nr_trees == 2 && opts.merge)\n-\t\tprime_cache_tree(&active_cache_tree, trees[1]);\n \n \tif (write_cache(newfd, active_cache, active_nr) ||\n \t commit_locked_index(&lock_file))\ndiff --git a/t/t1001-read-tree-m-2way.sh b/t/t1001-read-tree-m-2way.sh\nindex 271bc4e..6e3b601 100755\n--- a/t/t1001-read-tree-m-2way.sh\n+++ b/t/t1001-read-tree-m-2way.sh\n@@ -392,4 +392,20 @@ test_expect_success \\\n git ls-files --stage | tee >treeMcheck.out &&\n test_cmp treeM.out treeMcheck.out'\n \n+test_expect_success '-m references the correct modified tree' '\n+\techo >file-a &&\n+\techo >file-b &&\n+\tgit add file-a file-b &&\n+\tgit commit -a -m \"test for correct modified tree\"\n+\tgit branch initial-mod &&\n+\techo b >file-b &&\n+\tgit commit -a -m \"B\" &&\n+\techo a >file-a &&\n+\tgit add file-a &&\n+\tgit ls-tree $(git write-tree) file-a >expect &&\n+\tgit read-tree -m HEAD initial-mod &&\n+\tgit ls-tree $(git write-tree) file-a >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n1.7.2.rc2.191.gd2de1\n"},{"id":"145201","messageId":"20100709100433.GA7829@book.hvoigt.net","threadId":"24338","inReplyTo":"7v630po53r.fsf@alter.siamese.dyndns.org","subject":"Re: Re: [PATCH] fix read-tree storing wrong tree reference with modified index","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2010-07-09T10:04:34Z","receivedAt":"2010-07-09T10:04:34Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"On Thu, Jul 08, 2010 at 05:45:28PM -0700, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n> > Heiko Voigt writes:\n> >\n> >> \t * The same holds true if we are switching between two trees\n> >> -\t * using read-tree -m A B. The index must match B after that.\n> >> +\t * using read-tree A B (without -m). The index must match B\n> >> +\t * after that. With given -m it can be a mix of the old index\n> >> +\t * and the read one.\n> >\n> > I think the justification of the original patch is completely bogus. Why\n> > not just drop the priming instead? Two-tree read-tree without -m does not\n> > make much sense but the result would look like an overlay of two trees,\n> > and is not likely to match either of the trees.\n\nIf you say so I believe you. I was searching for the reason anyway.\n\n> IOW, how about doing this (backported to 1.6.4 codebase) instead?\n\nLooks good to me (from what I understand).\n\nFor the test code you can add my\n\nSigned-off-by: Heiko Voigt \n\n\n> -- >8 --\n> Subject: [PATCH] Fix \"read-tree -m A B\" priming the cache-tree\n> \n> In 456156d a shortcut to priming the index tree reference was\n> introduced, but the justification for it was completely bogus.\n> \n> \"read-tree -m A B\" is to take the index (and the working tree)\n> that is largely based on (but does not have to match exactly) A\n> and update it to B, while carrying the local change that does\n> not overlap the difference between A and B, so there is no reason\n> to expect that the resulting index should match the tree B.\n> \n> Noticed and test provided by Heiko Voigt.\n> \n> Signed-off-by: Junio C Hamano \n> ---\n> builtin-read-tree.c | 5 -----\n> t/t1001-read-tree-m-2way.sh | 16 ++++++++++++++++\n> 2 files changed, 16 insertions(+), 5 deletions(-)\n> \n> diff --git a/builtin-read-tree.c b/builtin-read-tree.c\n> index 82e25ea..4fbf5f8 100644\n> --- a/builtin-read-tree.c\n> +++ b/builtin-read-tree.c\n> @@ -199,14 +199,9 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)\n> \t * \"-m ent\" or \"--reset ent\" form), we can obtain a fully\n> \t * valid cache-tree because the index must match exactly\n> \t * what came from the tree.\n> -\t *\n> -\t * The same holds true if we are switching between two trees\n> -\t * using read-tree -m A B. The index must match B after that.\n> \t */\n> \tif (nr_trees == 1 && !opts.prefix)\n> \t\tprime_cache_tree(&active_cache_tree, trees[0]);\n> -\telse if (nr_trees == 2 && opts.merge)\n> -\t\tprime_cache_tree(&active_cache_tree, trees[1]);\n> \n> \tif (write_cache(newfd, active_cache, active_nr) ||\n> \t commit_locked_index(&lock_file))\n> diff --git a/t/t1001-read-tree-m-2way.sh b/t/t1001-read-tree-m-2way.sh\n> index 271bc4e..6e3b601 100755\n> --- a/t/t1001-read-tree-m-2way.sh\n> +++ b/t/t1001-read-tree-m-2way.sh\n> @@ -392,4 +392,20 @@ test_expect_success \\\n> git ls-files --stage | tee >treeMcheck.out &&\n> test_cmp treeM.out treeMcheck.out'\n> \n> +test_expect_success '-m references the correct modified tree' '\n> +\techo >file-a &&\n> +\techo >file-b &&\n> +\tgit add file-a file-b &&\n> +\tgit commit -a -m \"test for correct modified tree\"\n> +\tgit branch initial-mod &&\n> +\techo b >file-b &&\n> +\tgit commit -a -m \"B\" &&\n> +\techo a >file-a &&\n> +\tgit add file-a &&\n> +\tgit ls-tree $(git write-tree) file-a >expect &&\n> +\tgit read-tree -m HEAD initial-mod &&\n> +\tgit ls-tree $(git write-tree) file-a >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_done\n> -- \n> 1.7.2.rc2.191.gd2de1\n> \n"}]}