Re: [PATCH v1 2/2] entry.c: check if file exists after checkout
> 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, &st); >> +if (lstat(ce->name, &st) < 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
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
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
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, &st); >> +if (lstat(ce->name, &st) < 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
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, &st); > + if (lstat(ce->name, &st) < 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
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, &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. 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, &st); + if (lstat(ce->name, &st) < 0) + return error_errno("unable stat just-written file %s", + ce->name); fill_stat_cache_info(ce, &st); 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
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, &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
[PATCH v1 2/2] entry.c: check if file exists after checkout
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, &st); + if (lstat(ce->name, &st) < 0) + return error("unable to get status of file %s", ce->name); fill_stat_cache_info(ce, &st); ce->ce_flags |= CE_UPDATE_IN_BASE; state->istate->cache_changed |= CE_ENTRY_CHANGED; -- 2.14.2