Re: [Fix v1] builtin/ls-files.c: add error check on lstat for modified files

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

randall.s.becker@rogers.com writes:

> From: "Randall S. Becker" <rsbecker@nexbridge.com>
>
> The result from lstat, checking whether a file has been deleted, is now
> included priot to calling id_modified when showing modified files. Prior

s/priot/prior/

> to this fix, it is possible that files that were deleted could show up
> as being modified because the lstat error was unchecked.
>
> Reported-by: Joe Ranieri <jranieri@grammatech.com>
> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
> ---
>  builtin/ls-files.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

The justification for the change reads quite convincing.  Is it
merely "it is _possible_ ... _could_ show up", though?  The code is
iterating over the in-core index, so if you add a blob at path F in
the index then remove that regular file F from the working tree,
when it is the cache entry for "F"'s turn to get inspected, lstat()
would say ENOENT, (show_deleted && err) would show tag_removed, and
ie_modified() gets a garbage &st and ie_match_stat() would say
"modified", no?  

> diff --git a/builtin/ls-files.c b/builtin/ls-files.c
> index 29a8762d4..fc21f4795 100644
> --- a/builtin/ls-files.c
> +++ b/builtin/ls-files.c
> @@ -348,7 +348,7 @@ static void show_files(struct repository *repo, struct dir_struct *dir)
>  			err = lstat(fullname.buf, &st);
>  			if (show_deleted && err)
>  				show_ce(repo, dir, ce, fullname.buf, tag_removed);
> -			if (show_modified && ie_modified(repo->index, ce, &st, 0))
> +			if (show_modified && !err && ie_modified(repo->index, ce, &st, 0))
>  				show_ce(repo, dir, ce, fullname.buf, tag_modified);
>  		}
>  	}

And the implementation of the change looks OK.

I wonder if there is an easy way to cover this with a test or two.
Wouldn't it be just the matter of doing something like this

	test_expect_success 'allow telling modified and deleted ones apart' '
		>testfile &&
		git add testfile &&
		rm testfile &&
		echo C testfile >expect &&
		git ls-files -m -d -t testfile >actual &&
		test_cmp expect actual
	'

in some existing test file for ls-files, perhaps in t3004 (ls-files-basic)?

I went back to the original discussion of the "BUG" around mid Feb
2019, and didn't find anybody worried about backward compatibility.
As "ls-files -[dm...t]" is marked semi-deprecated, perhaps breaking
the current users does not matter that much ;-)