Charvi Mendiratta <charvi077@xxxxxxxxx> writes:
> `git commit --fixup=amend:<commit>` will create an "amend!" commit.
> The resulting commit message subject will be "amend! ..." where
> "..." is the subject line of <commit> and the initial message
> body will be <commit>'s message. -m can be used to override the
> message body.
>
> The "amend!" commit when rebased with --autosquash will fixup the
> contents and replace the commit message of <commit> with the
> "amend!" commit's message body.
>
> 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.
> + /*
> + * 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?
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?
> ...
> + 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).
> + 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?
> 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;
> + if (starts_with("amend", fixup_message))
> + fixup_prefix = "amend";
> + else
> + die(_("unknown option: --fixup=%s:%s"), fixup_message, fixup_commit);
> + } else {
> + fixup_commit = fixup_message;
> + fixup_prefix = "fixup";
> + use_editor = 0;
> + }
> + }
