[PATCH] check_and_freshen_file: fix reversed success-check

[Jeff King <peff@peff.net>]

On Wed, Jul 08, 2015 at 02:05:39PM -0400, Jeff King wrote:

> 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