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

From: Jeff King <peff@peff.net>
Date: Oct 5, 2017, 11:23 UTC
Message-ID: <20171005112355.lsoqxybgsovpqriy@sigill.intra.peff.net>
In-Reply-To: <20171005104407.65948-3-lars.schneider@autodesk.com>

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

Show 6 quoted lines

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

Show 11 quoted lines

> 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