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

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

quoted

@@ -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 ?

quoted

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

quoted

@@ -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`.