Re: [PATCH] check_and_freshen_file: fix reversed success-check

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 8, 2015, 19:24 UTC
Message-ID: <xmqqegkixxja.fsf@gitster.dls.corp.google.com>
In-Reply-To: <20150708183331.GA16138@peff.net>

Jeff King <peff@peff.net> writes:

Show 13 quoted lines

> 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

s/inverst/invert/?

Show 17 quoted lines

> _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 think each of the functions in the check_and_freshen_* callchain can at least have a comment in front of it, saying what the returned value means, to unconfuse readers. "Return 1 when the thing exists and no further action is necessary; return 0 when the thing does not exist or not in a good state and should be overwritten (if the caller has something to overwrite it with, that is)" or something?

Their returning "1" instead of "-1" could be taken as a hint that says "this non-zero return does not signal a _failure_", but it is a rather weak hint.

Show 21 quoted lines

>
> 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;
>  }