From: Ramkumar Ramachandra <artagnon@gmail.com>
Date: Wed, 11 Jan 2012 04:02:18 GMT
Subject: Re: [PATCH 2/8] revert: decouple sequencer actions from builtin commands
Message-ID: <CALkWK0k=44znLr2oYSx61Mk=qdAurona0f0H4i4=YXNSAeQhHQ@mail.gmail.com>
In-Reply-To: <20120110183857.GC22184@burratino>

Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
> [...]
>> -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.

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