Re: [PATCH v1 2/2] entry.c: check if file exists after checkout

[Jeff King <peff@peff.net>]

On Thu, Oct 05, 2017 at 12:44:07PM +0200, lars.schneider@autodesk.com wrote:

> From: Lars Schneider <larsxschneider@gmail.com>
> 
> If we are checking out a file and somebody else racily deletes our file,
> then we would write garbage to the cache entry. Fix that by checking
> the result of the lstat() call on that file. Print an error to the user
> if the file does not exist.

My gut tells me this is the right thing to be doing, but this commit
message gives very little analysis. Let's see if we can talk it out a
bit.

Aside from bizarre lstat failures, the plausible reason for seeing this
is that somebody racily deleted the file. I.e.,:

  1. We wrote the file.

  2. They deleted it.

  3. We ran lstat() on it and found that it went away.

But imagine that the race went the other way, and (3) happened before
(2). Then we'd actually get a real index entry, but the file would
appear deleted to anybody who checks the filesystem against the stat
data.

So I guess my question is: is step 3 an integral part of the checkout
procedure, or is it simply an opportunity to refresh the index (since we
know we just wrote out the content)?

If it's an integral part, then I agree that the error return you add
here is the right thing to do. But if it's just an index refresh, then I
wonder if we should report a successful checkout, but mark the entry as
stat-dirty.

I dunno. It's pretty philosophical, and I have a feeling that nobody
really cares all that much in practice. Certainly the error return seems
like the easiest fix.

> diff --git a/entry.c b/entry.c
> index 5dab656364..2252d96756 100644
> --- a/entry.c
> +++ b/entry.c
> @@ -355,7 +355,8 @@ static int write_entry(struct cache_entry *ce,
>  	if (state->refresh_cache) {
>  		assert(state->istate);
>  		if (!fstat_done)
> -			lstat(ce->name, &st);
> +			if (lstat(ce->name, &st) < 0)
> +				return error("unable to get status of file %s", ce->name);

We could probably be a bit more specific about the situation, since the
user will see this message with no context. Maybe something like:

  unable to stat just-written file %s

or something. We should probably also use error_errno(). I'd bet if this
ever triggers that it's likely to be ENOENT, but certainly if it _isn't_
that would be interesting information.

-Peff