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, 19:21 UTC
Message-ID: <CAPSFM5cECFPk180G3aOdqnZ09FogwsBzC_WwO4OFutsjnTXQ7w@mail.gmail.com>
In-Reply-To: <CAPig+cSJCSLBDumaXvnkdSBTATs62h9EfgtgJu0O7R_PJEumCg@mail.gmail.com>

Hi Eric,

On Wed, 3 Mar 2021 at 13:16, Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 21 quoted lines

>
> On Wed, Mar 3, 2021 at 2:37 AM Charvi Mendiratta <charvi077@gmail.com> wrote:
> > 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.

Agree. I think we can do this...

Show 5 quoted lines

> 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.".)

I looked into it and got another alternative, to extend the same prepare_amend_commit() function and replace the check condition of if (starts_with(sb->buf, "amend! amend!")) with the code as below :

const char *buffer = get_commit_buffer(commit, NULL); const char *subject; find_commit_subject(buffer, &subject); if (starts_with(subject, "amend!")) const char *fmt = starts_with(subject, "amend!") ? "%b" : "%B"; format_commit_message(commit, fmt, sb, ctx); unuse_commit_buffer(commit, buffer);

So, now it checks the commit subject here only. Otherwise as you have suggested above to check before adding a second "amend!", I think that can result in confusion as currently both "fixup!" and "amend!" commits (commit's subject) are prepared by same code and further for "amend!" commit as we write a commit message body also so we used prepare_amend_commit() to do that stuff.