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

Eric Sunshine [off-list ref] 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.

Yes, in different contexts, where a helpers are designed to be used
by multiple callers that may not even be aware of each other, we do
encourage naming them after what they do _means_.  But in this
codepath, I do not think it applies.

Thanks.

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