Subject: Re: [PATCH v2 01/18] maintenance: create basic maintenance runner

From: Jonathan Nieder <[email-masked]>
Date: Wed, 29 Jul 2020 15:19:05 -0700

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