"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;
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?
> + 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.
> static int maintenance_task_gc(void)
> {
> int result;
> @@ -768,6 +836,10 @@ static void initialize_tasks(void)
> tasks[num_tasks]->fn = maintenance_task_gc;
> tasks[num_tasks]->enabled = 1;
> num_tasks++;
> +
> + tasks[num_tasks]->name = "commit-graph";
> + tasks[num_tasks]->fn = maintenance_task_commit_graph;
> + num_tasks++;
Again, I am not sure if we want to keep piling on this.
> 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.
> GIT_TRACE2_EVENT="$(pwd)/run-no-auto.txt" git maintenance run --no-quiet &&
> GIT_TRACE2_EVENT="$(pwd)/run-auto.txt" git maintenance run --auto &&
> GIT_TRACE2_EVENT="$(pwd)/run-quiet.txt" git maintenance run --quiet &&
