On 7/23/2020 4:22 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@xxxxxxxxx> 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
