| Message ID | 85268bd53ef7f4e7b3d97a8ae091290e8e74441d.1597760589.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Maintenance I: Command, gc and commit-graph tasks | expand |
> @@ -66,6 +68,10 @@ OPTIONS > --quiet:: > Do not report progress or other information over `stderr`. > > +--task=<task>:: > + If this option is specified one or more times, then only run the > + specified tasks in the specified order. We should list the accepted tasks somewhere but maybe this can wait until after part 2. > @@ -791,7 +791,9 @@ typedef int maintenance_task_fn(struct maintenance_opts *opts); > struct maintenance_task { > const char *name; > maintenance_task_fn *fn; > - unsigned enabled:1; > + unsigned enabled:1, > + selected:1; > + int selected_order; > }; "selected" and "selected_order" are redundant in some cases - I think this would be better if selected_order is negative if this task is not selected, and non-negative otherwise. Apart from that, maybe this should be documented. It is unusual (to me) that a selection can override something being enabled, but that is the case here.
Jonathan Tan <jonathantanmy@google.com> writes: >> @@ -66,6 +68,10 @@ OPTIONS >> --quiet:: >> Do not report progress or other information over `stderr`. >> >> +--task=<task>:: >> + If this option is specified one or more times, then only run the >> + specified tasks in the specified order. > > We should list the accepted tasks somewhere but maybe this can wait > until after part 2. > >> @@ -791,7 +791,9 @@ typedef int maintenance_task_fn(struct maintenance_opts *opts); >> struct maintenance_task { >> const char *name; >> maintenance_task_fn *fn; >> - unsigned enabled:1; >> + unsigned enabled:1, >> + selected:1; >> + int selected_order; >> }; > > "selected" and "selected_order" are redundant in some cases - I think > this would be better if selected_order is negative if this task is not > selected, and non-negative otherwise. It is good to get rid of redundancies. > Apart from that, maybe this should be documented. It is unusual (to me) > that a selection can override something being enabled, but that is the > case here. I had the same reaction as "(to me)" above during an earlier review round, IIRC. This definitely deserves documentation.
On 8/18/2020 8:36 PM, Junio C Hamano wrote: > Jonathan Tan <jonathantanmy@google.com> writes: > >>> @@ -66,6 +68,10 @@ OPTIONS >>> --quiet:: >>> Do not report progress or other information over `stderr`. >>> >>> +--task=<task>:: >>> + If this option is specified one or more times, then only run the >>> + specified tasks in the specified order. >> >> We should list the accepted tasks somewhere but maybe this can wait >> until after part 2. It's hard to see from the patch-context, but there is a section titled "TASKS" that lists the 'gc' and 'commit-graph' tasks from the earlier patches. Would a reference to that section be helpful? See the TASKS section for the list of available tasks. >>> @@ -791,7 +791,9 @@ typedef int maintenance_task_fn(struct maintenance_opts *opts); >>> struct maintenance_task { >>> const char *name; >>> maintenance_task_fn *fn; >>> - unsigned enabled:1; >>> + unsigned enabled:1, >>> + selected:1; >>> + int selected_order; >>> }; >> >> "selected" and "selected_order" are redundant in some cases - I think >> this would be better if selected_order is negative if this task is not >> selected, and non-negative otherwise. > > It is good to get rid of redundancies. > >> Apart from that, maybe this should be documented. It is unusual (to me) >> that a selection can override something being enabled, but that is the >> case here. > > I had the same reaction as "(to me)" above during an earlier review > round, IIRC. This definitely deserves documentation. Will do. Thanks. -Stolee
> On 8/18/2020 8:36 PM, Junio C Hamano wrote: > > Jonathan Tan <jonathantanmy@google.com> writes: > > > >>> @@ -66,6 +68,10 @@ OPTIONS > >>> --quiet:: > >>> Do not report progress or other information over `stderr`. > >>> > >>> +--task=<task>:: > >>> + If this option is specified one or more times, then only run the > >>> + specified tasks in the specified order. > >> > >> We should list the accepted tasks somewhere but maybe this can wait > >> until after part 2. > > It's hard to see from the patch-context, but there is a section titled > "TASKS" that lists the 'gc' and 'commit-graph' tasks from the earlier > patches. Would a reference to that section be helpful? > > See the TASKS section for the list of available tasks. Ah, I missed that. What you have now is fine then, but a reference might be helpful if this man page ever grows.
diff --git a/Documentation/git-maintenance.txt b/Documentation/git-maintenance.txt index c816fa1dcd..cf59eb258c 100644 --- a/Documentation/git-maintenance.txt +++ b/Documentation/git-maintenance.txt @@ -30,7 +30,9 @@ SUBCOMMANDS ----------- run:: - Run one or more maintenance tasks. + Run one or more maintenance tasks. If one or more `--task=<task>` + options are specified, then those tasks are run in the provided + order. Otherwise, only the `gc` task is run. TASKS ----- @@ -66,6 +68,10 @@ OPTIONS --quiet:: Do not report progress or other information over `stderr`. +--task=<task>:: + If this option is specified one or more times, then only run the + specified tasks in the specified order. + GIT --- Part of the linkgit:git[1] suite diff --git a/builtin/gc.c b/builtin/gc.c index 4f9352b9d0..0231d2f8c1 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -791,7 +791,9 @@ typedef int maintenance_task_fn(struct maintenance_opts *opts); struct maintenance_task { const char *name; maintenance_task_fn *fn; - unsigned enabled:1; + unsigned enabled:1, + selected:1; + int selected_order; }; enum maintenance_task_label { @@ -814,13 +816,32 @@ static struct maintenance_task tasks[] = { }, }; +static int compare_tasks_by_selection(const void *a_, const void *b_) +{ + const struct maintenance_task *a, *b; + + a = (const struct maintenance_task *)&a_; + b = (const struct maintenance_task *)&b_; + + return b->selected_order - a->selected_order; +} + static int maintenance_run(struct maintenance_opts *opts) { - int i; + int i, found_selected = 0; int result = 0; + for (i = 0; !found_selected && i < TASK__COUNT; i++) + found_selected = tasks[i].selected; + + if (found_selected) + QSORT(tasks, TASK__COUNT, compare_tasks_by_selection); + for (i = 0; i < TASK__COUNT; i++) { - if (!tasks[i].enabled) + if (found_selected && !tasks[i].selected) + continue; + + if (!found_selected && !tasks[i].enabled) continue; if (tasks[i].fn(opts)) { @@ -832,6 +853,37 @@ static int maintenance_run(struct maintenance_opts *opts) return result; } +static int task_option_parse(const struct option *opt, + const char *arg, int unset) +{ + int i, num_selected = 0; + struct maintenance_task *task = NULL; + + BUG_ON_OPT_NEG(unset); + + for (i = 0; i < TASK__COUNT; i++) { + num_selected += tasks[i].selected; + if (!strcasecmp(tasks[i].name, arg)) { + task = &tasks[i]; + } + } + + if (!task) { + error(_("'%s' is not a valid task"), arg); + return 1; + } + + if (task->selected) { + error(_("task '%s' cannot be selected multiple times"), arg); + return 1; + } + + task->selected = 1; + task->selected_order = num_selected + 1; + + return 0; +} + int cmd_maintenance(int argc, const char **argv, const char *prefix) { struct maintenance_opts opts; @@ -840,6 +892,9 @@ int cmd_maintenance(int argc, const char **argv, const char *prefix) N_("run tasks based on the state of the repository")), OPT_BOOL(0, "quiet", &opts.quiet, N_("do not report progress or other information over stderr")), + OPT_CALLBACK_F(0, "task", NULL, N_("task"), + N_("run a specific task"), + PARSE_OPT_NONEG, task_option_parse), OPT_END() }; diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh index c0c4e6846e..792765aff7 100755 --- a/t/t7900-maintenance.sh +++ b/t/t7900-maintenance.sh @@ -25,4 +25,31 @@ test_expect_success 'run [--auto|--quiet]' ' test_subcommand git gc --no-quiet <run-no-quiet.txt ' +test_expect_success 'run --task=<task>' ' + GIT_TRACE2_EVENT="$(pwd)/run-commit-graph.txt" \ + git maintenance run --task=commit-graph 2>/dev/null && + GIT_TRACE2_EVENT="$(pwd)/run-gc.txt" \ + git maintenance run --task=gc 2>/dev/null && + GIT_TRACE2_EVENT="$(pwd)/run-commit-graph.txt" \ + git maintenance run --task=commit-graph 2>/dev/null && + GIT_TRACE2_EVENT="$(pwd)/run-both.txt" \ + git maintenance run --task=commit-graph --task=gc 2>/dev/null && + test_subcommand ! git gc --quiet <run-commit-graph.txt && + test_subcommand git gc --quiet <run-gc.txt && + test_subcommand git gc --quiet <run-both.txt && + test_subcommand git commit-graph write --split --reachable --no-progress <run-commit-graph.txt && + test_subcommand ! git commit-graph write --split --reachable --no-progress <run-gc.txt && + test_subcommand git commit-graph write --split --reachable --no-progress <run-both.txt +' + +test_expect_success 'run --task=bogus' ' + test_must_fail git maintenance run --task=bogus 2>err && + test_i18ngrep "is not a valid task" err +' + +test_expect_success 'run --task duplicate' ' + test_must_fail git maintenance run --task=gc --task=gc 2>err && + test_i18ngrep "cannot be selected multiple times" err +' + test_done