Re: [PATCH 3/4] usage.c + gc: add and use a die_message_errno()

Ævar Arnfjörð Bjarmason  [off-list ref] writes:

Change code the "error: " output when we exit with 128 due to gc.log
errors to use a "fatal: " prefix instead. This adds a sibling function
to the die_errno() added in a preceding commit.

Since it returns 128 instead of -1 (like die_message()) we'll need to
adjust report_last_gc_error().

Let's adjust it while we're at it to not conflate the "should skip"
and "exit with this non-zero code" conditions, as the caller is no
longer hardcoding "128", but relying on die_errno() to return a
nen-zero exit() status.

Since we're touching this code let's also use "return ret" in place of
an "exit(ret)". This is kinder to any cleanup our our "cmd_main()" in
"git.c" might want to do.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 builtin/gc.c      | 21 ++++++++++++---------
 git-compat-util.h |  1 +
 usage.c           | 12 ++++++++++++
 3 files changed, 25 insertions(+), 9 deletions(-)

The structure of the series makes sense here.

1 and 2 were structured to add new set of API functions in 1 and to
make use of them in 2 (which is also OK if done right; as pointed
out, a few changes need to be moved from 2 to 1 to make 1 complete).

This makes further addition and adds its uses at the same time.

Insert a new statement after the blank that separates the
declaration from the statements.

Nobody called die() in here, so we always returned to the caller.
It feels a bit strange that we need, in addition to the existing
'ret' that becomes the return value, an extra out parameter *skip.

Let's see why the caller needs both.

We used to use the fact that 

 - negative return was from error_errno() to cuase exit(128) to
   emulate die.

 - return with 1 is from the "previous gc failed" block and declare
   we can safely return 0 (success).

 - return with 0 is sucess and nothing needed to be, and was, done.

So, I still do not see the point of this change to add an extra
error channel.  I do not find this argument

... as the caller is no
longer hardcoding "128", but relying on die_errno() to return a
nen-zero exit() status.

particularly a convincing one.  We are not obliged to assign the
value returned from die_message_errno() to 'ret' to begin with.
report_last_gc_error() can document what its return values mean
and the caller can react to it.

Indeed, if we assign -1 to 'ret' where we used to call error_errno(),
the caller needs no changes at all, no?

Something like this as a replacement for this part?

 builtin/gc.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git c/builtin/gc.c w/builtin/gc.c
index f7270b26ea..2b54465608 100644
--- c/builtin/gc.c
+++ w/builtin/gc.c

@@ -477,47 +477,49 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
 /*
  * Returns 0 if there was no previous error and gc can proceed, 1 if
  * gc should not proceed due to an error in the last run. Prints a
  * message and returns -1 if an error occurred while reading gc.log
  */
 static int report_last_gc_error(void)
 {
 	struct strbuf sb = STRBUF_INIT;
-	int ret = 0;
+	int ret = -1; /* a pessimist */
 	ssize_t len;
 	struct stat st;
 	char *gc_log_path = git_pathdup("gc.log");
 
 	if (stat(gc_log_path, &st)) {
 		if (errno == ENOENT)
 			goto done;
 
-		ret = error_errno(_("cannot stat '%s'"), gc_log_path);
+		die_message_errno(_("cannot stat '%s'"), gc_log_path);
 		goto done;
 	}
 
-	if (st.st_mtime < gc_log_expire_time)
+	if (st.st_mtime < gc_log_expire_time) {
+		ret = 0;
 		goto done;
+	}
 
 	len = strbuf_read_file(&sb, gc_log_path, 0);
 	if (len < 0)
-		ret = error_errno(_("cannot read '%s'"), gc_log_path);
+		die_message_errno(_("cannot read '%s'"), gc_log_path);
 	else if (len > 0) {
 		/*
 		 * A previous gc failed.  Report the error, and don't
 		 * bother with an automatic gc run since it is likely
 		 * to fail in the same way.
 		 */
 		warning(_("The last gc run reported the following. "
 			       "Please correct the root cause\n"
 			       "and remove %s\n"
 			       "Automatic cleanup will not be performed "
 			       "until the file is removed.\n\n"
 			       "%s"),
 			    gc_log_path, sb.buf);
-		ret = 1;
+		ret = 0;
 	}
 	strbuf_release(&sb);
 done:
 	free(gc_log_path);
 	return ret;
 }