Re: [PATCH v2 05/18] maintenance: add commit-graph task

[Derrick Stolee <stolee@gmail.com>]

On 7/23/2020 4:22 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> +static int maintenance_task_commit_graph(void)
>> +{
>> +	struct repository *r = the_repository;
>> +	char *chain_path;
>> +
>> +	/* Skip commit-graph when --auto is specified. */
>> +	if (opts.auto_flag)
>> +		return 0;

Now that you point this out, this is actually a stray condition
from an earlier version. We now have the ".enabled" config and
the auto condition function pointer. That handles all of that
"should we run this when --auto is specified?" logic outside of
the task itself.
 
> Stepping back a bit, back in "git gc" days, "--auto" had two
> distinct meanings rolled into one.  Check if it even needs to be
> done, and perform only the lightweight variant if needed.
> 
> For this task, there is no "lightweight variant" is possible, so
> returning without checking the need to do a lightweight one makes
> perfect sense here.
> 
> But wouldn't it suggest perhaps we could name "auto" field of the
> options struct in a more meaningful way?  Perhaps "quick" (i.e. only
> the quicker-variant of the maintenance job) or something?

But you are discussing here how the _behavior_ can change when
--auto is specified. And specifically, "git gc --auto" really
meant "This is running after a foreground command, so only do
work if necessary and do it quickly to minimize blocking time."

I'd be happy to replace "--auto" with "--quick" in the
maintenance builtin.

This opens up some extra design space for how the individual
tasks perform depending on "--quick" being specified or not.
My intention was to create tasks that are already in "quick"
mode:

* loose-objects have a maximum batch size.
* incremental-repack is capped in size.
* commit-graph uses the --split option.

But this "quick" distinction might be important for some of
the tasks we intend to extract from the gc builtin.

>> +	close_object_store(r->objects);
>> +	if (run_write_commit_graph()) {
>> +		error(_("failed to write commit-graph"));
>> +		return 1;
>> +	}
>> +
>> +	if (!run_verify_commit_graph())
>> +		return 0;
>> +
>> +	warning(_("commit-graph verify caught error, rewriting"));
>> +
>> +	chain_path = get_commit_graph_chain_filename(r->objects->odb);
>> +	if (unlink(chain_path)) {
>> +		UNLEAK(chain_path);
>> +		die(_("failed to remove commit-graph at %s"), chain_path);
> 
> OK.
> 
>> +	}
>> +	free(chain_path);
>> +
>> +	if (!run_write_commit_graph())
>> +		return 0;
>> +
>> +	error(_("failed to rewrite commit-graph"));
>> +	return 1;
>> +}
> 
> Error convention is "positive for error, zero for success?"  That is
> a bit unusual for our internal API.

Since the tasks are frequently running subcommands, returning
0 for success and non-zero for error matches the error codes
returned by those subcommands.

Should I instead change the behavior and clearly document that
task functions matching maintenance_task_fn follow this error
pattern?

>> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
>> index e4e4036e50..216ac0b19e 100755
>> --- a/t/t7900-maintenance.sh
>> +++ b/t/t7900-maintenance.sh
>> @@ -12,7 +12,7 @@ test_expect_success 'help text' '
>>  	test_i18ngrep "usage: git maintenance run" err
>>  '
>>  
>> -test_expect_success 'gc [--auto|--quiet]' '
>> +test_expect_success 'run [--auto|--quiet]' '
> 
> It does not look like this change belongs here.  If "run" is
> appropriate title for this test at this step, it must have been so
> in the previous step.

Thanks. Will fix.

-Stolee