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

[Matheus Tavares Bernardino <matheus.bernardino@usp.br>]

On Thu, Jul 9, 2020 at 2:08 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> The returned value from fstat_output() is suppsed to be "have we
> done fstat() so that we do not need to do a lstat()?"  Don't you
> instead want to extend it to "0 means we didn't, 1 means we did
> successfully, and -1 means we did and failed"?  At least, the way
> _this_ function is modified by this patch is in line with that.

Makes sense, thanks for spotting this issue.

> Which means that we'd need to update the caller(s) to match, to
> avoid risking this change to be just half a change, very similarly
> to how the change in 11179eb311 was just half a change.
>
> Perhaps like this?
>
>  entry.c | 23 +++++++++++++++--------
>  1 file changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/entry.c b/entry.c
> index 53380bb614..f48507ca42 100644
> --- a/entry.c
> +++ b/entry.c
> @@ -108,14 +108,21 @@ static int open_output_fd(char *path, const struct cache_entry *ce, int to_tempf
>         }
>  }
>
> +/*
> + * We have an open fd to a file that we may use lstat() on later.
> + * When able, try doing a fstat(fd) instead and tell the caller it
> + * does not have to do an extra lstat()
> + *
> + * Return 1 if we successfully ran fstat() and *st is valid.
> + * Return 0 if we did not do fstat() and the caller should do lstat().
> + * Return -1 if we got failure from fstat()---the caller can skip lstat().
> + */
>  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;
> -       }
> +           state->refresh_cache && !state->base_dir_len)
> +               return (fstat(fd, st) < 0) ? -1 : 1;
>         return 0;
>  }
>
> @@ -369,10 +376,10 @@ static int write_entry(struct cache_entry *ce,
>  finish:
>         if (state->refresh_cache) {
>                 assert(state->istate);
> -               if (!fstat_done)
> -                       if (lstat(ce->name, &st) < 0)
> -                               return error_errno("unable to stat just-written file %s",
> -                                                  ce->name);
> +               if (fstat_done < 0 ||
> +                   (!fstat_done && lstat(ce->name, &st) < 0))
> +                       return error_errno("unable to stat just-written file %s",
> +                                          ce->name);

If fstat() failed or we couldn't fstat() but lstat() failed, we return
an error. Nice! Thanks for the correction.