Re: [PATCH] ls-files: do not trust stat info if lstat() fails

[Junio C Hamano <gitster@pobox.com>]

Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:

> 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 <sunshine@sunshineco.com>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  On Fri, Mar 28, 2014 at 11:04 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>  > On Wed, Mar 26, 2014 at 9:48 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> 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;