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

Charvi Mendiratta [off-list ref] writes:

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

quoted

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.

FWIW I am also fine with Eric's simpler "open code it right there"
suggestion in this case.  Just like the "skip alphas" suggestion, it
makes the logic to parse subcommand name out isolated to a single
place without asking readers to refer to the implementation of a
helper, and it would be short enough.

Thanks.

On Sun, Mar 14, 2021 at 6:43 PM Junio C Hamano [off-list ref] wrote:

FWIW I am also fine with Eric's simpler "open code it right there"
suggestion in this case.  Just like the "skip alphas" suggestion, it
makes the logic to parse subcommand name out isolated to a single
place without asking readers to refer to the implementation of a
helper, and it would be short enough.

Likewise. If you're going to re-roll anyhow, the open-coded:

    char *p = fixup_mesage;
    while (isalpha(*p))
        p++;
    if (p > fixup_message && *p == ':') {
        *p = '\0';
        fixup_commit = p + 1;

would be perfectly fine with me too (or any simple variation on that
theme). Whether or not it's worth re-rolling again, I leave up to you
and Junio.

On Mon, 15 Mar 2021 at 04:37, Eric Sunshine [off-list ref] wrote:

On Sun, Mar 14, 2021 at 6:43 PM Junio C Hamano [off-list ref] wrote:

quoted

FWIW I am also fine with Eric's simpler "open code it right there"
suggestion in this case.  Just like the "skip alphas" suggestion, it
makes the logic to parse subcommand name out isolated to a single
place without asking readers to refer to the implementation of a
helper, and it would be short enough.

Likewise. If you're going to re-roll anyhow, the open-coded:

    char *p = fixup_mesage;
    while (isalpha(*p))
        p++;
    if (p > fixup_message && *p == ':') {
        *p = '\0';
        fixup_commit = p + 1;

would be perfectly fine with me too (or any simple variation on that
theme). Whether or not it's worth re-rolling again, I leave up to you
and Junio.

okay, I agree too. I have updated it in the re-roll.

Thanks for all the detailed reviews and suggestions.

Thanks and Regards,
Charvi

On Mon, Mar 15, 2021 at 3:59 AM Charvi Mendiratta [off-list ref] wrote:

On Mon, 15 Mar 2021 at 04:37, Eric Sunshine [off-list ref] wrote:

quoted

Likewise. If you're going to re-roll anyhow, the open-coded:
would be perfectly fine with me too (or any simple variation on that
theme). Whether or not it's worth re-rolling again, I leave up to you
and Junio.

okay, I agree too. I have updated it in the re-roll.

Thanks for all the detailed reviews and suggestions.

Thanks for patiently putting up with reviewers who sometimes have
opposing or contradictory opinions and recommendations.

On Mon, 15 Mar 2021 at 13:46, Eric Sunshine [off-list ref] wrote:
[..]

Thanks for patiently putting up with reviewers who sometimes have
opposing or contradictory opinions and recommendations.

It was more a learning path and helpful for me to proceed. I really
appreciate all the guidance and suggestions received. Glad to get this
merge!

Thanks and regards,
Charvi