Jonathan Nieder <jrnie...@gmail.com> writes:
> 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 <jrnie...@gmail.com>
> ---
> As in
> https://public-inbox.org/git/20180716225639.gk11...@aiede.svl.corp.google.com/.
> The only change is splitting out patch 1/3.
>
> builtin/gc.c | 35 ++++++++++++++---------------------
> t/t6500-gc.sh | 2 +-
> 2 files changed, 15 insertions(+), 22 deletions(-)
I double checked "return' in cmd_gc() and this patch catches them
all. Good clean-up.
Thanks.
> diff --git a/builtin/gc.c b/builtin/gc.c
> index d69fc4c0b0..95c8afd07b 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -438,10 +438,9 @@ 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 len;
> struct stat st;
> char *gc_log_path = git_pathdup("gc.log");
> @@ -450,8 +449,7 @@ static int report_last_gc_error(void)
> if (errno == ENOENT)
> goto done;
>
> - ret = error_errno(_("cannot stat '%s'"), gc_log_path);
> - goto done;
> + die_errno(_("cannot stat '%s'"), gc_log_path);
> }
>
> if (st.st_mtime < gc_log_expire_time)
> @@ -459,9 +457,9 @@ 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);
> + die_errno(_("cannot read '%s'"), gc_log_path);
> else if (len > 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 "
> @@ -471,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)
> @@ -565,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);
>
> /*
> @@ -611,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);
> @@ -626,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 &&
