[PATCH v2] check_and_freshen_file: fix reversed success-check

[Jeff King <peff@peff.net>]

On Wed, Jul 08, 2015 at 12:24:41PM -0700, Junio C Hamano wrote:

> > 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/?

Whoops, should be "inverts".

> 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.

Fair enough. Here's a re-roll with some extra comments, and the typo-fix
from above.

-- >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 inverts 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 was inefficient, but not wrong.

While we're here, let's also drop a comment in front of the
check_and_freshen functions, making a note of their return
type (since it is not our usual "0 for success, -1 for
error").

Signed-off-by: Jeff King <peff@peff.net>
---
 sha1_file.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/sha1_file.c b/sha1_file.c
index 77cd81d..e435289 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -443,6 +443,7 @@ void prepare_alt_odb(void)
 	read_info_alternates(get_object_directory(), 0);
 }
 
+/* Returns 1 if we have successfully freshened the file, 0 otherwise. */
 static int freshen_file(const char *fn)
 {
 	struct utimbuf t;
@@ -450,11 +451,18 @@ static int freshen_file(const char *fn)
 	return !utime(fn, &t);
 }
 
+/*
+ * All of the check_and_freshen functions return 1 if the file exists and was
+ * freshened (if freshening was requested), 0 otherwise. If they return
+ * 0, you should not assume that it is safe to skip a write of the object (it
+ * either does not exist on disk, or has a stale mtime and may be subject to
+ * pruning).
+ */
 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