Re: [PATCH v2 2/2] checkout: fix regression in checkout -b on intitial checkout

From: Jeff King <peff@peff.net>
Date: Jan 22, 2019, 18:49 UTC
Message-ID: <20190122184959.GF4399@sigill.intra.peff.net>
In-Reply-To: <nycvar.QRO.7.76.6.1901221529210.41@tvgsbejvaqbjf.bet>

On Tue, Jan 22, 2019 at 03:35:21PM +0100, Johannes Schindelin wrote:

Show 5 quoted lines

> I also looked at the implementation of `file_exists()` and found that it
> uses `lstat()`. Peff, you introduced this (using `stat()`) in c91f0d92efb3
> (git-commit.sh: convert run_status to a C builtin, 2006-09-08), could you
> enlighten me why you chose `stat()` over `access()` (the latter seems more
> light-weight to me)?

That's quite a while ago, but I'm pretty sure I was just following existing practice. It would be fine to switch from stat() to access(). But...

> Also, Junio, you changed it to use `lstat()` in
> a50f9fc5feb0 (file_exists(): dangling symlinks do exist, 2007-11-18), do
> you think we can/should use `access()` instead?

I think access() will always dereference. So it would not detect a dangling symlink. Whether that matters or not is going to depend on each caller.

I doubt it would matter much either way in this case. And I don't think this is performance critical (it should be once per checkout, not once per file).

If there are callers that care (and I assume there are due to the existence of a50f9fcfeb0), and if we do care about performance on platforms where stat() is slower, it might be reasonable to have a platform-specific implementation of file_exists().

Likewise, anybody converting from access() should consider whether each site cares about dangling symlinks (though in general, I'd expect most of them to simply not have thought of it, and be happy to start considering that as "exists").

-Peff