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

[Charvi Mendiratta <charvi077@gmail.com>]

On Fri, 26 Feb 2021 at 02:30, Junio C Hamano <gitster@pobox.com> wrote:
>
> Charvi Mendiratta <charvi077@gmail.com> writes:

> > Inorder to prevent rebase from creating commits with an empty
>
> In order to prevent ...
>
> > message we refuse to create an "amend!" commit if commit message
> > body is empty.
> > ...
> > +static int prepare_amend_commit(struct commit *commit, struct strbuf *sb,
> > +                                                             struct pretty_print_context *ctx) {
>
> Don't indent the second line unnecessarily too deep.
>

Okay, I will fix the above changes.

> > +     /*
> > +      * If we amend the 'amend!' commit then we don't want to
> > +      * duplicate the subject line.
> > +      */
> > +     const char *format = NULL;
> > +     if (starts_with(sb->buf, "amend! amend!"))
> > +             format = "%b";
> > +     else
> > +             format = "%B";
>
> I am not sure how well this strategy of special case only two amend!
> scales.  What would happen when we --fixup another "fixup!"commit?
>

This is applied for more than one "amend!" or in other words, "amend!"
commit chain. On the other hand we don't need to skip any subject if we
--fixup another fixup! commit because the resulting commit message is
just one liner. But yes if "fixup!" commit is combined with `--squash`
then it comments the complete "fixup!" commit line by finding its length
and increasing pointer.

> Shouldn't the caller, when it called format_commit_message() to
> prepare sb it passed to us, have stripped out existing prefix, if
> any, so that we can always use the same %B format, or something like
> that?
>

I am not sure about this, because I think in the way you have suggested, we need
to strip off the complete subject line instead of prefix. I am saying
this because the
commit message body of "amend!" commit contains the complete commit message
of the commit we are fixing up.
for example :
$ git commit --fixup=amend:HEAD will create commit with log message :
amend! subject of head

subject of head
body of head

and again if we `--fixup:amend` the HEAD commit then :
$ git commit --fixup=amend:HEAD (by default) will create commit with
log message:
amend! amend! subject of head

amend! subject of head /* we need to comment this complete line */

subject of head
body of head

So, I am not sure about the other option to implement it ?

> > ...
> > +             format_commit_message(commit, fmt, &sb, &ctx);
> > +             free(fmt);
> >               hook_arg1 = "message";
> > +
> > +             if ((have_option_m) && !strcmp(fixup_prefix,"fixup"))
>
> Unnecessary () around have_option_m, and missing SP after ",".
>
> > +                     strbuf_addbuf(&sb, &message);
> > +
> > +             if (!strcmp(fixup_prefix,"amend")) {
>
> Missing SP after "," (I won't repeat---please re-check the whole
> patch series before rerolling).

Apologies for this. I will take care of it.

>
> > +                     if (have_option_m)
> > +                             die(_("cannot combine -m with --fixup:%s"), fixup_message);
> > +                     else
> > +                             prepare_amend_commit(commit, &sb, &ctx);
>
> Hmph, why is -m so special?  Should we allow --fixup=amend:<cmd>
> with -F (or -c/-C for that matter), or are these other options
> caught at a lot higher layer already and we do not have to check
> them here?
>

yes, those options are caught earlier and give the error as below:
"Only one of -c/-C/-F/--fixup can be used."
and only `-m` is checked over here.

> >       if (also + only + all + interactive > 1)
> >               die(_("Only one of --include/--only/--all/--interactive/--patch can be used."));
> > +
> > +     if (fixup_message) {
> > +             /*
> > +              * As `amend` suboption contains only alpha
> > +              * character. So check if first non alpha
> > +              * character in fixup_message is ':'.
> > +              */
> > +             size_t len = get_alpha_len(fixup_message);
> > +             if (len && fixup_message[len] == ':') {
> > +                     fixup_message[len] = '\0';
> > +                     fixup_commit = fixup_message + ++len;
>
> It would be easier to follow to write it this way, no?
>
>                         fixup_message[len++] = '\0';
>                         fixup_commit = fixup_message + len;
>

I agree and will update it .

Thanks a lot for the reviews. I will do the fixes and update in the
next revision.

Thanks and Regards,
Charvi