From: Matheus Tavares Bernardino <matheus.bernardino@usp.br>
To: Junio C Hamano <gitster@pobox.com>
Cc: git <git@vger.kernel.org>
Subject: Re: [PATCH] entry: check for fstat() errors after checkout
Date: Thu, 9 Jul 2020 14:39:18 -0300 [thread overview]
Message-ID: <CAHd-oW7sz5nGUGZDDguz3fdeRLa+8iWRdC+vyCB2jWvFMh3aXg@mail.gmail.com> (raw)
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:
>
> 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.
next prev parent reply other threads:[~2020-07-09 17:39 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-09 2:10 [PATCH] entry: check for fstat() errors after checkout Matheus Tavares
2020-07-09 11:41 ` Derrick Stolee
2020-07-09 14:08 ` Junio C Hamano
2020-07-09 17:08 ` Junio C Hamano
2020-07-09 17:39 ` Matheus Tavares Bernardino [this message]
2020-07-09 18:09 ` Junio C Hamano
2020-07-21 15:39 ` Matheus Tavares Bernardino
2020-07-21 20:00 ` Junio C Hamano
2020-07-21 20:57 ` Derrick Stolee
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAHd-oW7sz5nGUGZDDguz3fdeRLa+8iWRdC+vyCB2jWvFMh3aXg@mail.gmail.com \
--to=matheus.bernardino@usp.br \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).