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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 3, 2021, 07:46 UTC
Message-ID: <CAPig+cSJCSLBDumaXvnkdSBTATs62h9EfgtgJu0O7R_PJEumCg@mail.gmail.com>
In-Reply-To: <CAPSFM5dm7vD8pszCG+JKNb7Hu6Zp48wKXaj9ayxe6yaf6swePA@mail.gmail.com>

On Wed, Mar 3, 2021 at 2:37 AM Charvi Mendiratta <charvi077@gmail.com> wrote:

Show 12 quoted lines

> On Tue, 2 Mar 2021 at 03:45, Eric Sunshine <sunshine@sunshineco.com> wrote:
> > > +       if (starts_with(sb->buf, "amend! amend!"))
> >
> > Is the content of the incoming strbuf created mechanically so that we
> > know that there will only ever be one space between the two "amend!"
> > literals? If not, then this starts_with() check feels fragile.
>
> Yes, so for preparing each "amend!" commit we add prefix "amend! '' to
> the subject of the specific commit. And further if we amend the
> "amend!" commit then this above code is checked before creating a
> "amend! amend!" commit for the user. So I think maybe we don't need to
> check for multiple spaces ?

Okay, if this is guaranteed to be created mechanically, then what you have should work, though it may be a good idea to add an in-code comment stating the reason it is okay to expect just the single space.

The alternative would be to avoid having "amend! amend!" in the first place. I didn't trace through the code carefully so I don't know if it is possible, but would it make sense for the caller(s) to check before adding a second "amend!", thus eliminating the need to do so here? (Perhaps I'm misunderstanding, but the above code almost feels like a case of "whoops, we did something undesirable, so let's undo it.".)