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

Hi Eric,

On Wed, 3 Mar 2021 at 13:16, Eric Sunshine [off-list ref] wrote:

On Wed, Mar 3, 2021 at 2:37 AM Charvi Mendiratta [off-list ref] wrote:

quoted

On Tue, 2 Mar 2021 at 03:45, Eric Sunshine [off-list ref] wrote:

quoted

quoted

+       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...

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.