[PATCH] check_and_freshen_file: fix reversed success-check
- From
Jeff King <peff@peff.net>
- Date
- Jul 8, 2015, 18:33 UTC
- Message-ID
- <20150708183331.GA16138@peff.net>
- In-Reply-To
- <20150708180539.GA12353@peff.net>
On Wed, Jul 08, 2015 at 02:05:39PM -0400, Jeff King wrote:
Show 18 quoted lines
> The code path should be unpack-objects.c:write_object, which calls
> sha1_file.cwrite_sha1_file, which then checks has_sha1_file(). These
> days it uses the freshen_* functions instead of the latter, which does a
> similar check. But it does report failure if we cannot call utime() on
> the file, preferring to write it out instead (this is the safer choice
> from a preventing-prune-corruption perspective).
>
> So it's possible that the sequence is:
>
> - unpack-objects tries to write object 1234abcd...
>
> - write_sha1_file calls freshen_loose_object
>
> - we call access("12/34abcd...", F_OK) and see that it does indeed
> exist
>
> - we call utime("12/34abcd...") which fails (presumably due to EPERM);
> we return failure and assume we must write out the object...or maybe in the utime() step there is actually a bug, and we report failure for no good reason. Ugh.
-- >8 -- Subject: check_and_freshen_file: fix reversed success-check
When we want to write out a loose object file, we have always first made sure we don't already have the object somewhere. Since 33d4221 (write_sha1_file: freshen existing objects, 2014-10-15), we also update the timestamp on the file, so that a simultaneous prune knows somebody is likely to reference it soon.
If our utime() call fails, we treat this the same as not having the object in the first place; the safe thing to do is write out another copy. However, the loose-object check accidentally inverst the utime() check; it returns failure _only_ when the utime() call actually succeeded. Thus it was failing to protect us there, and in the normal case where utime() succeeds, it caused us to pointlessly write out and link the object.
This passed our freshening tests, because writing out the new object is certainly _one_ way of updating its utime. So the normal case of a successful utime() was inefficient, but not wrong.
Signed-off-by: Jeff King <peff@peff.net> --- The worst part of this is that I had the _same_ bug in the pack code path when I initially posted what became 33d4221. René noticed during review, and my fix was to invert the return value from freshen_file to match the other functions. But of course doing that without fixing the other caller meant I introduced the same bug there.
I'll be curious if this fixes the problem the OP is seeing. If not, then we can dig deeper into the weird EPERM problems around this particular object database.
sha1_file.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sha1_file.c b/sha1_file.c index 77cd81d..721eadc 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -454,7 +454,7 @@ static int check_and_freshen_file(const char *fn, int freshen) { if (access(fn, F_OK)) return 0; - if (freshen && freshen_file(fn)) + if (freshen && !freshen_file(fn)) return 0; return 1; }
-- 2.5.0.rc1.340.ge59e3eb