Re: [PATCH v2 2/6] commit: add amend suboption to --fixup to create amend! commit
flat view
From: Junio C Hamano <hidden>
Date: 2021-02-25 21:05:22
Charvi Mendiratta [off-list ref] 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.
quoted hunk ↗ jump to hunk
+ /* + * 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 ",".
quoted hunk ↗ jump to hunk
+ 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).
quoted hunk ↗ jump to hunk
+ 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?
quoted hunk ↗ jump to hunk
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;
quoted hunk ↗ jump to hunk
+ 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; + } + }