On Sun, 14 Mar 2021 at 07:55, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> Eric Sunshine <sunshine@xxxxxxxxxxxxxx> 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
