{"thread":{"id":"50529","subject":"[Fix v1] builtin/ls-files.c: add error check on lstat for modified files","startedAt":"2019-02-17T16:35:20Z","lastAt":"2019-04-01T09:29:23Z","messageCount":4,"participants":["randall.s.becker@rogers.com","Ramsay Jones","Randall S. Becker","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"369514","messageId":"20190217163456.17560-1-randall.s.becker@rogers.com","threadId":"50529","inReplyTo":null,"subject":"[Fix v1] builtin/ls-files.c: add error check on lstat for modified files","fromName":"","fromEmail":"randall.s.becker@rogers.com","sentAt":"2019-02-17T16:34:56Z","receivedAt":"2019-02-17T16:35:20Z","isPatch":false,"sender":{"key":"randall.s.becker@rogers.com","avatar":null},"body":"From: \"Randall S. Becker\" \n\nThe result from lstat, checking whether a file has been deleted, is now\nincluded priot to calling id_modified when showing modified files. Prior\nto this fix, it is possible that files that were deleted could show up\nas being modified because the lstat error was unchecked.\n\nReported-by: Joe Ranieri \nSigned-off-by: Randall S. Becker \n---\n builtin/ls-files.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex 29a8762d4..fc21f4795 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -348,7 +348,7 @@ static void show_files(struct repository *repo, struct dir_struct *dir)\n \t\t\terr = lstat(fullname.buf, &st);\n \t\t\tif (show_deleted && err)\n \t\t\t\tshow_ce(repo, dir, ce, fullname.buf, tag_removed);\n-\t\t\tif (show_modified && ie_modified(repo->index, ce, &st, 0))\n+\t\t\tif (show_modified && !err && ie_modified(repo->index, ce, &st, 0))\n \t\t\t\tshow_ce(repo, dir, ce, fullname.buf, tag_modified);\n \t\t}\n \t}\n-- \n2.12.3\n\n"},{"id":"369515","messageId":"e762e80a-d1ef-f906-e128-03f35de3c3ba@ramsayjones.plus.com","threadId":"50529","inReplyTo":"20190217163456.17560-1-randall.s.becker@rogers.com","subject":"Re: [Fix v1] builtin/ls-files.c: add error check on lstat for modified files","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2019-02-17T17:04:49Z","receivedAt":"2019-02-17T17:04:54Z","isPatch":false,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 17/02/2019 16:34, randall.s.becker@rogers.com wrote:\n> From: \"Randall S. Becker\" \n> \n> The result from lstat, checking whether a file has been deleted, is now\n> included priot to calling id_modified when showing modified files. Prior\n\ns/priot/prior/; s/id_modified/ie_modified/\n\n> to this fix, it is possible that files that were deleted could show up\n> as being modified because the lstat error was unchecked.\n> \n> Reported-by: Joe Ranieri \n> Signed-off-by: Randall S. Becker \n> ---\n> builtin/ls-files.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/builtin/ls-files.c b/builtin/ls-files.c\n> index 29a8762d4..fc21f4795 100644\n> --- a/builtin/ls-files.c\n> +++ b/builtin/ls-files.c\n> @@ -348,7 +348,7 @@ static void show_files(struct repository *repo, struct dir_struct *dir)\n> \t\t\terr = lstat(fullname.buf, &st);\n> \t\t\tif (show_deleted && err)\n\nTo be pedantic, this should probably check for (err == ENOENT), since\nlstat() can fail for several reasons which don't imply that the path\nhas been deleted. However, that is unlikely.\n\nNo reason to include such a check in this patch, of course.\n\nATB,\nRamsay Jones\n\n> \t\t\t\tshow_ce(repo, dir, ce, fullname.buf, tag_removed);\n> -\t\t\tif (show_modified && ie_modified(repo->index, ce, &st, 0))\n> +\t\t\tif (show_modified && !err && ie_modified(repo->index, ce, &st, 0))\n> \t\t\t\tshow_ce(repo, dir, ce, fullname.buf, tag_modified);\n> \t\t}\n> \t}\n> \n"},{"id":"369516","messageId":"000001d4c6e6$52475010$f6d5f030$@nexbridge.com","threadId":"50529","inReplyTo":"e762e80a-d1ef-f906-e128-03f35de3c3ba@ramsayjones.plus.com","subject":"RE: [Fix v1] builtin/ls-files.c: add error check on lstat for modified files","fromName":"Randall S. Becker","fromEmail":"rsbecker@nexbridge.com","sentAt":"2019-02-17T17:29:16Z","receivedAt":"2019-02-17T17:29:33Z","isPatch":false,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On February 17, 2019 12:05, Ramsay Jones wrote:\n> On 17/02/2019 16:34, randall.s.becker@rogers.com wrote:\n> > From: \"Randall S. Becker\" \n> >\n> > The result from lstat, checking whether a file has been deleted, is\n> > now included priot to calling id_modified when showing modified files.\n> > Prior\n> \n> s/priot/prior/; s/id_modified/ie_modified/\n> \n> > to this fix, it is possible that files that were deleted could show up\n> > as being modified because the lstat error was unchecked.\n> >\n> > Reported-by: Joe Ranieri \n> > Signed-off-by: Randall S. Becker \n> > ---\n> > builtin/ls-files.c | 2 +-\n> > 1 file changed, 1 insertion(+), 1 deletion(-)\n> >\n> > diff --git a/builtin/ls-files.c b/builtin/ls-files.c index\n> > 29a8762d4..fc21f4795 100644\n> > --- a/builtin/ls-files.c\n> > +++ b/builtin/ls-files.c\n> > @@ -348,7 +348,7 @@ static void show_files(struct repository *repo,\n> struct dir_struct *dir)\n> > \t\t\terr = lstat(fullname.buf, &st);\n> > \t\t\tif (show_deleted && err)\n> \n> To be pedantic, this should probably check for (err == ENOENT), since\n> lstat() can fail for several reasons which don't imply that the path has been\n> deleted. However, that is unlikely.\n\nThat would be very platform specific error checking. lstat can fail for a variety of other reasons also leaving the file deleted (like a symlink issue), but you are correct. I was following the prior line's model of checking for consistency.\n\n> No reason to include such a check in this patch, of course.\n> \n> ATB,\n> Ramsay Jones\n> \n> > \t\t\t\tshow_ce(repo, dir, ce, fullname.buf,\n> tag_removed);\n> > -\t\t\tif (show_modified && ie_modified(repo->index, ce,\n> &st, 0))\n> > +\t\t\tif (show_modified && !err && ie_modified(repo-\n> >index, ce, &st, 0))\n> > \t\t\t\tshow_ce(repo, dir, ce, fullname.buf,\n> tag_modified);\n> > \t\t}\n> > \t}\n> >\n\nThis was just to address what Joe reported earlier. It seemed like an easy one to address.\n\nRegards,\nRandall\n\n"},{"id":"372864","messageId":"xmqq4l7ixeds.fsf@gitster-ct.c.googlers.com","threadId":"50529","inReplyTo":"20190217163456.17560-1-randall.s.becker@rogers.com","subject":"Re: [Fix v1] builtin/ls-files.c: add error check on lstat for modified files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-01T09:29:19Z","receivedAt":"2019-04-01T09:29:23Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"randall.s.becker@rogers.com writes:\n\n> From: \"Randall S. Becker\" \n>\n> The result from lstat, checking whether a file has been deleted, is now\n> included priot to calling id_modified when showing modified files. Prior\n\ns/priot/prior/\n\n> to this fix, it is possible that files that were deleted could show up\n> as being modified because the lstat error was unchecked.\n>\n> Reported-by: Joe Ranieri \n> Signed-off-by: Randall S. Becker \n> ---\n> builtin/ls-files.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n\nThe justification for the change reads quite convincing. Is it\nmerely \"it is _possible_ ... _could_ show up\", though? The code is\niterating over the in-core index, so if you add a blob at path F in\nthe index then remove that regular file F from the working tree,\nwhen it is the cache entry for \"F\"'s turn to get inspected, lstat()\nwould say ENOENT, (show_deleted && err) would show tag_removed, and\nie_modified() gets a garbage &st and ie_match_stat() would say\n\"modified\", no? \n\n> diff --git a/builtin/ls-files.c b/builtin/ls-files.c\n> index 29a8762d4..fc21f4795 100644\n> --- a/builtin/ls-files.c\n> +++ b/builtin/ls-files.c\n> @@ -348,7 +348,7 @@ static void show_files(struct repository *repo, struct dir_struct *dir)\n> \t\t\terr = lstat(fullname.buf, &st);\n> \t\t\tif (show_deleted && err)\n> \t\t\t\tshow_ce(repo, dir, ce, fullname.buf, tag_removed);\n> -\t\t\tif (show_modified && ie_modified(repo->index, ce, &st, 0))\n> +\t\t\tif (show_modified && !err && ie_modified(repo->index, ce, &st, 0))\n> \t\t\t\tshow_ce(repo, dir, ce, fullname.buf, tag_modified);\n> \t\t}\n> \t}\n\nAnd the implementation of the change looks OK.\n\nI wonder if there is an easy way to cover this with a test or two.\nWouldn't it be just the matter of doing something like this\n\n\ttest_expect_success 'allow telling modified and deleted ones apart' '\n\t\t>testfile &&\n\t\tgit add testfile &&\n\t\trm testfile &&\n\t\techo C testfile >expect &&\n\t\tgit ls-files -m -d -t testfile >actual &&\n\t\ttest_cmp expect actual\n\t'\n\nin some existing test file for ls-files, perhaps in t3004 (ls-files-basic)?\n\nI went back to the original discussion of the \"BUG\" around mid Feb\n2019, and didn't find anybody worried about backward compatibility.\nAs \"ls-files -[dm...t]\" is marked semi-deprecated, perhaps breaking\nthe current users does not matter that much ;-)\n"}]}