git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Matheus Tavares Bernardino <matheus.bernardino@usp.br>
Date
Jul 9, 2020, 17:39 UTC
Message-ID
<CAHd-oW7sz5nGUGZDDguz3fdeRLa+8iWRdC+vyCB2jWvFMh3aXg@mail.gmail.com>
In-Reply-To
<xmqqd054y5t1.fsf@gitster.c.googlers.com>
On Thu, Jul 9, 2020 at 2:08 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 6 quoted lines
>
> 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.
Show 51 quoted lines
> 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.

Previous: Junio C HamanoNext: Junio C Hamano
Message 5 of 9 in “entry: check for fstat() errors after checkout”
  1. entry: check for fstat() errors after checkoutMatheus Tavares, Jul 9, 2020
  2. Derrick StoleeJul 9, 2020
  3. Junio C HamanoJul 9, 2020
  4. Junio C HamanoJul 9, 2020
  5. Matheus Tavares BernardinoJul 9, 2020
  6. Junio C HamanoJul 9, 2020
  7. Matheus Tavares BernardinoJul 21, 2020
  8. Junio C HamanoJul 21, 2020
  9. Derrick StoleeJul 21, 2020

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.