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

[Lars Schneider]

> On 06 Oct 2017, at 06:56, Jeff King  wrote:
> 
> On Fri, Oct 06, 2017 at 01:26:48PM +0900, Junio C Hamano wrote:
> 
> ...
>> -- >8 --
>> From: Lars Schneider 
>> Date: Thu, 5 Oct 2017 12:44:07 +0200
>> Subject: [PATCH] entry.c: check if file exists after checkout
>> 
>> 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.
> 
> I don't know if we wanted to capture any of the reasoning behind using
> error() here or not. Frankly, I'm not sure how to argue for it
> succinctly. :) I'm happy with letting it live on in the list archive.
> 
>> diff --git a/entry.c b/entry.c
>> index f879758c73..6d9de3a5aa 100644
>> --- a/entry.c
>> +++ b/entry.c
>> @@ -341,7 +341,9 @@ static int write_entry(struct cache_entry *ce,
>>  if (state->refresh_cache) {
>>  assert(state->istate);
>>  if (!fstat_done)
>> -lstat(ce->name, );
>> +if (lstat(ce->name, ) < 0)
>> +return error_errno("unable stat just-written 
>> file %s",
>> +   ce->name);
> 
> s/unable stat/unable to stat/, I think.
> 
> Other than that, this looks fine to me.
> 
> -Peff

Looks fine to me, too.

Thanks,
Lars

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

[Junio C Hamano]

On Fri, Oct 6, 2017 at 3:05 PM, Jeff King  wrote:
>
>> Because we cannot quite tell between the two cases (one is error--we
>> wrote or we thought we wrote, but we cannot find it, the other is
>> dubious--somebody was racing with us in the filesystem), I think it
>> is reasonable to err on the safer side, even though an error abort
>> while doing "as we know we wrote the thing that match the index, we
>> might as well lstat and mark the cache entry as up-to-date" might be
>> a bit irritating.
>
> OK. I can live with that line of thought.

Still that, or any other, line of thought we follow to declare that it
is a good change should be recorded in the log ;-)

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

[Jeff King]

On Fri, Oct 06, 2017 at 03:03:49PM +0900, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > I don't know if we wanted to capture any of the reasoning behind using
> > error() here or not. Frankly, I'm not sure how to argue for it
> > succinctly. :) I'm happy with letting it live on in the list archive.
> 
> Are you talking about the "philosophical" thing?

Right, whether we ought to just mark the entry as stat-dirty and return
success.

> Because we cannot quite tell between the two cases (one is error--we
> wrote or we thought we wrote, but we cannot find it, the other is
> dubious--somebody was racing with us in the filesystem), I think it
> is reasonable to err on the safer side, even though an error abort
> while doing "as we know we wrote the thing that match the index, we
> might as well lstat and mark the cache entry as up-to-date" might be
> a bit irritating.

OK. I can live with that line of thought.

-Peff

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

[Junio C Hamano]

Jeff King  writes:

> I don't know if we wanted to capture any of the reasoning behind using
> error() here or not. Frankly, I'm not sure how to argue for it
> succinctly. :) I'm happy with letting it live on in the list archive.

Are you talking about the "philosophical" thing?  

Because we cannot quite tell between the two cases (one is error--we
wrote or we thought we wrote, but we cannot find it, the other is
dubious--somebody was racing with us in the filesystem), I think it
is reasonable to err on the safer side, even though an error abort
while doing "as we know we wrote the thing that match the index, we
might as well lstat and mark the cache entry as up-to-date" might be
a bit irritating.


>> diff --git a/entry.c b/entry.c
>> index f879758c73..6d9de3a5aa 100644
>> --- a/entry.c
>> +++ b/entry.c
>> @@ -341,7 +341,9 @@ static int write_entry(struct cache_entry *ce,
>>  if (state->refresh_cache) {
>>  assert(state->istate);
>>  if (!fstat_done)
>> -lstat(ce->name, );
>> +if (lstat(ce->name, ) < 0)
>> +return error_errno("unable stat just-written 
>> file %s",
>> +   ce->name);
>
> s/unable stat/unable to stat/, I think.

Thanks.

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

[Jeff King]

On Fri, Oct 06, 2017 at 01:26:48PM +0900, Junio C Hamano wrote:

> > 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.
> 
> ENOTDIR and to a lesser degree EACCES and ELOOP are also
> uninteresting, as we are talking about somebody else mucking with
> the filesystem.

True. The nice thing about the error() route is that we don't need to
make such judgements. The user can decide what is unexpected.

> -- >8 --
> From: Lars Schneider 
> Date: Thu, 5 Oct 2017 12:44:07 +0200
> Subject: [PATCH] entry.c: check if file exists after checkout
> 
> 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.

I don't know if we wanted to capture any of the reasoning behind using
error() here or not. Frankly, I'm not sure how to argue for it
succinctly. :) I'm happy with letting it live on in the list archive.

> diff --git a/entry.c b/entry.c
> index f879758c73..6d9de3a5aa 100644
> --- a/entry.c
> +++ b/entry.c
> @@ -341,7 +341,9 @@ static int write_entry(struct cache_entry *ce,
>   if (state->refresh_cache) {
>   assert(state->istate);
>   if (!fstat_done)
> - lstat(ce->name, );
> + if (lstat(ce->name, ) < 0)
> + return error_errno("unable stat just-written 
> file %s",
> +ce->name);

s/unable stat/unable to stat/, I think.

Other than that, this looks fine to me.

-Peff

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

[Junio C Hamano]

Jeff King  writes:

>> 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, );
>> +if (lstat(ce->name, ) < 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.

ENOTDIR and to a lesser degree EACCES and ELOOP are also
uninteresting, as we are talking about somebody else mucking with
the filesystem.

To tie the loose end, here is what will be queued and merged to
'next' soonish.

Thanks.

-- >8 --
From: Lars Schneider 
Date: Thu, 5 Oct 2017 12:44:07 +0200
Subject: [PATCH] entry.c: check if file exists after checkout

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.

Reported-by: Jeff King 
Signed-off-by: Lars Schneider 
Signed-off-by: Junio C Hamano 
---
 entry.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/entry.c b/entry.c
index f879758c73..6d9de3a5aa 100644
--- a/entry.c
+++ b/entry.c
@@ -341,7 +341,9 @@ static int write_entry(struct cache_entry *ce,
if (state->refresh_cache) {
assert(state->istate);
if (!fstat_done)
-   lstat(ce->name, );
+   if (lstat(ce->name, ) < 0)
+   return error_errno("unable stat just-written 
file %s",
+  ce->name);
fill_stat_cache_info(ce, );
ce->ce_flags |= CE_UPDATE_IN_BASE;
state->istate->cache_changed |= CE_ENTRY_CHANGED;
-- 
2.15.0-rc0-155-g07e9c1a78d

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

[Jeff King]

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

> From: Lars Schneider 
> 
> 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, );
> + if (lstat(ce->name, ) < 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

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

[lars . schneider]

From: Lars Schneider 

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.

Reported-by: Jeff King 
Signed-off-by: Lars Schneider 
---
 entry.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

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, );
+   if (lstat(ce->name, ) < 0)
+   return error("unable to get status of file %s", 
ce->name);
fill_stat_cache_info(ce, );
ce->ce_flags |= CE_UPDATE_IN_BASE;
state->istate->cache_changed |= CE_ENTRY_CHANGED;
-- 
2.14.2