On Fri, 26 Feb 2021 at 02:30, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> Charvi Mendiratta <charvi077@xxxxxxxxx> 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
