Re: [PATCH v3 2/6] commit: add amend suboption to --fixup to create amend! commit
- From
Charvi Mendiratta <charvi077@gmail.com>
- Date
- Mar 3, 2021, 07:42 UTC
- Message-ID
- <CAPSFM5eosfQofhZjPz_TbchrSqj07EnQPKU0MFvgG=LtsiKVPg@mail.gmail.com>
- In-Reply-To
- <CAPig+cQ9wBLdKqmgjX4eOT6mvukmHXCDicTo5Ud+4VTkzxcXrg@mail.gmail.com>
On Tue, 2 Mar 2021 at 04:17, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 25 quoted lines
>
> 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.Okay, I will reword it and make it more clear regarding the main reason for choosing this way.