{"thread":{"id":"36633","subject":"Strange situation with --assume-unchanged and diff --find-copies-harder","startedAt":"2014-05-11T19:20:57Z","lastAt":"2014-05-15T16:37:50Z","messageCount":3,"participants":["Elliott Cable","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"241268","messageId":"CAPZ477Ot8MiTUNx1AwDTb5sGDDerDvBY=znsK4Fhcb5taYsaHA@mail.gmail.com","threadId":"36633","inReplyTo":null,"subject":"Strange situation with --assume-unchanged and diff --find-copies-harder","fromName":"Elliott Cable","fromEmail":"me@ell.io","sentAt":"2014-05-11T19:20:57Z","receivedAt":"2014-05-11T19:20:57Z","isPatch":false,"sender":{"key":"me@ell.io","avatar":"https://gravatar.com/avatar/0d35bb7d7c29b6bbcaafdf13ec70745573d31cb222130cc754a064e707c08d63?d=mp&s=160"},"body":"So, I've spent some time in the #git channel on Freenode chatting\nabout this, and we couldn't figure it out. I can't reproduce it in a\nnewly-made repository, but it's reproducible with the repository I've\nbeen working in.\n\n    > git status\n    On branch Master\n    Your branch is ahead of 'ec/Master' by 2 commits.\n     (use \"git push\" to publish your local commits)\n\n    nothing to commit, working directory clean\n    > g diff --find-copies-harder\n    diff --git i/Executables/paws.js w/Executables/paws.js\n    old mode 100755\n    new mode 100644\n    > stat -f '%p' Executables/paws.js\n    100755\n    >\n\n - As demonstrated by the `stat`, the mode-change shown by `git diff`\nis a phantom change; it never happened.\n - If I remove the `--find-copies-harder` flag, it doesn't show up.\n - If I choose to --no-assume-unchanged the executable, it doesn't show up.\n - If I change the actual file-mode to the 644 it thinks it is, and\ncommit it, it doesn't show up.\n\nIt's only the precise combination of A) a file flagged +x, B) that\nfile --assume-unchange'd on the index, and C) diff called with the\n--find-copies-harder flag, that shows the phantom mode-change.\n\nI tried reproducing that situation in a clean repository, and the\nproblem didn't seem to surface. You're welcome to clone the repository\nin question, however, and reproduce it yourself:\n\n    > git clone https://github.com/ELLIOTTCABLE/Paws.js.git\n    > git update-index --assume-unchanged Executables/paws.js\n    > git diff --find-copies-harder\n    > stat -f '%p' Executables/paws.js\n\nI'm working on git 1.9.2.\n\n⁓ ELLIOTTCABLE — fly safe.\n  http://ell.io/tt\n"},{"id":"241592","messageId":"20140514221306.GA5020@sigill.intra.peff.net","threadId":"36633","inReplyTo":"CAPZ477Ot8MiTUNx1AwDTb5sGDDerDvBY=znsK4Fhcb5taYsaHA@mail.gmail.com","subject":"[PATCH] run_diff_files: do not look at uninitialized stat data","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-14T22:13:06Z","receivedAt":"2014-05-14T22:13:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, May 11, 2014 at 02:20:57PM -0500, Elliott Cable wrote:\n\n> So, I've spent some time in the #git channel on Freenode chatting\n> about this, and we couldn't figure it out. I can't reproduce it in a\n> newly-made repository, but it's reproducible with the repository I've\n> been working in.\n>\n>     > git status\n>     On branch Master\n>     Your branch is ahead of 'ec/Master' by 2 commits.\n>      (use \"git push\" to publish your local commits)\n> \n>     nothing to commit, working directory clean\n>     > g diff --find-copies-harder\n>     diff --git i/Executables/paws.js w/Executables/paws.js\n>     old mode 100755\n>     new mode 100644\n>     > stat -f '%p' Executables/paws.js\n>     100755\n>     >\n\nThanks for a thorough bug report. I was able to reproduce it. The\nproblem is related to accessing uninitialized memory, so it may vary\nfrom system to system, or even run to run.\n\nThe test I've included below seems to trigger reliably for me. I\nwouldn't be surprised if it does not trigger on other people's systems,\nbut I think it does not hurt to include it (the behavior it tests\nfor certainly _should_ be what happens).\n\n-- >8 --\nSubject: run_diff_files: do not look at uninitialized stat data\n\nIf we try to diff an index entry marked CE_VALID (because it\nwas marked with --assume-unchanged), we do not bother even\nrunning stat() on the file to see if it was removed. This\nstarted long ago with 540e694 (Prevent diff machinery from\nexamining assume-unchanged entries on worktree, 2009-08-11).\n\nHowever, the subsequent code may look at our \"struct stat\"\nand expect to find actual data; currently it will find\nwhatever cruft was left on the stack. This can cause\nproblems in two situations:\n\n  1. We call match_stat_with_submodule with the stat data,\n     so a submodule may be erroneously marked as changed.\n\n  2. If --find-copies-harder is in effect, we pass all\n     entries, even unchanged ones, to diff_change, so it can\n     list them as rename/copy sources. Since we found no\n     change, we assume that function will realize it and not\n     actually display any diff output. However, we end up\n     feeding it a bogus mode, leading it to sometimes claim\n     there was a mode change.\n\nWe can fix both by splitting the CE_VALID and regular code\npaths, and making sure only to look at the stat information\nin the latter. Furthermore, we push the declaration of our\n\"struct stat\" down into the code paths that actually set it,\nso we cannot accidentally access it uninitialized in future\ncode.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThe patch is kind of nasty due to re-indenting. \"diff -b\" makes it much\nclearer.\n\n diff-lib.c                       | 34 ++++++++++++++++++++++------------\n t/t4039-diff-assume-unchanged.sh | 11 +++++++++++\n 2 files changed, 33 insertions(+), 12 deletions(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 0448729..62aee81 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -97,7 +97,6 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\tdiff_unmerged_stage = 2;\n \tentries = active_nr;\n \tfor (i = 0; i < entries; i++) {\n-\t\tstruct stat st;\n \t\tunsigned int oldmode, newmode;\n \t\tstruct cache_entry *ce = active_cache[i];\n \t\tint changed;\n@@ -115,6 +114,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\t\tunsigned int wt_mode = 0;\n \t\t\tint num_compare_stages = 0;\n \t\t\tsize_t path_len;\n+\t\t\tstruct stat st;\n \n \t\t\tpath_len = ce_namelen(ce);\n \n@@ -195,26 +195,36 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\t\tcontinue;\n \n \t\t/* If CE_VALID is set, don't look at workdir for file removal */\n-\t\tchanged = (ce->ce_flags & CE_VALID) ? 0 : check_removed(ce, &st);\n-\t\tif (changed) {\n-\t\t\tif (changed < 0) {\n-\t\t\t\tperror(ce->name);\n+\t\tif (ce->ce_flags & CE_VALID) {\n+\t\t\tchanged = 0;\n+\t\t\tnewmode = ce->ce_mode;\n+\t\t}\n+\t\telse {\n+\t\t\tstruct stat st;\n+\n+\t\t\tchanged = check_removed(ce, &st);\n+\t\t\tif (changed) {\n+\t\t\t\tif (changed < 0) {\n+\t\t\t\t\tperror(ce->name);\n+\t\t\t\t\tcontinue;\n+\t\t\t\t}\n+\t\t\t\tdiff_addremove(&revs->diffopt, '-', ce->ce_mode,\n+\t\t\t\t\t       ce->sha1, !is_null_sha1(ce->sha1),\n+\t\t\t\t\t       ce->name, 0);\n \t\t\t\tcontinue;\n \t\t\t}\n-\t\t\tdiff_addremove(&revs->diffopt, '-', ce->ce_mode,\n-\t\t\t\t       ce->sha1, !is_null_sha1(ce->sha1),\n-\t\t\t\t       ce->name, 0);\n-\t\t\tcontinue;\n+\n+\t\t\tchanged = match_stat_with_submodule(&revs->diffopt, ce, &st,\n+\t\t\t\t\t\t\t    ce_option, &dirty_submodule);\n+\t\t\tnewmode = ce_mode_from_stat(ce, st.st_mode);\n \t\t}\n-\t\tchanged = match_stat_with_submodule(&revs->diffopt, ce, &st,\n-\t\t\t\t\t\t    ce_option, &dirty_submodule);\n+\n \t\tif (!changed && !dirty_submodule) {\n \t\t\tce_mark_uptodate(ce);\n \t\t\tif (!DIFF_OPT_TST(&revs->diffopt, FIND_COPIES_HARDER))\n \t\t\t\tcontinue;\n \t\t}\n \t\toldmode = ce->ce_mode;\n-\t\tnewmode = ce_mode_from_stat(ce, st.st_mode);\n \t\tdiff_change(&revs->diffopt, oldmode, newmode,\n \t\t\t    ce->sha1, (changed ? null_sha1 : ce->sha1),\n \t\t\t    !is_null_sha1(ce->sha1), (changed ? 0 : !is_null_sha1(ce->sha1)),\ndiff --git a/t/t4039-diff-assume-unchanged.sh b/t/t4039-diff-assume-unchanged.sh\nindex 9d9498b..23c0e35 100755\n--- a/t/t4039-diff-assume-unchanged.sh\n+++ b/t/t4039-diff-assume-unchanged.sh\n@@ -28,4 +28,15 @@ test_expect_success 'diff-files does not examine assume-unchanged entries' '\n \ttest -z \"$(git diff-files -- one)\"\n '\n \n+test_expect_success POSIXPERM 'find-copies-harder is not confused by mode bits' '\n+\techo content >exec &&\n+\tchmod +x exec &&\n+\tgit add exec &&\n+\tgit commit -m exec &&\n+\tgit update-index --assume-unchanged exec &&\n+\t>expect &&\n+\tgit diff-files --find-copies-harder -- exec >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.0.0.rc1.436.g03cb729\n"},{"id":"241693","messageId":"xmqqsiobgfq9.fsf@gitster.dls.corp.google.com","threadId":"36633","inReplyTo":"20140514221306.GA5020@sigill.intra.peff.net","subject":"Re: [PATCH] run_diff_files: do not look at uninitialized stat data","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-15T16:37:50Z","receivedAt":"2014-05-15T16:37:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Thanks for a thorough bug report. I was able to reproduce it. The\n> ...\n> -- >8 --\n> Subject: run_diff_files: do not look at uninitialized stat data\n> ...\n> We can fix both by splitting the CE_VALID and regular code\n> paths, and making sure only to look at the stat information\n> in the latter. Furthermore, we push the declaration of our\n> \"struct stat\" down into the code paths that actually set it,\n> so we cannot accidentally access it uninitialized in future\n> code.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n\nThanks for an excellent work, as always.  Will queue.\n"}]}