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

From: Eric Sunshine <sunshine@sunshineco.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: 20210217072904.16257-1-charvi077@gmail.com,
	Git List <git@vger.kernel.org>,
	Christian Couder <christian.couder@gmail.com>,
	Phillip Wood <phillip.wood123@gmail.com>,
	Charvi Mendiratta <charvi077@gmail.com>,
	Christian Couder <chriscool@tuxfamily.org>,
	Phillip Wood <phillip.wood@dunelm.org.uk>
Subject: Re: [PATCH v3 2/6] commit: add amend suboption to --fixup to create amend! commit
Date: Mon, 1 Mar 2021 17:47:38 -0500	[thread overview]
Message-ID: <CAPig+cQ9wBLdKqmgjX4eOT6mvukmHXCDicTo5Ud+4VTkzxcXrg@mail.gmail.com> (raw)
In-Reply-To: <xmqqlfb6mrnx.fsf@gitster.c.googlers.com>

On Mon, Mar 1, 2021 at 5:32 PM Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> >     if (skip_prefix(msg, "amend:", &arg) ||
> >         skip_prefix(msg, "reword:", &arg)) {
> >         ...
> >     }
>
> You still need to compute "len" because you'd want to tell between
> --fixup="HEAD^{/^area: string}" and --fixup=bogus:HEAD (the latter
> would want to say "no such variant 'bogus' for --fixup", but the
> colon in the former is not the end of the name of variant.

I see what you mean. I vaguely recall quickly scanning over that
earlier discussion about ":" being otherwise legitimate when embedded
in the argument as you demonstrate, but didn't think about it when
reading this code. Perhaps the comment which this code adds:

    * As `amend` suboption contains only alpha
    * character. So check if first non alpha
    * character in fixup_message is ':'.

could be extended a bit to mention that briefly since, without it, the
significance of "alpha-only characters followed by colon" is not
immediately obvious.