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

On Fri, 26 Feb 2021 at 02:30, Junio C Hamano [off-list ref] wrote:

Charvi Mendiratta [off-list ref] writes:

quoted

Inorder to prevent rebase from creating commits with an empty

In order to prevent ...

quoted

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.

quoted

+     /*
+      * 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 ?

quoted

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

quoted

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

quoted

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

quoted

      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