| Message ID | 20210301084512.27170-3-charvi077@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | commit: Implementation of "amend!" commit | expand |
Charvi Mendiratta <charvi077@gmail.com> writes: > +static int prepare_amend_commit(struct commit *commit, struct strbuf *sb, > + struct pretty_print_context *ctx) { Why does this need to be overly indented? Are you using some funny tab width settings? In this project, a tab stop is 8-spaces wide. > + /* > + * Only `-m` commit message option is checked here, as > + * it supports `--fixup` to append the commit message. As it is OK to use "-m" with the plain vanilla "--fixup", an earlier check did not reject the combination, but now we look at what kind of fixup it is, and error out if it is "--fixup=amend:". OK. > + * The other commit message options `-c`/`-C`/`-F` are > + * incompatible with all the forms of `--fixup` and > + * have already errored out while parsing the `git commit` > + * options. > + */
On Mon, Mar 1, 2021 at 3:50 AM Charvi Mendiratta <charvi077@gmail.com> wrote: > `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. > > In order to prevent rebase from creating commits with an empty > message we refuse to create an "amend!" commit if commit message > body is empty. > > Signed-off-by: Charvi Mendiratta <charvi077@gmail.com> > --- > diff --git a/builtin/commit.c b/builtin/commit.c > @@ -105,7 +105,8 @@ static const char *template_file; > +static int prepare_amend_commit(struct commit *commit, struct strbuf *sb, > + struct pretty_print_context *ctx) { > + /* > + * 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!")) Is the content of the incoming strbuf created mechanically so that we know that there will only ever be one space between the two "amend!" literals? If not, then this starts_with() check feels fragile. (Compare with the code in sequencer.c which checks for this sort of duplication but is tolerant of one or more spaces, not just a single space.) > + format = "%b"; > + else > + format = "%B"; It's subjective and minor, but this could be expressed more compactly as: const char *fmt = starts_with(...) ? "%b" : "%B"; Also, no need to initialize `format` to NULL since it gets assigned in all code paths. Not worth a re-roll. > @@ -745,15 +761,33 @@ static int prepare_to_commit(const char *index_file, const char *prefix, > + if (!strcmp(fixup_prefix, "amend")) { > + if (have_option_m) > + die(_("cannot combine -m with --fixup:%s"), fixup_message); > + else > + prepare_amend_commit(commit, &sb, &ctx); > + } This is minor, but the way this is written, the error case and the normal case appear to have the same significance, whereas, if you write it like this: if (!strcmp(...)) { if (have_option_m) die(...); prepare_amend_commit(...); } then it's easier to see that you're checking for and getting an error case out of the way early, which allows the reader to concentrate without distraction on the normal case. As a minor benefit, you also get to eliminate an indentation level for the normal case, which could be important if more code is added to that case. Not worth a re-roll. > @@ -1227,6 +1267,28 @@ static int parse_and_validate_options(int argc, const char *argv[], > + 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; > + if (starts_with("amend", fixup_message)) > + fixup_prefix = "amend"; > + else > + die(_("unknown option: --fixup=%s:%s"), fixup_message, fixup_commit); I haven't read ahead in the series yet, but I presume you're making this code extra generic because you plan to support additional `fixup` options (such as `reword`), but I wonder if the cognitive overhead is warranted or you could get by with something simpler, such as: if (skip_prefix(msg, "amend:", &arg) || skip_prefix(msg, "reword:", &arg)) { ... } Also, am I misreading when I think that the use of starts_with() could be replaced with a simple strcmp() since you've already inserted a '\0' immediately after the final alphabetic character? Not necessarily worth a re-roll. > @@ -1663,6 +1729,19 @@ int cmd_commit(int argc, const char **argv, const char *prefix) > + if (message_is_empty(&body, cleanup_mode)) { > + rollback_index_files(); > + fprintf(stderr, _("Aborting commit due to empty commit message body.\n")); > + exit(1); I was wondering why you are capitalizing the error message (these days we don't) and using exit() instead of die(), but I see that you're mirroring existing practice in this function. Okay.
Eric Sunshine <sunshine@sunshineco.com> writes: >> + if (len && fixup_message[len] == ':') { >> + 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); > > I haven't read ahead in the series yet, but I presume you're making > this code extra generic because you plan to support additional `fixup` > options (such as `reword`), but I wonder if the cognitive overhead is > warranted or you could get by with something simpler, such as: > > if (skip_prefix(msg, "amend:", &arg) || > skip_prefix(msg, "reword:", &arg)) { > ... > } You still need to compute "len" because you'd want to tell between --fixup="HEAD^{/^area: string}" and --fixup=bogus:HEAD (the latter would want to say "no such variant 'bogus' for --fixup", but the colon in the former is not the end of the name of variant. So, skip_prefix() would not buy us much, I guess. But the use of starts_with() in the original patch is bogus, I think. fixup_message[] by the time the comparison is made is NULL terminated at where the colon was originally, so we should be doing !strcmp() to reject "--fixup=amendo:HEAD~2" with "no, 'amendo' is not a valid variant name for --fixup option". > Also, am I misreading when I think that the use of starts_with() could > be replaced with a simple strcmp() since you've already inserted a > '\0' immediately after the final alphabetic character? Correct.
On Mon, Mar 1, 2021 at 5:32 PM Junio C Hamano <gitster@pobox.com> wrote: > Eric Sunshine <sunshine@sunshineco.com> writes: > > if (skip_prefix(msg, "amend:", &arg) || > > skip_prefix(msg, "reword:", &arg)) { > > ... > > } > > You still need to compute "len" because you'd want to tell between > --fixup="HEAD^{/^area: string}" and --fixup=bogus:HEAD (the latter > would want to say "no such variant 'bogus' for --fixup", but the > colon in the former is not the end of the name of variant. I see what you mean. I vaguely recall quickly scanning over that earlier discussion about ":" being otherwise legitimate when embedded in the argument as you demonstrate, but didn't think about it when reading this code. Perhaps the comment which this code adds: * As `amend` suboption contains only alpha * character. So check if first non alpha * character in fixup_message is ':'. could be extended a bit to mention that briefly since, without it, the significance of "alpha-only characters followed by colon" is not immediately obvious.
On Tue, 2 Mar 2021 at 00:04, Junio C Hamano <gitster@pobox.com> wrote: > > Charvi Mendiratta <charvi077@gmail.com> writes: > > > +static int prepare_amend_commit(struct commit *commit, struct strbuf *sb, > > + struct pretty_print_context *ctx) { > > Why does this need to be overly indented? Are you using some funny > tab width settings? In this project, a tab stop is 8-spaces wide. > Oops, I myself didn't expect it after cross-check in the vscode editor. I will fix this.
On Tue, 2 Mar 2021 at 03:45, Eric Sunshine <sunshine@sunshineco.com> wrote: > > @@ -105,7 +105,8 @@ static const char *template_file; > > +static int prepare_amend_commit(struct commit *commit, struct strbuf *sb, > > + struct pretty_print_context *ctx) { > > + /* > > + * 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!")) > > Is the content of the incoming strbuf created mechanically so that we > know that there will only ever be one space between the two "amend!" > literals? If not, then this starts_with() check feels fragile. > (Compare with the code in sequencer.c which checks for this sort of > duplication but is tolerant of one or more spaces, not just a single > space.) > Yes, so for preparing each "amend!" commit we add prefix "amend! '' to the subject of the specific commit. And further if we amend the "amend!" commit then this above code is checked before creating a "amend! amend!" commit for the user. So I think maybe we don't need to check for multiple spaces ? > > + format = "%b"; > > + else > > + format = "%B"; > > It's subjective and minor, but this could be expressed more compactly as: > > const char *fmt = starts_with(...) ? "%b" : "%B"; > > Also, no need to initialize `format` to NULL since it gets assigned in > all code paths. > > Not worth a re-roll. > I agree, I will fix it. > > @@ -745,15 +761,33 @@ static int prepare_to_commit(const char *index_file, const char *prefix, > > + if (!strcmp(fixup_prefix, "amend")) { > > + if (have_option_m) > > + die(_("cannot combine -m with --fixup:%s"), fixup_message); > > + else > > + prepare_amend_commit(commit, &sb, &ctx); > > + } > > This is minor, but the way this is written, the error case and the > normal case appear to have the same significance, whereas, if you > write it like this: > > if (!strcmp(...)) { > if (have_option_m) > die(...); > prepare_amend_commit(...); > } > > then it's easier to see that you're checking for and getting an error > case out of the way early, which allows the reader to concentrate > without distraction on the normal case. As a minor benefit, you also > get to eliminate an indentation level for the normal case, which could > be important if more code is added to that case. > > Not worth a re-roll. > Thanks for pointing this out, I will remove the unnecessary `else`.
On Tue, 2 Mar 2021 at 04:02, Junio C Hamano <gitster@pobox.com> wrote: > > Eric Sunshine <sunshine@sunshineco.com> writes: > > >> + if (len && fixup_message[len] == ':') { > >> + 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); > > > > I haven't read ahead in the series yet, but I presume you're making > > this code extra generic because you plan to support additional `fixup` > > options (such as `reword`), but I wonder if the cognitive overhead is > > warranted or you could get by with something simpler, such as: > > > > if (skip_prefix(msg, "amend:", &arg) || > > skip_prefix(msg, "reword:", &arg)) { > > ... > > } > > You still need to compute "len" because you'd want to tell between > --fixup="HEAD^{/^area: string}" and --fixup=bogus:HEAD (the latter > would want to say "no such variant 'bogus' for --fixup", but the > colon in the former is not the end of the name of variant. > > So, skip_prefix() would not buy us much, I guess. > Yes, I also agree. > But the use of starts_with() in the original patch is bogus, I > think. fixup_message[] by the time the comparison is made is > NULL terminated at where the colon was originally, so we should be > doing !strcmp() to reject "--fixup=amendo:HEAD~2" with "no, 'amendo' > is not a valid variant name for --fixup option". > I am not sure about this because we used the starts_with() so that it can support the _any_ prefix of `amend` or `reword` i.e to make all below like combinations possible : --fixup=a:HEAD~2 --fixup=am:HEAD~2 So, I am not sure if we need to replace it with !strcmp and work for the specified prefix only ? > > Also, am I misreading when I think that the use of starts_with() could > > be replaced with a simple strcmp() since you've already inserted a > > '\0' immediately after the final alphabetic character? > > Correct. > Same reason for using the starts_with() applies for this part also after inserting the '\0'. So, I think we can keep starts_with() ?
On Tue, 2 Mar 2021 at 04:17, Eric Sunshine <sunshine@sunshineco.com> wrote: > > On Mon, Mar 1, 2021 at 5:32 PM Junio C Hamano <gitster@pobox.com> wrote: > > Eric Sunshine <sunshine@sunshineco.com> writes: > > > if (skip_prefix(msg, "amend:", &arg) || > > > skip_prefix(msg, "reword:", &arg)) { > > > ... > > > } > > > > You still need to compute "len" because you'd want to tell between > > --fixup="HEAD^{/^area: string}" and --fixup=bogus:HEAD (the latter > > would want to say "no such variant 'bogus' for --fixup", but the > > colon in the former is not the end of the name of variant. > > I see what you mean. I vaguely recall quickly scanning over that > earlier discussion about ":" being otherwise legitimate when embedded > in the argument as you demonstrate, but didn't think about it when > reading this code. Perhaps the comment which this code adds: > > * As `amend` suboption contains only alpha > * character. So check if first non alpha > * character in fixup_message is ':'. > > could be extended a bit to mention that briefly since, without it, the > significance of "alpha-only characters followed by colon" is not > immediately obvious. Okay, I will reword it and make it more clear regarding the main reason for choosing this way.
On Wed, Mar 3, 2021 at 2:37 AM Charvi Mendiratta <charvi077@gmail.com> wrote: > On Tue, 2 Mar 2021 at 03:45, Eric Sunshine <sunshine@sunshineco.com> wrote: > > > + if (starts_with(sb->buf, "amend! amend!")) > > > > Is the content of the incoming strbuf created mechanically so that we > > know that there will only ever be one space between the two "amend!" > > literals? If not, then this starts_with() check feels fragile. > > Yes, so for preparing each "amend!" commit we add prefix "amend! '' to > the subject of the specific commit. And further if we amend the > "amend!" commit then this above code is checked before creating a > "amend! amend!" commit for the user. So I think maybe we don't need to > check for multiple spaces ? Okay, if this is guaranteed to be created mechanically, then what you have should work, though it may be a good idea to add an in-code comment stating the reason it is okay to expect just the single space. The alternative would be to avoid having "amend! amend!" in the first place. I didn't trace through the code carefully so I don't know if it is possible, but would it make sense for the caller(s) to check before adding a second "amend!", thus eliminating the need to do so here? (Perhaps I'm misunderstanding, but the above code almost feels like a case of "whoops, we did something undesirable, so let's undo it.".)
On Wed, Mar 3, 2021 at 2:41 AM Charvi Mendiratta <charvi077@gmail.com> wrote: > On Tue, 2 Mar 2021 at 04:02, Junio C Hamano <gitster@pobox.com> wrote: > > But the use of starts_with() in the original patch is bogus, I > > think. fixup_message[] by the time the comparison is made is > > NULL terminated at where the colon was originally, so we should be > > doing !strcmp() to reject "--fixup=amendo:HEAD~2" with "no, 'amendo' > > is not a valid variant name for --fixup option". > > I am not sure about this because we used the starts_with() so that it can > support the _any_ prefix of `amend` or `reword` i.e to make all below > like combinations possible : > --fixup=a:HEAD~2 > --fixup=am:HEAD~2 > > So, I am not sure if we need to replace it with !strcmp and work for > the specified prefix only ? Hmm, I see. I didn't follow whatever discussion led to the decision to use this sort of prefix matching, but I have to wonder if it is a good idea. Was the idea that it behave similarly to sequencer commands in `git rebase --interactive` which are often abbreviated to a single letter? I personally would feel much more comfortable requiring a full-word match for `amend` and `reword` at initial implementation. That leaves the door open to later loosening it to do prefix-matching if enough people request such a feature, whereas starting with prefix-matching closes that door since we can never later tighten it to require full words. Anyhow, if the decision is to keep this behavior, then it almost certainly deserves an in-code comment explaining the sort of prefix-matching it's doing since it's otherwise too easy for readers to be fooled as Junio and I were by not noticing that you had reversed the arguments to starts_with().
Hi Eric, On Wed, 3 Mar 2021 at 13:16, Eric Sunshine <sunshine@sunshineco.com> wrote: > > On Wed, Mar 3, 2021 at 2:37 AM Charvi Mendiratta <charvi077@gmail.com> wrote: > > On Tue, 2 Mar 2021 at 03:45, Eric Sunshine <sunshine@sunshineco.com> wrote: > > > > + if (starts_with(sb->buf, "amend! amend!")) > > > > > > Is the content of the incoming strbuf created mechanically so that we > > > know that there will only ever be one space between the two "amend!" > > > literals? If not, then this starts_with() check feels fragile. > > > > Yes, so for preparing each "amend!" commit we add prefix "amend! '' to > > the subject of the specific commit. And further if we amend the > > "amend!" commit then this above code is checked before creating a > > "amend! amend!" commit for the user. So I think maybe we don't need to > > check for multiple spaces ? > > Okay, if this is guaranteed to be created mechanically, then what you > have should work, though it may be a good idea to add an in-code > comment stating the reason it is okay to expect just the single space. > > The alternative would be to avoid having "amend! amend!" in the first > place. Agree. I think we can do this... > I didn't trace through the code carefully so I don't know if it > is possible, but would it make sense for the caller(s) to check before > adding a second "amend!", thus eliminating the need to do so here? > (Perhaps I'm misunderstanding, but the above code almost feels like a > case of "whoops, we did something undesirable, so let's undo it.".) I looked into it and got another alternative, to extend the same prepare_amend_commit() function and replace the check condition of if (starts_with(sb->buf, "amend! amend!")) with the code as below : const char *buffer = get_commit_buffer(commit, NULL); const char *subject; find_commit_subject(buffer, &subject); if (starts_with(subject, "amend!")) const char *fmt = starts_with(subject, "amend!") ? "%b" : "%B"; format_commit_message(commit, fmt, sb, ctx); unuse_commit_buffer(commit, buffer); So, now it checks the commit subject here only. Otherwise as you have suggested above to check before adding a second "amend!", I think that can result in confusion as currently both "fixup!" and "amend!" commits (commit's subject) are prepared by same code and further for "amend!" commit as we write a commit message body also so we used prepare_amend_commit() to do that stuff.
On Wed, 3 Mar 2021 at 13:27, Eric Sunshine <sunshine@sunshineco.com> wrote: > [...] > > I am not sure about this because we used the starts_with() so that it can > > support the _any_ prefix of `amend` or `reword` i.e to make all below > > like combinations possible : > > --fixup=a:HEAD~2 > > --fixup=am:HEAD~2 > > > > So, I am not sure if we need to replace it with !strcmp and work for > > the specified prefix only ? > > Hmm, I see. I didn't follow whatever discussion led to the decision to > use this sort of prefix matching, but I have to wonder if it is a good > idea. Was the idea that it behave similarly to sequencer commands in > `git rebase --interactive` which are often abbreviated to a single > letter? Yes, this is also true. Also, same is discussed as here: https://lore.kernel.org/git/CAPSFM5cEnex1xaBy5ia_xNFDNzt5_Y=W-6TB9d9yW_AiPAKxDg@mail.gmail.com/ >I personally would feel much more comfortable requiring a > full-word match for `amend` and `reword` at initial implementation. > That leaves the door open to later loosening it to do prefix-matching > if enough people request such a feature, whereas starting with > prefix-matching closes that door since we can never later tighten it > to require full words. > > Anyhow, if the decision is to keep this behavior, then it almost > certainly deserves an in-code comment explaining the sort of > prefix-matching it's doing since it's otherwise too easy for readers > to be fooled as Junio and I were by not noticing that you had reversed > the arguments to starts_with(). Okay, I will add the comments to it .
Eric Sunshine <sunshine@sunshineco.com> writes: > Hmm, I see. I didn't follow whatever discussion led to the decision to > use this sort of prefix matching, but I have to wonder if it is a good > idea. Meaning --fixup=a:<commit> and --fixup=amend:<commit> do the same thing, until somebody invents --fixup=another:<commit> and makes the prefix 'a' no longer unique? I tend to agree that, especially with command line completion support with modern shells, such a prefix matching would not be necessary. > Was the idea that it behave similarly to sequencer commands in > `git rebase --interactive` which are often abbreviated to a single > letter? I personally would feel much more comfortable requiring a > full-word match for `amend` and `reword` at initial implementation. Me too.
On Thu, 4 Mar 2021 at 06:28, Junio C Hamano <gitster@pobox.com> wrote: > > Eric Sunshine <sunshine@sunshineco.com> writes: > > > Hmm, I see. I didn't follow whatever discussion led to the decision to > > use this sort of prefix matching, but I have to wonder if it is a good > > idea. > > Meaning --fixup=a:<commit> and --fixup=amend:<commit> do the same > thing, until somebody invents --fixup=another:<commit> and makes the > prefix 'a' no longer unique? I tend to agree that, especially with > command line completion support with modern shells, such a prefix > matching would not be necessary. > Okay, so for now I think (as suggested), let's remove it and directly use "!strcmp()" only instead of "starts_with()". I agree that if we keep prefix matching or not, it will not matter much. Thanks and Regards, Charvi
diff --git a/builtin/commit.c b/builtin/commit.c index 505fe60956..200ef83cc0 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -105,7 +105,8 @@ static const char *template_file; */ static const char *author_message, *author_message_buffer; static char *edit_message, *use_message; -static char *fixup_message, *squash_message; +static char *fixup_message, *fixup_commit, *squash_message; +static const char *fixup_prefix; static int all, also, interactive, patch_interactive, only, amend, signoff; static int edit_flag = -1; /* unspecified */ static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship; @@ -681,6 +682,21 @@ static void adjust_comment_line_char(const struct strbuf *sb) comment_line_char = *p; } +static int prepare_amend_commit(struct commit *commit, struct strbuf *sb, + struct pretty_print_context *ctx) { + /* + * 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"; + format_commit_message(commit, format, sb, ctx); + return 0; +} + static int prepare_to_commit(const char *index_file, const char *prefix, struct commit *current_head, struct wt_status *s, @@ -745,15 +761,33 @@ static int prepare_to_commit(const char *index_file, const char *prefix, } else if (fixup_message) { struct pretty_print_context ctx = {0}; struct commit *commit; - commit = lookup_commit_reference_by_name(fixup_message); + char *fmt = xstrfmt("%s! %%s\n\n", fixup_prefix); + commit = lookup_commit_reference_by_name(fixup_commit); if (!commit) - die(_("could not lookup commit %s"), fixup_message); + die(_("could not lookup commit %s"), fixup_commit); ctx.output_encoding = get_commit_output_encoding(); - format_commit_message(commit, "fixup! %s\n\n", - &sb, &ctx); - if (have_option_m) - strbuf_addbuf(&sb, &message); + format_commit_message(commit, fmt, &sb, &ctx); + free(fmt); hook_arg1 = "message"; + + /* + * Only `-m` commit message option is checked here, as + * it supports `--fixup` to append the commit message. + * + * The other commit message options `-c`/`-C`/`-F` are + * incompatible with all the forms of `--fixup` and + * have already errored out while parsing the `git commit` + * options. + */ + if (have_option_m && !strcmp(fixup_prefix, "fixup")) + strbuf_addbuf(&sb, &message); + + if (!strcmp(fixup_prefix, "amend")) { + if (have_option_m) + die(_("cannot combine -m with --fixup:%s"), fixup_message); + else + prepare_amend_commit(commit, &sb, &ctx); + } } else if (!stat(git_path_merge_msg(the_repository), &statbuf)) { size_t merge_msg_start; @@ -1152,6 +1186,12 @@ static void finalize_deferred_config(struct wt_status *s) s->ahead_behind_flags = AHEAD_BEHIND_FULL; } +/* returns the length of intial segment of alpha characters only */ +static size_t get_alpha_len(char *fixup_message) { + const char alphas[] = "abcdefghijklmnopqrstuvwxyz"; + return strspn(fixup_message, alphas); +} + static int parse_and_validate_options(int argc, const char *argv[], const struct option *options, const char * const usage[], @@ -1170,7 +1210,7 @@ static int parse_and_validate_options(int argc, const char *argv[], if (force_author && renew_authorship) die(_("Using both --reset-author and --author does not make sense")); - if (logfile || have_option_m || use_message || fixup_message) + if (logfile || have_option_m || use_message) use_editor = 0; if (0 <= edit_flag) use_editor = edit_flag; @@ -1227,6 +1267,28 @@ static int parse_and_validate_options(int argc, const char *argv[], 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; + 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; + } + } + cleanup_mode = get_cleanup_mode(cleanup_arg, use_editor); handle_untracked_files_arg(s); @@ -1504,7 +1566,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix) OPT_CALLBACK('m', "message", &message, N_("message"), N_("commit message"), opt_parse_m), OPT_STRING('c', "reedit-message", &edit_message, N_("commit"), N_("reuse and edit message from specified commit")), OPT_STRING('C', "reuse-message", &use_message, N_("commit"), N_("reuse message from specified commit")), - OPT_STRING(0, "fixup", &fixup_message, N_("commit"), N_("use autosquash formatted message to fixup specified commit")), + /* + * TRANSLATORS: Leave "[amend:]" as-is, and + * only translate <commit>. + */ + OPT_STRING(0, "fixup", &fixup_message, N_("[amend:]commit"), N_("use autosquash formatted message to fixup or amend specified commit")), OPT_STRING(0, "squash", &squash_message, N_("commit"), N_("use autosquash formatted message to squash specified commit")), OPT_BOOL(0, "reset-author", &renew_authorship, N_("the commit is authored by me now (used with -C/-c/--amend)")), OPT_BOOL('s', "signoff", &signoff, N_("add a Signed-off-by trailer")), @@ -1663,6 +1729,19 @@ int cmd_commit(int argc, const char **argv, const char *prefix) exit(1); } + if (fixup_message && starts_with(sb.buf, "amend! ") && + !allow_empty_message) { + struct strbuf body = STRBUF_INIT; + size_t len = subject_length(sb.buf); + strbuf_addstr(&body, sb.buf + len); + if (message_is_empty(&body, cleanup_mode)) { + rollback_index_files(); + fprintf(stderr, _("Aborting commit due to empty commit message body.\n")); + exit(1); + } + strbuf_release(&body); + } + if (amend) { const char *exclude_gpgsig[3] = { "gpgsig", "gpgsig-sha256", NULL }; extra = read_commit_extra_headers(current_head, exclude_gpgsig);
`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. In order to prevent rebase from creating commits with an empty message we refuse to create an "amend!" commit if commit message body is empty. Mentored-by: Christian Couder <chriscool@tuxfamily.org> Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk> Helped-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Charvi Mendiratta <charvi077@gmail.com> --- builtin/commit.c | 97 +++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 88 insertions(+), 9 deletions(-) -- 2.29.0.rc1