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

Re: [PATCH] gc: do not warn about too many loose objects

From
Jonathan Nieder <jrnieder@gmail.com>
Date
Jul 16, 2018, 22:56 UTC
Message-ID
<20180716225639.GK11513@aiede.svl.corp.google.com>
In-Reply-To
<20180716224337.GB12482@sigill.intra.peff.net>
Jeff King wrote:
> On Mon, Jul 16, 2018 at 03:03:06PM -0700, Jonathan Nieder wrote:
Show 7 quoted lines
>> Oh, good point.  In non-daemon mode, we don't let "gc --auto" failure
>> cause the invoking command to fail, but in daemon mode we do.  That
>> should be a straightforward fix; patch coming in a moment.
>
> OK, that definitely sounds like a bug. I'm still confused how that could
> happen, though, since from the caller's perspective they ignore git-gc's
> exit code either way. I guess I'll see in your patch. :)

Alas, I just misremembered. What I was remembering is that gc --auto does

	if (auto_gc && too_many_loose_objects())
		warning(_("There are too many unreachable loose objects; "
			"run 'git prune' to remove them."));
	return 0;

which means that too_many_loose_objects is not an error in undaemonized mode, while it is in daemonized mode. But we've already discussed that.

The calling command in the motivating example is Android's "repo" tool:
            bare_git.gc('--auto')

from https://gerrit-review.googlesource.com/c/git-repo/+/10598/. I think it's reasonable that it expects a status code of 0 in the normal case. So life is less simple than I hoped.

[...]
Show 9 quoted lines
>> Can you point me to some discussion about building that rate-limiting?
>> The commit message for v2.12.2~17^2 (gc: ignore old gc.log files,
>> 2017-02-10) definitely doesn't describe that as its intent.
>
> I think that commit is a loosening of the rate-limiting (because we'd
> refuse to progress for something that was actually time-based). But the
> original stopping comes from this discussion, I think:
>
>   https://public-inbox.org/git/xmqqlhijznpm.fsf@gitster.dls.corp.google.com/

Interesting! It looks like that thread anticipated the problems we've seen here. Three years without having to have fixed it is a good run, I suppose.

The discussion of stopping there appears to be primarily about stopping in the error case, not rate-limiting in the success or warning case.

Here's a patch for the 'return -1' thing.
-- >8 --
Subject: gc: exit with status 128 on failure

A value of -1 returned from cmd_gc gets propagated to exit(), resulting in an exit status of 255. Use die instead for a clearer error message and a controlled exit.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 builtin/gc.c  | 36 ++++++++++++++++--------------------
 t/t6500-gc.sh |  2 +-
 2 files changed, 17 insertions(+), 21 deletions(-)
diff --git a/builtin/gc.c b/builtin/gc.c
index ccfb1ceaeb..2bebc52bda 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -438,10 +438,10 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
 	return NULL;
 }
 
-static int report_last_gc_error(void)
+static void report_last_gc_error(void)
 {
 	struct strbuf sb = STRBUF_INIT;
-	int ret = 0;
+	ssize_t ret;
 	struct stat st;
 	char *gc_log_path = git_pathdup("gc.log");
 
@@ -449,16 +449,17 @@ static int report_last_gc_error(void)
 		if (errno == ENOENT)
 			goto done;
 
-		ret = error_errno(_("Can't stat %s"), gc_log_path);
-		goto done;
+		die_errno(_("cannot stat '%s'"), gc_log_path);
 	}
 
 	if (st.st_mtime < gc_log_expire_time)
 		goto done;
 
 	ret = strbuf_read_file(&sb, gc_log_path, 0);
+	if (ret < 0)
+		die_errno(_("cannot read '%s'"), gc_log_path);
 	if (ret > 0)
-		ret = error(_("The last gc run reported the following. "
+		die(_("The last gc run reported the following. "
 			       "Please correct the root cause\n"
 			       "and remove %s.\n"
 			       "Automatic cleanup will not be performed "
@@ -468,20 +469,18 @@ static int report_last_gc_error(void)
 	strbuf_release(&sb);
 done:
 	free(gc_log_path);
-	return ret;
 }
 
-static int gc_before_repack(void)
+static void gc_before_repack(void)
 {
 	if (pack_refs && run_command_v_opt(pack_refs_cmd.argv, RUN_GIT_CMD))
-		return error(FAILED_RUN, pack_refs_cmd.argv[0]);
+		die(FAILED_RUN, pack_refs_cmd.argv[0]);
 
 	if (prune_reflogs && run_command_v_opt(reflog.argv, RUN_GIT_CMD))
-		return error(FAILED_RUN, reflog.argv[0]);
+		die(FAILED_RUN, reflog.argv[0]);
 
 	pack_refs = 0;
 	prune_reflogs = 0;
-	return 0;
 }
 
 int cmd_gc(int argc, const char **argv, const char *prefix)
@@ -562,13 +561,11 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 			fprintf(stderr, _("See \"git help gc\" for manual housekeeping.\n"));
 		}
 		if (detach_auto) {
-			if (report_last_gc_error())
-				return -1;
+			report_last_gc_error(); /* dies on error */
 
 			if (lock_repo_for_gc(force, &pid))
 				return 0;
-			if (gc_before_repack())
-				return -1;
+			gc_before_repack(); /* dies on failure */
 			delete_tempfile(&pidfile);
 
 			/*
@@ -608,12 +605,11 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 		atexit(process_log_file_at_exit);
 	}
 
-	if (gc_before_repack())
-		return -1;
+	gc_before_repack();
 
 	if (!repository_format_precious_objects) {
 		if (run_command_v_opt(repack.argv, RUN_GIT_CMD))
-			return error(FAILED_RUN, repack.argv[0]);
+			die(FAILED_RUN, repack.argv[0]);
 
 		if (prune_expire) {
 			argv_array_push(&prune, prune_expire);
@@ -623,18 +619,18 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 				argv_array_push(&prune,
 						"--exclude-promisor-objects");
 			if (run_command_v_opt(prune.argv, RUN_GIT_CMD))
-				return error(FAILED_RUN, prune.argv[0]);
+				die(FAILED_RUN, prune.argv[0]);
 		}
 	}
 
 	if (prune_worktrees_expire) {
 		argv_array_push(&prune_worktrees, prune_worktrees_expire);
 		if (run_command_v_opt(prune_worktrees.argv, RUN_GIT_CMD))
-			return error(FAILED_RUN, prune_worktrees.argv[0]);
+			die(FAILED_RUN, prune_worktrees.argv[0]);
 	}
 
 	if (run_command_v_opt(rerere.argv, RUN_GIT_CMD))
-		return error(FAILED_RUN, rerere.argv[0]);
+		die(FAILED_RUN, rerere.argv[0]);
 
 	report_garbage = report_pack_garbage;
 	reprepare_packed_git(the_repository);
diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh
index 818435f04e..c474a94a9f 100755
--- a/t/t6500-gc.sh
+++ b/t/t6500-gc.sh
@@ -117,7 +117,7 @@ test_expect_success 'background auto gc does not run if gc.log is present and re
 	test_config gc.autodetach true &&
 	echo fleem >.git/gc.log &&
 	test_must_fail git gc --auto 2>err &&
-	test_i18ngrep "^error:" err &&
+	test_i18ngrep "^fatal:" err &&
 	test_config gc.logexpiry 5.days &&
 	test-tool chmtime =-345600 .git/gc.log &&
 	test_must_fail git gc --auto &&

-- 
2.18.0.233.g985f88cf7e
Previous: Jeff KingNext: Jeff King
Message 15 of 57 in “gc: do not warn about too many loose objects”
  1. gc: do not warn about too many loose objectsJonathan Tan, Jul 16, 2018
  2. Jeff KingJul 16, 2018
  3. Jonathan NiederJul 16, 2018
  4. Jeff KingJul 16, 2018
  5. Jonathan NiederJul 16, 2018
  6. Jeff KingJul 16, 2018
  7. Jonathan NiederJul 16, 2018
  8. Jeff KingJul 16, 2018
  9. Jonathan NiederJul 16, 2018
  10. Jeff KingJul 16, 2018
  11. Jonathan NiederJul 16, 2018
  12. Jeff KingJul 16, 2018
  13. Jonathan NiederJul 16, 2018
  14. Jeff KingJul 16, 2018
  15. Jonathan NiederJul 16, 2018
  16. Jeff KingJul 16, 2018
  17. Jonathan NiederJul 17, 2018
  18. Ævar Arnfjörð BjarmasonJul 17, 2018
  19. Jonathan NiederJul 17, 2018
  20. Ævar Arnfjörð BjarmasonJul 17, 2018
  21. Jeff KingJul 17, 2018
  22. Ævar Arnfjörð BjarmasonJul 18, 2018
  23. Jeff KingJul 18, 2018
  24. Duy NguyenJul 17, 2018
  25. Junio C HamanoJul 17, 2018
  26. Elijah NewrenJul 16, 2018
  27. Jonathan NiederJul 16, 2018
  28. Elijah NewrenJul 16, 2018
  29. Jeff KingJul 16, 2018
  30. Jonathan NiederJul 16, 2018
  31. Jeff KingJul 16, 2018
  32. Jeff KingJul 16, 2018
  33. Elijah NewrenJul 16, 2018
  34. Jeff KingJul 16, 2018
  35. Elijah NewrenJul 16, 2018
  36. Jeff KingJul 16, 2018
  37. Elijah NewrenJul 16, 2018
  38. Jeff KingJul 16, 2018
  39. Elijah NewrenJul 16, 2018
  40. Jonathan NiederJul 16, 2018
  41. 0/3 gc --auto: do not return error for prior errors in daemonized modeJonathan Nieder, Jul 17, 2018
  42. 1/3 gc: improve handling of errors reading gc.logJonathan Nieder, Jul 17, 2018
  43. Junio C HamanoJul 17, 2018
  44. Jeff KingJul 17, 2018
  45. 2/3 gc: exit with status 128 on failureJonathan Nieder, Jul 17, 2018
  46. Junio C HamanoJul 17, 2018
  47. Jeff KingJul 17, 2018
  48. Jeff KingSep 17, 2018
  49. Jonathan NiederSep 17, 2018
  50. Jeff KingSep 18, 2018
  51. 3/3 gc: do not return error for prior errors in daemonized modeJonathan Nieder, Jul 17, 2018
  52. Jeff KingJul 17, 2018
  53. Junio C HamanoJul 18, 2018
  54. Jeff KingJul 18, 2018
  55. Junio C HamanoJul 18, 2018
  56. Jeff KingJul 18, 2018
  57. Junio C HamanoJul 18, 2018

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.