Re: [PATCH v2 2/6] commit: add amend suboption to --fixup to create amend! commit
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 25, 2021, 21:00 UTC
- Message-ID
- <xmqqwnuvsw0d.fsf@gitster.g>
- In-Reply-To
- <20210225100855.25530-3-charvi077@gmail.com>
Charvi Mendiratta <charvi077@gmail.com> writes:
Show 11 quoted lines
> `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 ...
Show 5 quoted lines
> 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.
Show 9 quoted lines
> + /* > + * 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?
Show 6 quoted lines
> ... > + 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?
Show 13 quoted lines
> 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;
Show 10 quoted lines
> + 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;
> + }
> + }