Re: [PATCH v4 2/6] commit: add amend suboption to --fixup to create amend! commit

[Charvi Mendiratta <charvi077@gmail.com>]

On Sun, 14 Mar 2021 at 07:55, Junio C Hamano <gitster@pobox.com> wrote:
>
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
> > The one thing that does bother me, however, is the name of the
> > function, get_alpha_len(), which tells you (somewhat) literally what
> > it does but doesn't convey to the reader its actual purpose (which is
> > something we should strive for when naming functions and variables).
>
> I actually think the helper function that is used as a building
> block the "subcommand parser" uses should be named more directly
> to represent what it does (i.e. look for a run of alphas) than
> what it means (i.e. look for a run of letters allowed in a
> subcommand name).  IOW
>
>         char *end = skip_alphas(ptr);
>         if (*end == ':' && ptr != end) {
>                 /*
>                  * ptr..end could be a subcommand in
>                  * "--fixup=<subcommand>:"; see if it is a known one
>                  */
>                 *end = '\0';
>                 if (!strcmp(ptr, "amend"))
>                         ... do the amend thing ...
>                 else if (!strcmp(ptr, "reword"))
>                         ... do the reword thing ...
>                 else
>                         ... we do not know such a subcommand yet ...
>         } else {
>                 /* assume it is --fixup=<command> form */
>                 ...
>         }
>
> conveys more information to readers than a variant where you replace
> "skip_alphas" with "skip_subcommand_chars" without losing any
> information.
>

I thought to just rename get_alpha_len() to skip_alpha() that returns
alpha length. But even removing the "len" variable and implementing as
suggested above seems a better and clear alternative. I also agree to
update it.

Thanks for the suggestions.

Thanks and Regards,
Charvi