Re: [PATCH 2/8] revert: decouple sequencer actions from builtin commands

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Jan 10, 2012, 18:38 UTC
Message-ID: <20120110183857.GC22184@burratino>
In-Reply-To: <1326212039-13806-3-git-send-email-artagnon@gmail.com>

Ramkumar Ramachandra wrote:

Show 5 quoted lines

> Currently, 'git cherry-pick' fills up the '.git/sequencer/todo'
> instruction sheet with "pick" actions, while 'git revert' fills it up
> with "revert" actions.  Inspired by the way 'rebase -i' works, we
> would like to permit mixing arbitrary actions in the same instruction
> sheet.

Ok.

>        To do this, we first have to decouple the notion of an action
> in the instruction sheet from builtin commands.

Wha?

I guess at this point I get lost because you are using jargon. What is the difference between an action in the instruction sheet and a builtin command? How does being a busybox-style hard link to the main "git" binary as opposed to a separate binary (which is what "builtin" means) have anything to do with this at all?

And I don't have any sense of the impact. Will this change the behavior of "git cherry-pick"? Will it avoid some confusion on the part of future people modifying the code? Is it a necessary step before some future change we want? Does it just make the code prettier, and help the sanity of future readers in the process (which is definitely valuable, too)? The above doesn't make it clear to me.

> --- a/builtin/revert.c
> +++ b/builtin/revert.c

[...]

Show 7 quoted lines

> @@ -52,7 +57,7 @@ enum replay_subcommand {
>  };
>  
>  struct replay_opts {
> -	enum replay_action action;
> +	enum replay_command command;
>  	enum replay_subcommand subcommand;

This part seems to be renaming "action" to "command". A cosmetic change, which I don't have an immediate opinion about either way.

[...]

> -static const char *action_name(const struct replay_opts *opts)
> +static const char *command_name(struct replay_opts *opts)

This part does a similar renaming, and drops a const while at it for no intelligible reason.

[...]

Show 6 quoted lines

> @@ -142,7 +147,7 @@ static void verify_opt_mutually_compatible(const char *me, ...)
>  static void parse_args(int argc, const char **argv, struct replay_opts *opts)
>  {
>  	const char * const * usage_str = revert_or_cherry_pick_usage(opts);
> -	const char *me = action_name(opts);
> +	const char *me = command_name(opts);

The rest is stuff like this, which follows from the first part.

Stepping back, I think the idea is that "enum replay_action" is not a good way to identify the command name in error messages like

	fatal: cherry-pick: --abort cannot be used with --continue

So you introduce a _new_ enum to represent the command name. Why not just use a string, so commands using the nice and generic sequencer library do not have to register themselves in a global callers list to use it?

Plus the renaming of the ->action field and action_name() function feel gratuitous, and drowned out the actual changes in the noise.

Does that clarify?

I guess I agree with the problem you are solving, but I don't agree with the solution at all.

Thanks, Jonathan