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

On Sun, 14 Mar 2021 at 07:55, Junio C Hamano [off-list ref] wrote:

Eric Sunshine [off-list ref] writes:

quoted

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