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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 14, 2021, 02:25 UTC
Message-ID: <xmqqft0y4gl5.fsf@gitster.g>
In-Reply-To: <CAPig+cTBXBjZkJoGFYpzWBnEOmV52dnoBHxceuoe3pu3F0BnYw@mail.gmail.com>

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.

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.