Hi,
Derrick Stolee wrote:
> [Subject: maintenance: create basic maintenance runner]
Seems sensible, and a good way to set up for the later patches. Let's
take a look at how it does that.
[...]
> --- /dev/null
> +++ b/Documentation/git-maintenance.txt
> @@ -0,0 +1,57 @@
[...]
> +SUBCOMMANDS
> +-----------
> +
> +run::
> + Run one or more maintenance tasks.
[jrnieder] How do I supply the tasks on the command line? Are they
parameters to this subcommand? If so, it could make sense for this to
say something like
run <task>...::
What is the exit code convention for "git maintenance run"? (Many Git
commands don't document this, but since we're making a new command
it seems worth building the habit.)
[...]
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -699,3 +699,62 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
[...]
> +static struct maintenance_opts {
> + int auto_flag;
> +} opts;
Packing this in a struct feels a bit unusual. Is the struct going to
be passed somewhere, or could these be individual locals in
cmd_maintenance?
[...]
> +
> +static int maintenance_task_gc(void)
> +{
> + int result;
> + struct argv_array cmd = ARGV_ARRAY_INIT;
> +
> + argv_array_pushl(&cmd, "gc", NULL);
> +
> + if (opts.auto_flag)
> + argv_array_pushl(&cmd, "--auto", NULL);
These are both pushing single strings, so they can use argv_array_push.
[...]
> --- /dev/null
> +++ b/t/t7900-maintenance.sh
> @@ -0,0 +1,22 @@
> +#!/bin/sh
> +
> +test_description='git maintenance builtin'
> +
> +GIT_TEST_COMMIT_GRAPH=0
> +GIT_TEST_MULTI_PACK_INDEX=0
Why does this disable commit graph and multipack index? Is that setting
up for something that comes later?
[...]
> +test_expect_success 'gc [--auto]' '
> + GIT_TRACE2_EVENT="$(pwd)/run-no-auto.txt" git maintenance run &&
> + GIT_TRACE2_EVENT="$(pwd)/run-auto.txt" git maintenance run --auto &&
> + grep ",\"gc\"]" run-no-auto.txt &&
> + grep ",\"gc\",\"--auto\"]" run-auto.txt
This feels a little odd in two ways:
- the use of "git gc" isn't a user-facing detail, so this is testing
implementation instead of the user-facing behavior. That's okay,
though --- it can be useful to test internals sometimes.
- the way that this tests for "git gc" feels brittle: if the trace
emitter changes some day to include a space after the comma, for
example, then the test would start failing. I thought that in the
spirit of fakes, it could make sense to write a custom "git gc"
command using test_write_script, but that isn't likely to work
either since gc is a builtin.
Perhaps this is suggesting that we need some central test helper for
parsing traces so we can do this reliably in one place. What do you
think?
Thanks,
Jonathan
