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, 07:37 UTC
Message-ID: <CAPSFM5dm7vD8pszCG+JKNb7Hu6Zp48wKXaj9ayxe6yaf6swePA@mail.gmail.com>
In-Reply-To: <CAPig+cQAG3p6rhGHzBNGPUxMXQbbLZgiw-uUpizNiEWY9wm-3A@mail.gmail.com>

On Tue, 2 Mar 2021 at 03:45, Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 17 quoted lines

> > @@ -105,7 +105,8 @@ static const char *template_file;
> > +static int prepare_amend_commit(struct commit *commit, struct strbuf *sb,
> > +                                                                struct pretty_print_context *ctx) {
> > +       /*
> > +        * 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!"))
>
> 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.
> (Compare with the code in sequencer.c which checks for this sort of
> duplication but is tolerant of one or more spaces, not just a single
> space.)
>

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 ?

Show 13 quoted lines

> > +               format = "%b";
> > +       else
> > +               format = "%B";
>
> It's subjective and minor, but this could be expressed more compactly as:
>
>     const char *fmt = starts_with(...) ? "%b" : "%B";
>
> Also, no need to initialize `format` to NULL since it gets assigned in
> all code paths.
>
> Not worth a re-roll.
>

I agree, I will fix it.

Show 26 quoted lines

> > @@ -745,15 +761,33 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
> > +               if (!strcmp(fixup_prefix, "amend")) {
> > +                       if (have_option_m)
> > +                               die(_("cannot combine -m with --fixup:%s"), fixup_message);
> > +                       else
> > +                               prepare_amend_commit(commit, &sb, &ctx);
> > +               }
>
> This is minor, but the way this is written, the error case and the
> normal case appear to have the same significance, whereas, if you
> write it like this:
>
>     if (!strcmp(...)) {
>         if (have_option_m)
>             die(...);
>         prepare_amend_commit(...);
>     }
>
> then it's easier to see that you're checking for and getting an error
> case out of the way early, which allows the reader to concentrate
> without distraction on the normal case. As a minor benefit, you also
> get to eliminate an indentation level for the normal case, which could
> be important if more code is added to that case.
>
> Not worth a re-roll.
>

Thanks for pointing this out, I will remove the unnecessary `else`.