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

[Junio C Hamano <gitster@pobox.com>]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> 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 <avarab@gmail.com>
> ---
>  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.

> diff --git a/builtin/gc.c b/builtin/gc.c
> index bcef6a4c8da..7b451dfeefc 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -472,19 +472,20 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
>   * 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)
> +static int report_last_gc_error(int *skip)
>  {
>  	struct strbuf sb = STRBUF_INIT;
>  	int ret = 0;
>  	ssize_t len;
>  	struct stat st;
>  	char *gc_log_path = git_pathdup("gc.log");
> +	*skip = 0;
>  
>  	if (stat(gc_log_path, &st)) {

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

>  		if (errno == ENOENT)
>  			goto done;
>  
> -		ret = error_errno(_("cannot stat '%s'"), gc_log_path);
> +		ret = die_message_errno(_("cannot stat '%s'"), gc_log_path);
>  		goto done;
>  	}
>  
> @@ -493,7 +494,7 @@ static int report_last_gc_error(void)
>  
>  	len = strbuf_read_file(&sb, gc_log_path, 0);
>  	if (len < 0)
> -		ret = error_errno(_("cannot read '%s'"), gc_log_path);
> +		ret = die_message_errno(_("cannot read '%s'"), gc_log_path);
>  	else if (len > 0) {
>  		/*
>  		 * A previous gc failed.  Report the error, and don't
> @@ -507,7 +508,7 @@ static int report_last_gc_error(void)
>  			       "until the file is removed.\n\n"
>  			       "%s"),
>  			    gc_log_path, sb.buf);
> -		ret = 1;
> +		*skip = 1;
>  	}
>  	strbuf_release(&sb);
>  done:

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.


> @@ -610,13 +611,15 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
>  			fprintf(stderr, _("See \"git help gc\" for manual housekeeping.\n"));
>  		}
>  		if (detach_auto) {
> -			int ret = report_last_gc_error();
> -			if (ret < 0)
> -				/* an I/O error occurred, already reported */
> -				exit(128);
> -			if (ret == 1)
> +			int skip;
> +			int ret = report_last_gc_error(&skip);
> +
> +			if (skip)
>  				/* Last gc --auto failed. Skip this one. */
>  				return 0;
> +			if (ret)
> +				/* an I/O error occurred, already reported */
> +				return ret;

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