Re: [PATCH v2 05/18] maintenance: add commit-graph task
- From
Derrick Stolee <stolee@gmail.com>
- Date
- Jul 24, 2020, 13:09 UTC
- Message-ID
- <b0bc5d89-52fa-e0f4-e3af-5eb6c76d312e@gmail.com>
- In-Reply-To
- <xmqq4kpyq8wh.fsf@gitster.c.googlers.com>
On 7/23/2020 4:22 PM, Junio C Hamano wrote:
Show 10 quoted lines
> "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.
Show 11 quoted lines
> 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.
Show 30 quoted lines
>> + 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?
Show 14 quoted lines
>> 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