Re: [PATCH 5/5] revert: simplify communicating command-line arguments

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Dec 7, 2011, 08:09 UTC
Message-ID: <20111207080944.GF11737@elie.hsd1.il.comcast.net>
In-Reply-To: <1323239877-24101-6-git-send-email-artagnon@gmail.com>

Ramkumar Ramachandra wrote:

Show 9 quoted lines

> From: Jonathan Nieder <jrnieder@gmail.com>
>
> Currently, command-line arguments are communicated using (argc, argv)
> until a prepare_revs() turns it into a terse structure.  However,
> since we plan to expose the cherry-picking machinery through a public
> API in the future, we want callers to be able to call in with a
> filled-in structure.  For the revert builtin, this means that the
> chief argument parser, parse_args(), should parse into such a
> structure.  Make this change.

Fine. I guess it can be said more clearly while emphasizing public interfaces over implementation details:

	Since the "multiple cherry-pick" feature was introduced (commit
	7e2bfd3f, 2010-07-02), the pick/revert machinery has kept track
	of the set of commits to be cherry-picked or reverted using
	commit_argc and commit_argv variables storing the relevant
	command-line parameters.

	Future callers as other commands are built in (am, rebase,
	sequencer) may find it easier to pass rev-list options to this
	machinery in already-parsed form, though.  So teach
	cmd_cherry_pick and cmd_revert to parse the rev-list arguments
	in advance and pass the commit set to pick_revisions() as a
	value of type "struct rev_info".

[...]

Show 20 quoted lines

> @@ -222,7 +220,20 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
>  				"-x", opts->record_origin,
>  				"--edit", opts->edit,
>  				NULL);
> -	opts->commit_argv = argv;
> +
> +	if (opts->subcommand == REPLAY_NONE) {
> +		opts->revs = xmalloc(sizeof(*opts->revs));
> +		init_revisions(opts->revs, NULL);
> +		opts->revs->no_walk = 1;
> +		if (argc < 2)
> +			usage_with_options(usage_str, options);
> +		argc = setup_revisions(argc, argv, opts->revs, NULL);
> +	} else
> +		opts->revs = NULL;
> +
> +	/* Forbid stray command-line arguments */
> +	if (argc > 1)
> +		usage_with_options(usage_str, options);
>  }

Looks good.

Tests? What happens if I try "git cherry-pick --all"? How about "git cherry-pick HEAD..HEAD"?

FWIW, with the commit message clarified and with or without new tests,
Acked-by: Jonathan Nieder <jrnieder@gmail.com>