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

From: Derrick Stolee <stolee@gmail.com>
Date: Jul 30, 2020, 13:29 UTC
Message-ID: <9aadb889-007a-c973-c59c-a471436368bb@gmail.com>
In-Reply-To: <xmqqblk6q8ws.fsf@gitster.c.googlers.com>

On 7/23/2020 4:21 PM, Junio C Hamano wrote:

Show 13 quoted lines

> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> 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.

Adding "auto-" to the argument can be informative.

I would think that abbreviating the option may make understanding the argument more difficult for users where English is not their first language. Of course, I'm a bad judge of that.

I also don't think this option is called often by users at the command-line and instead is done by scripts and third-party tools. Abbreviating the word may have less value there.

(This reminds me that is would be good to have a config option that prevents running this process _at all_. Sure, gc.auto=0 prevents the command from doing anything, but there is still an extra cost of starting a process. This is more of a problem on Windows. Making a note to self here.)

Show 16 quoted lines

>>  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.

I don't completely eliminate 'git gc' at the end of this series, but instead hope that we can peel it apart slowly in follow-up series. However, you are correct that I should be more careful to obliterate it from the documentation instead of keeping references around.

Show 54 quoted lines

>> 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.

Yes, I should have noticed that. Also, with Peff's feedback from another thread, the method can look a bit simpler this way:

int run_auto_maintenance(int quiet)
{
	struct child_process maint = CHILD_PROCESS_INIT;
	maint.git_cmd = 1;

	argv_array_pushl(&maint.argv, "maintenance", "run", "--auto", NULL);
	if (quiet)
		argv_array_push(&maint.argv, "--quiet");
	else
		argv_array_push(&maint.argv, "--no-quiet");

	return run_command(&maint.argv);
}

(I will update it again to work on Peff's argv_array work, but hopefully it is clear how this is simpler.)

Thanks, -Stolee