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

[Jonathan Nieder <jrnieder@gmail.com>]

Ramkumar Ramachandra wrote:

> 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
[...]
> @@ -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.

[...]
> @@ -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