git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Matheus Tavares Bernardino <matheus.bernardino@usp.br>
Cc: git <git@vger.kernel.org>
Subject: Re: [PATCH] entry: check for fstat() errors after checkout
Date: Thu, 09 Jul 2020 11:09:39 -0700	[thread overview]
Message-ID: <xmqq1rlky2zg.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <CAHd-oW7sz5nGUGZDDguz3fdeRLa+8iWRdC+vyCB2jWvFMh3aXg@mail.gmail.com> (Matheus Tavares Bernardino's message of "Thu, 9 Jul 2020 14:39:18 -0300")

Matheus Tavares Bernardino <matheus.bernardino@usp.br> writes:

> 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.

Thinking about this again, you _could_ argue that your version is
being more defensive.  fstat_is_reliable() might lie and tell us it
is OK to use fstat() when we should do lstat(), and in such a case,
we take a failure from fstat() as a sign to pretend that we didn't
even call it, and tell the caller to do an lstat().  I am actually
OK to go in that direction, but then we probably should save away
errno before making this fstat() call, and restore it after it when
we see an error, if we were to truly pretend that we didn't make a
call.  Otherwise error_errno() call we will make later in the flow
would end up reporting the error from the fstat() we chose to pretend
that we didn't call.

And having said all that, I think fstat_is_reliable() can be trusted
(it says false on Windows and says true on all others).



  reply	other threads:[~2020-07-09 18:09 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
2020-07-09 18:09     ` Junio C Hamano [this message]
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=xmqq1rlky2zg.fsf@gitster.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=matheus.bernardino@usp.br \
    /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).