Re: [PATCH] ls-files: do not trust stat info if lstat() fails
From: Junio C Hamano <hidden>
Date: 2016-06-15 23:00:37
Nguyễn Thái Ngọc Duy [off-list ref] writes:
quoted hunk
If 'err' is non-zero, lstat() has failed. Consider the entry modified without passing the (unreliable) stat info to ce_modified() in this case. Noticed-by: Eric Sunshine [off-list ref] Signed-off-by: Nguyễn Thái Ngọc Duy <redacted> --- On Fri, Mar 28, 2014 at 11:04 AM, Eric Sunshine [off-list ref] wrote: > On Wed, Mar 26, 2014 at 9:48 AM, Nguyễn Thái Ngọc Duy [off-list ref] wrote: >> + err = lstat(ce->name, &st); >> + if (show_deleted && err) { >> + show_ce_entry(tag_removed, ce); >> + shown = 1; >> + } >> + if (show_modified && ce_modified(ce, &st, 0)) { > > Is it possible for the lstat() to have failed for some reason when we > get here? If so, relying upon 'st' is unsafe, isn't it? The chance of random stat making ce_modified() return false is pretty low, but you're right. This code is a copy from the old show_files(). I'll fix it in the git-ls series. Meanwhile a patch for maint to fix the original function. builtin/ls-files.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)diff --git a/builtin/ls-files.c b/builtin/ls-files.c index 47c3880..e6bd00e 100644 --- a/builtin/ls-files.c +++ b/builtin/ls-files.c@@ -260,7 +260,7 @@ static void show_files(struct dir_struct *dir) err = lstat(ce->name, &st); if (show_deleted && err) show_ce_entry(tag_removed, ce); - if (show_modified && ce_modified(ce, &st, 0)) + if (show_modified && (err || ce_modified(ce, &st, 0))) show_ce_entry(tag_modified, ce); } }
I am guessing that, even though this was discovered during the development of list-files, is a fix applicable outside the context of that series. I do think the patched result is an improvement than the status quo, but at the same time, I find it insufficient in the context of the whole codepath. What if errno were other than ENOENT and we were told to show_deleted (with or without show_modified)? We would end up saying the path was deleted and modified at the same time, when we do not know either is or is not true at all, because of the failure to lstat() the path. Wouldn't it be saner to add tag_unknown and do something like this instead, I wonder? builtin/ls-files.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 47c3880..af2ce99 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c@@ -46,6 +46,7 @@ static const char *tag_killed = ""; static const char *tag_modified = ""; static const char *tag_skip_worktree = ""; static const char *tag_resolve_undo = ""; +static const char *tag_unknown = ""; static void write_name(const char *name) {
@@ -249,7 +250,7 @@ static void show_files(struct dir_struct *dir) for (i = 0; i < active_nr; i++) { const struct cache_entry *ce = active_cache[i]; struct stat st; - int err; + if ((dir->flags & DIR_SHOW_IGNORED) && !ce_excluded(dir, ce)) continue;
@@ -257,11 +258,15 @@ static void show_files(struct dir_struct *dir) continue; if (ce_skip_worktree(ce)) continue; - err = lstat(ce->name, &st); - if (show_deleted && err) - show_ce_entry(tag_removed, ce); - if (show_modified && ce_modified(ce, &st, 0)) + errno = 0; + if (lstat(ce->name, &st)) { + if (errno != ENOENT && errno != ENOTDIR) + show_ce_entry(tag_unknown, ce); + else if (show_deleted) + show_ce_entry(tag_removed, ce); + } else if (show_modified && ce_modified(ce, &st, 0)) { show_ce_entry(tag_modified, ce); + } } } }
@@ -533,6 +538,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix) tag_killed = "K "; tag_skip_worktree = "S "; tag_resolve_undo = "U "; + tag_unknown = "! "; } if (show_modified || show_others || show_deleted || (dir.flags & DIR_SHOW_IGNORED) || show_killed) require_work_tree = 1;