Re: [PATCH 2/3] builtin/commit-graph.c: introduce '--input=<source>'

[Taylor Blau <me@ttaylorr.com>]

On Fri, Jan 31, 2020 at 09:40:23AM -0500, Derrick Stolee wrote:
> On 1/30/2020 7:28 PM, Taylor Blau wrote:
> > The 'write' mode of the 'commit-graph' supports input from a number of
> > different sources: pack indexes over stdin, commits over stdin, commits
> > reachable from all references, and so on. Each of these options are
> > specified with a unique option: '--stdin-packs', '--stdin-commits', etc.
> >
> > Similar to our replacement of 'git config [--<type>]' with 'git config
> > [--type=<type>]' (c.f., fb0dc3bac1 (builtin/config.c: support
> > `--type=<type>` as preferred alias for `--<type>`, 2018-04-18)), softly
> > deprecate '[--<input>]' in favor of '[--input=<source>]'.
> >
> > This makes it more clear to implement new options that are combinations
> > of other options (such as, for example, "none", a combination of the old
> > "--append" and a new sentinel to specify to _not_ look in other packs,
> > which we will implement in a future patch).
> >
> > Unfortunately, the new enumerated type is a bitfield, even though it
> > makes much more sense as '0, 1, 2, ...'. Even though *almost* all
> > options are pairwise exclusive, '--stdin-{packs,commits}' *is*
> > compatible with '--append'. For this reason, use a bitfield.
> >
> > Signed-off-by: Taylor Blau <me@ttaylorr.com>
> > ---
> >  Documentation/git-commit-graph.txt | 26 +++++-----
> >  builtin/commit-graph.c             | 77 ++++++++++++++++++++++--------
> >  t/t5318-commit-graph.sh            | 46 +++++++++---------
> >  t/t5324-split-commit-graph.sh      | 44 ++++++++---------
> >  4 files changed, 114 insertions(+), 79 deletions(-)
> >
> > diff --git a/Documentation/git-commit-graph.txt b/Documentation/git-commit-graph.txt
> > index 8d61ba9f56..cbf80226e9 100644
> > --- a/Documentation/git-commit-graph.txt
> > +++ b/Documentation/git-commit-graph.txt
> > @@ -41,21 +41,21 @@ COMMANDS
> >
> >  Write a commit-graph file based on the commits found in packfiles.
> >  +
> > -With the `--stdin-packs` option, generate the new commit graph by
> > +With the `--input=stdin-packs` option, generate the new commit graph by
> >  walking objects only in the specified pack-indexes. (Cannot be combined
> > -with `--stdin-commits` or `--reachable`.)
> > +with `--input=stdin-commits` or `--input=reachable`.)
> >  +
> > -With the `--stdin-commits` option, generate the new commit graph by
> > -walking commits starting at the commits specified in stdin as a list
> > +With the `--input=stdin-commits` option, generate the new commit graph
> > +by walking commits starting at the commits specified in stdin as a list
> >  of OIDs in hex, one OID per line. (Cannot be combined with
> > -`--stdin-packs` or `--reachable`.)
> > +`--input=stdin-packs` or `--input=reachable`.)
> >  +
> > -With the `--reachable` option, generate the new commit graph by walking
> > -commits starting at all refs. (Cannot be combined with `--stdin-commits`
> > -or `--stdin-packs`.)
> > +With the `--input=reachable` option, generate the new commit graph by
> > +walking commits starting at all refs. (Cannot be combined with
> > +`--input=stdin-commits` or `--input=stdin-packs`.)
> >  +
> > -With the `--append` option, include all commits that are present in the
> > -existing commit-graph file.
> > +With the `--input=append` option, include all commits that are present
> > +in the existing commit-graph file.
> >  +
> >  With the `--split[=<strategy>]` option, write the commit-graph as a
> >  chain of multiple commit-graph files stored in
> > @@ -107,20 +107,20 @@ $ git commit-graph write
> >    using commits in `<pack-index>`.
> >  +
> >  ------------------------------------------------
> > -$ echo <pack-index> | git commit-graph write --stdin-packs
> > +$ echo <pack-index> | git commit-graph write --input=stdin-packs
> >  ------------------------------------------------
> >
> >  * Write a commit-graph file containing all reachable commits.
> >  +
> >  ------------------------------------------------
> > -$ git show-ref -s | git commit-graph write --stdin-commits
> > +$ git show-ref -s | git commit-graph write --input=stdin-commits
> >  ------------------------------------------------
> >
> >  * Write a commit-graph file containing all commits in the current
> >    commit-graph file along with those reachable from `HEAD`.
> >  +
> >  ------------------------------------------------
> > -$ git rev-parse HEAD | git commit-graph write --stdin-commits --append
> > +$ git rev-parse HEAD | git commit-graph write --input=stdin-commits --input=append
> >  ------------------------------------------------
> >
> >
> > diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
> > index f03b46d627..03d815e652 100644
> > --- a/builtin/commit-graph.c
> > +++ b/builtin/commit-graph.c
> > @@ -10,7 +10,7 @@
> >  static char const * const builtin_commit_graph_usage[] = {
> >  	N_("git commit-graph verify [--object-dir <objdir>] [--shallow] [--[no-]progress]"),
> >  	N_("git commit-graph write [--object-dir <objdir>] [--append] "
> > -	   "[--split[=<strategy>]] [--reachable|--stdin-packs|--stdin-commits] "
> > +	   "[--split[=<strategy>]] [--input=<reachable|stdin-packs|stdin-commits>] "
> >  	   "[--[no-]progress] <split options>"),
> >  	NULL
> >  };
> > @@ -22,22 +22,48 @@ static const char * const builtin_commit_graph_verify_usage[] = {
> >
> >  static const char * const builtin_commit_graph_write_usage[] = {
> >  	N_("git commit-graph write [--object-dir <objdir>] [--append] "
> > -	   "[--split[=<strategy>]] [--reachable|--stdin-packs|--stdin-commits] "
> > +	   "[--split[=<strategy>]] [--input=<reachable|stdin-packs|stdin-commits>] "
> >  	   "[--[no-]progress] <split options>"),
> >  	NULL
> >  };
> >
> > +enum commit_graph_input {
> > +	COMMIT_GRAPH_INPUT_REACHABLE     = (1 << 1),
> > +	COMMIT_GRAPH_INPUT_STDIN_PACKS   = (1 << 2),
> > +	COMMIT_GRAPH_INPUT_STDIN_COMMITS = (1 << 3),
> > +	COMMIT_GRAPH_INPUT_APPEND        = (1 << 4)
> > +};
> > +
> >  static struct opts_commit_graph {
> >  	const char *obj_dir;
> > -	int reachable;
> > -	int stdin_packs;
> > -	int stdin_commits;
> > -	int append;
> > +	enum commit_graph_input input;
> >  	int split;
> >  	int shallow;
> >  	int progress;
> >  } opts;
> >
> > +static int option_parse_input(const struct option *opt, const char *arg,
> > +			      int unset)
> > +{
> > +	enum commit_graph_input *to = opt->value;
> > +	if (unset || !strcmp(arg, "packs")) {
> > +		*to = 0;
> > +		return 0;
> > +	}
>
> Here, you _do_ clear the bitfield, allowing "--input=reachable --input"
> to do the correct override. Thanks!
>
> > +
> > +	if (!strcmp(arg, "reachable"))
> > +		*to |= COMMIT_GRAPH_INPUT_REACHABLE;
> > +	else if (!strcmp(arg, "stdin-packs"))
> > +		*to |= COMMIT_GRAPH_INPUT_STDIN_PACKS;
> > +	else if (!strcmp(arg, "stdin-commits"))
> > +		*to |= COMMIT_GRAPH_INPUT_STDIN_COMMITS;
> > +	else if (!strcmp(arg, "append"))
> > +		*to |= COMMIT_GRAPH_INPUT_APPEND;
> > +	else
> > +		die(_("unrecognized --input source, %s"), arg);
> > +	return 0;
> > +}
> > +
> >  static struct object_directory *find_odb_or_die(struct repository *r,
> >  						const char *obj_dir)
> >  {
> > @@ -137,14 +163,21 @@ static int graph_write(int argc, const char **argv)
> >  		OPT_STRING(0, "object-dir", &opts.obj_dir,
> >  			N_("dir"),
> >  			N_("The object directory to store the graph")),
> > -		OPT_BOOL(0, "reachable", &opts.reachable,
> > -			N_("start walk at all refs")),
> > -		OPT_BOOL(0, "stdin-packs", &opts.stdin_packs,
> > -			N_("scan pack-indexes listed by stdin for commits")),
> > -		OPT_BOOL(0, "stdin-commits", &opts.stdin_commits,
> > -			N_("start walk at commits listed by stdin")),
> > -		OPT_BOOL(0, "append", &opts.append,
> > -			N_("include all commits already in the commit-graph file")),
> > +		OPT_CALLBACK(0, "input", &opts.input, NULL,
> > +			N_("include commits from this source in the graph"),
> > +			option_parse_input),
> > +		OPT_BIT(0, "reachable", &opts.input,
> > +			N_("start walk at all refs"),
> > +			COMMIT_GRAPH_INPUT_REACHABLE),
> > +		OPT_BIT(0, "stdin-packs", &opts.input,
> > +			N_("scan pack-indexes listed by stdin for commits"),
> > +			COMMIT_GRAPH_INPUT_STDIN_PACKS),
> > +		OPT_BIT(0, "stdin-commits", &opts.input,
> > +			N_("start walk at commits listed by stdin"),
> > +			COMMIT_GRAPH_INPUT_STDIN_COMMITS),
> > +		OPT_BIT(0, "append", &opts.input,
> > +			N_("include all commits already in the commit-graph file"),
> > +			COMMIT_GRAPH_INPUT_APPEND),
>
> Since you are rewriting how we interpret the deprecated options, perhaps we
> should keep some tests around that call these versions? It would make the
> test diff be a bit smaller. These options can be removed from the tests if/when
> we actually remove the options.

That sounds good. I thought about doing this in the original round, but
I talked myself out of it because it wasn't clear to me which tests were
the ones worth converting and which should be left alone.

But, since you think it's good, so do I. I picked the ones to convert
mostly at random, and left the new ones as-is using the '--input=' form.

> > @@ -351,10 +351,10 @@ test_expect_success '--split=merge-all always merges incrementals' '
> >  	git rev-list -3 HEAD~4 >a &&
> >  	git rev-list -2 HEAD~2 >b &&
> >  	git rev-list -2 HEAD >c &&
> > -	git commit-graph write --split=no-merge --stdin-commits <a &&
> > -	git commit-graph write --split=no-merge --stdin-commits <b &&
> > +	git commit-graph write --split=no-merge --input=stdin-commits <a &&
> > +	git commit-graph write --split=no-merge --input=stdin-commits <b &&
> >  	test_line_count = 2 $graphdir/commit-graph-chain &&
> > -	git commit-graph write --split=merge-all --stdin-commits <c &&
> > +	git commit-graph write --split=merge-all --input=stdin-commits <c &&
> >  	test_line_count = 1 $graphdir/commit-graph-chain
> >  '
> >
> > @@ -364,8 +364,8 @@ test_expect_success '--split=no-merge always writes an incremental' '
> >  	git reset --hard commits/2 &&
> >  	git rev-list HEAD~1 >a &&
> >  	git rev-list HEAD >b &&
> > -	git commit-graph write --split --stdin-commits <a &&
> > -	git commit-graph write --split=no-merge --stdin-commits <b &&
> > +	git commit-graph write --split --input=stdin-commits <a &&
> > +	git commit-graph write --split=no-merge --input=stdin-commits <b &&
> >  	test_line_count = 2 $graphdir/commit-graph-chain
> >  '
>
> Updating these new tests with the given options is good. Perhaps convert only one
> of the old tests for each of the stdin-packs, reachable, "", and "append" options?

Yup, thanks.

> Thanks,
> -Stolee

Thanks,
Taylor