On Thu, Jul 9, 2020 at 2:08 PM Junio C Hamano <gitster@xxxxxxxxx> 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.
