Re: [PATCH] entry: check for fstat() errors after checkout

[Derrick Stolee <stolee@gmail.com>]

On 7/8/2020 10:10 PM, Matheus Tavares wrote:
> In 11179eb311 ("entry.c: check if file exists after checkout",
> 2017-10-05) we started checking the result of the lstat() call done
> after writing a file, to avoid writing garbage to the corresponding
> cache entry. However, the code skips calling lstat() if it's possible
> to use fstat() when it still has the file descriptor open. And when
> calling fstat() we don't do the same error checking. To fix that, let
> the callers of fstat_output() know when fstat() fails. In this case,
> write_entry() will try to use lstat() and properly report an error if
> that fails as well.

Looking at this for the first time, I was confused because 11179eb311
doesn't touch these lines. But that's the point: it should have.

Thanks for finding this! I wonder if there is a way to expose this
behavior in a test... it definitely seems like this is only something
that happens if there is a failure in the filesystem, so I'm not sure
such a thing is possible.

It would just be nice to know the ramifications of this change in
behavior, keeping in mind that this behavior started way back in
e4c7292353 (write_entry(): use fstat() instead of lstat() when file is
open, 2009-02-09), over 11 years ago!

Thanks,
-Stolee

> Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
> ---
>  entry.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/entry.c b/entry.c
> index 00b4903366..449bd32dee 100644
> --- a/entry.c
> +++ b/entry.c
> @@ -113,8 +113,7 @@ static int fstat_output(int fd, const struct checkout *state, struct stat *st)
>  	/* use fstat() only when path == ce->name */
>  	if (fstat_is_reliable() &&
>  	    state->refresh_cache && !state->base_dir_len) {
> -		fstat(fd, st);
> -		return 1;
> +		return !fstat(fd, st);
>  	}
>  	return 0;
>  }
>