Subject: Re: [PATCH v2 03/18] maintenance: replace run_auto_gc()

From: Junio C Hamano <[email-masked]>
Date: Thu, 23 Jul 2020 13:21:55 -0700

"Derrick Stolee via GitGitGadget" <[email-masked]> writes:

> +--[no-]maintenance::
>  --[no-]auto-gc::
> -	Run `git gc --auto` at the end to perform garbage collection
> -	if needed. This is enabled by default.
> +	Run `git maintenance run --auto` at the end to perform garbage
> +	collection if needed. This is enabled by default.

Shouldn't the new synonym be called --auto-maintenance or an
abbreviation thereof?  It is not like we will run the full
maintenance suite when "--no-maintenance" is omitted, which
certainly is not the impression we want to give our readers.

>  These objects may be removed by normal Git operations (such as `git commit`)
> -which automatically call `git gc --auto`. (See linkgit:git-gc[1].)
> -If these objects are removed and were referenced by the cloned repository,
> -then the cloned repository will become corrupt.
> +which automatically call `git maintenance run --auto` and `git gc --auto`.

Hmph.  Perhaps the picture may change in the end of the series but I
got an impression that "gc --auto" would eventually become just part
of "maintenance --auto" and the users won't have to be even aware of
its existence?  Wouldn't we rather want to say something like

	--[no-]auto-maintenance::
	--[no-]auto-gc::
                Run `git maintenance run --auto` at the end to perform
                garbage collection if needed (`--[no-]auto-gc` is a
                synonym).  This is enabled by default.
	
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 82ac4be8a5..49a4d727d4 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -196,8 +196,10 @@ static struct option builtin_fetch_options[] = {
>  	OPT_STRING_LIST(0, "negotiation-tip", &negotiation_tip, N_("revision"),
>  			N_("report that we have only objects reachable from this object")),
>  	OPT_PARSE_LIST_OBJECTS_FILTER(&filter_options),

> +	OPT_BOOL(0, "maintenance", &enable_auto_gc,
> +		 N_("run 'maintenance --auto' after fetching")),
>  	OPT_BOOL(0, "auto-gc", &enable_auto_gc,
> +		 N_("run 'maintenance --auto' after fetching")),

OK, so this is truly a backward-compatible synonym at this point.

> diff --git a/run-command.c b/run-command.c
> index 9b3a57d1e3..82ad241638 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -1865,14 +1865,17 @@ int run_processes_parallel_tr2(int n, get_next_task_fn get_next_task,
>  	return result;
>  }
>  
> -int run_auto_gc(int quiet)
> +int run_auto_maintenance(int quiet)
>  {
>  	struct argv_array argv_gc_auto = ARGV_ARRAY_INIT;
>  	int status;
>  
> -	argv_array_pushl(&argv_gc_auto, "gc", "--auto", NULL);
> +	argv_array_pushl(&argv_gc_auto, "maintenance", "run", "--auto", NULL);
>  	if (quiet)
>  		argv_array_push(&argv_gc_auto, "--quiet");
> +	else
> +		argv_array_push(&argv_gc_auto, "--no-quiet");
> +
>  	status = run_command_v_opt(argv_gc_auto.argv, RUN_GIT_CMD);
>  	argv_array_clear(&argv_gc_auto);
>  	return status;

Don't we want to replace all _gc_ with _maintenance_ in this
function?  I think the first business before we can do so would be
to rethink if spelling out "maintenance" fully in code is a good
idea in the first space.  It would make names for variables,
structures and fields unnecessarily long without contributing to
ease of understanding an iota, and a easy-to-remember short-form or
an abbreviation may be needed.  Using a short-form/abbreviation
wouldn't worsen the end-user experience, and not the developer
experience for that matter.

If we choose "gc" as the short-hand, most of the change in this step
would become unnecessary.  I also do not mind if we some other words
or word-fragment (perhaps "maint"???) is chosen.

> diff --git a/run-command.h b/run-command.h
> index 191dfcdafe..d9a800e700 100644
> --- a/run-command.h
> +++ b/run-command.h
> @@ -221,7 +221,7 @@ int run_hook_ve(const char *const *env, const char *name, va_list args);
>  /*
>   * Trigger an auto-gc
>   */
> -int run_auto_gc(int quiet);
> +int run_auto_maintenance(int quiet);
>  
>  #define RUN_COMMAND_NO_STDIN 1
>  #define RUN_GIT_CMD	     2	/*If this is to be git sub-command */
> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
> index a66dbe0bde..9850ecde5d 100755
> --- a/t/t5510-fetch.sh
> +++ b/t/t5510-fetch.sh
> @@ -919,7 +919,7 @@ test_expect_success 'fetching with auto-gc does not lock up' '
>  		git config fetch.unpackLimit 1 &&
>  		git config gc.autoPackLimit 1 &&
>  		git config gc.autoDetach false &&
> -		GIT_ASK_YESNO="$D/askyesno" git fetch >fetch.out 2>&1 &&
> +		GIT_ASK_YESNO="$D/askyesno" git fetch --verbose >fetch.out 2>&1 &&
>  		test_i18ngrep "Auto packing the repository" fetch.out &&
>  		! grep "Should I try again" fetch.out
>  	)