{"thread":{"id":"17745","subject":"[PATCH] Bugfix: GIT_EXTERNAL_DIFF with more than one changed files","startedAt":"2009-02-12T13:36:14Z","lastAt":"2009-02-13T18:07:40Z","messageCount":4,"participants":["Nazri Ramliy","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"104385","messageId":"20090212133614.GA12746@bigbear","threadId":"17745","inReplyTo":null,"subject":"[PATCH] Bugfix: GIT_EXTERNAL_DIFF with more than one changed files","fromName":"Nazri Ramliy","fromEmail":"ayiehere@gmail.com","sentAt":"2009-02-12T13:36:14Z","receivedAt":"2009-02-12T13:36:14Z","isPatch":true,"sender":{"key":"ayiehere@gmail.com","avatar":"https://avatars.githubusercontent.com/u/164756?v=4"},"body":"Regression introduced in 479b0ae81c9291a8bb8d7b2347cc58eeaa701304.\n\nWhen there is more than one file that are changed, running\ngit diff with GIT_EXTERNAL_DIFF works only for the first file.\n\nThis patch fixes this problem and added a test case for it.\n\nSigned-off-by: Nazri Ramliy \n---\n diff.c | 8 ++++----\n t/t4020-diff-external.sh | 8 ++++++++\n 2 files changed, 12 insertions(+), 4 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex a5a540f..be3859e 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -184,11 +184,11 @@ static int remove_tempfile_installed;\n static void remove_tempfile(void)\n {\n \tint i;\n-\tfor (i = 0; i < ARRAY_SIZE(diff_temp); i++)\n-\t\tif (diff_temp[i].name == diff_temp[i].tmp_path) {\n+\tfor (i = 0; i < ARRAY_SIZE(diff_temp); i++) {\n+\t\tif (diff_temp[i].name == diff_temp[i].tmp_path)\n \t\t\tunlink(diff_temp[i].name);\n-\t\t\tdiff_temp[i].name = NULL;\n-\t\t}\n+\t\tdiff_temp[i].name = NULL;\n+\t}\n }\n \n static void remove_tempfile_on_signal(int signo)\ndiff --git a/t/t4020-diff-external.sh b/t/t4020-diff-external.sh\nindex caea292..281680d 100755\n--- a/t/t4020-diff-external.sh\n+++ b/t/t4020-diff-external.sh\n@@ -128,4 +128,12 @@ test_expect_success 'force diff with \"diff\"' '\n \ttest_cmp \"$TEST_DIRECTORY\"/t4020/diff.NUL actual\n '\n \n+test_expect_success 'GIT_EXTERNAL_DIFF with more than one changed files' '\n+\techo anotherfile > file2 &&\n+\tgit add file2 &&\n+\tgit commit -m \"added 2nd file\" &&\n+\techo modified >file2 &&\n+\tGIT_EXTERNAL_DIFF=echo git diff\n+'\n+\n test_done\n-- \n1.6.2.rc0.55.g30aa4f\n"},{"id":"104386","messageId":"20090212140740.GB3057@coredump.intra.peff.net","threadId":"17745","inReplyTo":"20090212133614.GA12746@bigbear","subject":"Re: [PATCH] Bugfix: GIT_EXTERNAL_DIFF with more than one changed files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-02-12T14:07:40Z","receivedAt":"2009-02-12T14:07:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 12, 2009 at 09:36:14PM +0800, Nazri Ramliy wrote:\n\n> Regression introduced in 479b0ae81c9291a8bb8d7b2347cc58eeaa701304.\n> \n> When there is more than one file that are changed, running\n> git diff with GIT_EXTERNAL_DIFF works only for the first file.\n> \n> This patch fixes this problem and added a test case for it.\n\nYikes. Thanks for finding this.\n\nActually, the situation is a little more complex than what you describe.\n\nWe used to just re-use the diff_temp array unconditionally; the commit\nyou mention introduced a safety check to make sure we are not\noverwriting an existing tempfile that should be cleaned up. That safety\ncheck looks for the \"name\" field being NULL to signal an unused slot.\n\nBut the \"remove_tempfile\" function uses a different test to see if a\nslot needs to be deleted: if the name and the tmp_path are the same.\nAnd they would not be if we were able to reuse a working tree file as\npart of the diff, in which case there would be nothing to clean up. And\nif we did clean something up, we set the name field to NULL.\n\nSo this bug should trigger only in the face of reusing worktree files. I\nchecked your test; it constructs a diff between the worktree and the\nindex, so it correctly finds the problem.\n\n> {\n> \tint i;\n> -\tfor (i = 0; i < ARRAY_SIZE(diff_temp); i++)\n> -\t\tif (diff_temp[i].name == diff_temp[i].tmp_path) {\n> +\tfor (i = 0; i < ARRAY_SIZE(diff_temp); i++) {\n> +\t\tif (diff_temp[i].name == diff_temp[i].tmp_path)\n> \t\t\tunlink(diff_temp[i].name);\n> -\t\t\tdiff_temp[i].name = NULL;\n> -\t\t}\n> +\t\tdiff_temp[i].name = NULL;\n> +\t}\n\nNote that the other possible fix for this bug is to change\nclaim_diff_tempfile to use the same test. But I prefer this, as it is\nmore idiomatic to use NULL as a marker for \"not in use\".\n\nAcked-by: Jeff King \n\n-Peff\n"},{"id":"104414","messageId":"7vskmjl729.fsf@gitster.siamese.dyndns.org","threadId":"17745","inReplyTo":"20090212140740.GB3057@coredump.intra.peff.net","subject":"Re: [PATCH] Bugfix: GIT_EXTERNAL_DIFF with more than one changed files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-02-12T20:43:42Z","receivedAt":"2009-02-12T20:43:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> So this bug should trigger only in the face of reusing worktree files. I\n> checked your test; it constructs a diff between the worktree and the\n> index, so it correctly finds the problem.\n>\n> Acked-by: Jeff King \n\nThanks, both.\n\nJeff, according to your analysis, this shouldn't trigger when\ncore.autocrlf is set, should it?\n"},{"id":"104536","messageId":"20090213180740.GA31860@coredump.intra.peff.net","threadId":"17745","inReplyTo":"7vskmjl729.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Bugfix: GIT_EXTERNAL_DIFF with more than one changed files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-02-13T18:07:40Z","receivedAt":"2009-02-13T18:07:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 12, 2009 at 12:43:42PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > So this bug should trigger only in the face of reusing worktree files. I\n> > checked your test; it constructs a diff between the worktree and the\n> > index, so it correctly finds the problem.\n> >\n> > Acked-by: Jeff King \n> \n> Thanks, both.\n> \n> Jeff, according to your analysis, this shouldn't trigger when\n> core.autocrlf is set, should it?\n\nDepending on the diff you are doing. If one of the sides is the\nworktree, then we should always be using the worktree file. But for\n\"diff --cached\" it depends on the file contents matching the index (in\ntheory, it would work for arbitrary tree diffs when one side matches the\nworktree, but see the comment in reuse_worktree_file -- if nobody has\nlooked at the cache already, we don't load it just for this).\n\nI tried to construct a simple test that shows this behavior, but I\ncouldn't. I did:\n\n mkdir repo && cd repo && git init\n\n git config core.autocrlf true\n\n printf 'one\\r\\n' >file1\n printf 'one\\r\\n' >file2\n git add .\n git commit -m one\n\n printf 'two\\r\\n' >file1\n printf 'two\\r\\n' >file2\n git add -u\n\n PAGER=cat GIT_EXTERNAL_DIFF=echo git diff --cached\n\nwhich should fail without the core.autocrlf setting, but work otherwise.\nBut it doesn't, and the reason is that the content in the index actually\nhas the CRLF:\n\n $ xxd < file1\n 0000000: 7477 6f0d 0a two..\n $ git cat-file blob :file1 | xxd\n 0000000: 7477 6f0d 0a two..\n\nwhich has me confused. Am I using autocrlf wrong? I have been fortunate\nenough in the past never to work on filesystems that needed such a\nthing.\n\n-Peff\n"}]}