From: Junio C Hamano <gitster@pobox.com>
Date: Sun, 14 Mar 2021 02:25:10 GMT
Subject: Re: [PATCH v4 2/6] commit: add amend suboption to --fixup to create amend! commit
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.