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

[Eric Sunshine <sunshine@sunshineco.com>]

On Thu, Mar 11, 2021 at 10:24 AM Charvi Mendiratta <charvi077@gmail.com> wrote:
> On Thu, 11 Mar 2021 at 11:55, Eric Sunshine <sunshine@sunshineco.com> wrote:
> > > +               size_t len = get_alpha_len(fixup_message);
> > > +               if (len && fixup_message[len] == ':') {
> > > +                       fixup_message[len++] = '\0';
> > > +                       fixup_commit = fixup_message + len;
> >
> > An alternate -- just about as compact and perhaps more idiomatic --
> > way to write all this without introducing the new get_alpha_len()
> > function:
> >
> >     char *p = fixup_mesage;
> >     while (isalpha(*p))
> >         p++;
> >     if (p > fixup_message && *p == ':') {
> >         *p = '\0';
> >         fixup_commit = p + 1;
>
> Earlier we had discussed[1] keeping a separate helper function, so
> that it may re-use it later. But I agree above is easier to get and
> compact so I think maybe it will be ok, for this patch series to
> replace it with the above and remove the function.

I don't have strong feelings one way or the other whether you should
use a function or inline it as I showed above, and since our aim is to
land this series rather than endlessly re-rolling it, let's not spend
a lot of cycles worrying about it.

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).
In that previous discussion you referenced, Junio mentioned that a
future sub-option might want to have punctuation in its name. If that
ever comes about, then the name get_alpha_len() no longer makes sense
and needs to be renamed so it doesn't become a lie. Giving the
function a better name up front would not only help readers now to
understand what is going on, but would also help down the road when or
if punctuation (or numbers or whatnot) become valid in a sub-option
name. suboption_length() is one possibility. With a slight semantic
change, skip_suboption() or latch_suboption() are other possibilities.
Or, if you were to open-code the loop as I did above, then you might
have a function named is_suboption_char() and use that in place of
isalpha().

So, if you do re-roll, I wouldn't mind seeing a better name for the
function; but the rest is subjective and not worth spending time
refining unless you actually feel that one style has a clear advantage
over others.