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

Jonathan Nieder wrote:

Ramkumar Ramachandra wrote:
[...]

quoted

-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.

Carried over from the previous iteration- sorry I forgot to fix this.

[...]

quoted

@@ -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?

Fine;  I'm sold on the string idea.  Also, I figured it would be
easier to explain the changes if I change this enum to a string -- I
should probably use "ease of explaining changes" as a stronger
criterion in the future when I have two competing implementations in
mind.

Thanks.

-- Ram