git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Junio C Hamano <gitster@pobox.com>
Date
Dec 6, 2021, 21:19 UTC
Message-ID
<xmqqo85te8j2.fsf@gitster.g>
In-Reply-To
<patch-3.4-8747afecdcd-20211206T165221Z-avarab@gmail.com>
Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
Show 22 quoted lines
> 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.
Show 19 quoted lines
> 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.

Show 26 quoted lines
>  		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.
Show 18 quoted lines
> @@ -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;
 }
Previous: Ævar Arnfjörð BjarmasonNext: Ævar Arnfjörð Bjarmason
Message 11 of 22 in “usage API: Add and use die_message()”
  1. 0/4 usage API: Add and use die_message()Ævar Arnfjörð Bjarmason, Dec 6, 2021
  2. 2/4 usage.c API users: use die_message() where appropriateÆvar Arnfjörð Bjarmason, Dec 6, 2021
  3. Junio C HamanoDec 6, 2021
  4. 1/4 usage.c: add a die_message() routineÆvar Arnfjörð Bjarmason, Dec 6, 2021
  5. Junio C HamanoDec 6, 2021
  6. Junio C HamanoDec 6, 2021
  7. 4/4 config API: don't use vreportf(), make it static in usage.cÆvar Arnfjörð Bjarmason, Dec 6, 2021
  8. Junio C HamanoDec 6, 2021
  9. Ævar Arnfjörð BjarmasonDec 7, 2021
  10. 3/4 usage.c + gc: add and use a die_message_errno()Ævar Arnfjörð Bjarmason, Dec 6, 2021
  11. Junio C HamanoDec 6, 2021
  12. 0/6 usage API: Add and use die_message()Ævar Arnfjörð Bjarmason, Dec 7, 2021
  13. 1/6 usage.c: add a die_message() routineÆvar Arnfjörð Bjarmason, Dec 7, 2021
  14. 2/6 usage.c API users: use die_message() for "fatal :" + exit 128Ævar Arnfjörð Bjarmason, Dec 7, 2021
  15. 3/6 usage.c API users: use die_message() for error() + exit 128Ævar Arnfjörð Bjarmason, Dec 7, 2021
  16. 4/6 gc: return from cmd_gc(), don't call exit()Ævar Arnfjörð Bjarmason, Dec 7, 2021
  17. 5/6 usage.c + gc: add and use a die_message_errno()Ævar Arnfjörð Bjarmason, Dec 7, 2021
  18. 6/6 config API: use get_error_routine(), not vreportf()Ævar Arnfjörð Bjarmason, Dec 7, 2021
  19. Junio C HamanoDec 7, 2021
  20. Jonathan TanDec 22, 2021
  21. Junio C HamanoDec 22, 2021
  22. Ævar Arnfjörð BjarmasonDec 24, 2021

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.