| Message ID | 20210310194306.32565-3-charvi077@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
On Wed, Mar 10, 2021 at 2:44 PM 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. > > 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. > [...] > Signed-off-by: Charvi Mendiratta <charvi077@gmail.com> > --- > diff --git a/builtin/commit.c b/builtin/commit.c > @@ -681,6 +683,23 @@ static void adjust_comment_line_char(const struct strbuf *sb) > +static int prepare_amend_commit(struct commit *commit, struct strbuf *sb, > + struct pretty_print_context *ctx) { > + > + const char *buffer, *subject, *fmt; Two style nits: * opening curly brace of function goes on its own line * we don't normally have a blank line at the top of the function body preceding the declarations So: static int prepare_amend_commit(...) { const char *buffer, *subject, *fmt; > + buffer = get_commit_buffer(commit, NULL); > + find_commit_subject(buffer, &subject); > + /* > + * If we amend the 'amend!' commit then we don't want to > + * duplicate the subject line. > + */ > + fmt = starts_with(subject, "amend!") ? "%b" : "%B"; > + format_commit_message(commit, fmt, sb, ctx); > + unuse_commit_buffer(commit, buffer); > + return 0; > +} What is the significance of this function's return value? At least in this patch, the single caller of this function ignores the return value, which suggests that the function need not return any value. Will a later patch add other possible return values to indicate an error or something? > @@ -745,15 +764,32 @@ static int prepare_to_commit(const char *index_file, const char *prefix, > + 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_commit); > ctx.output_encoding = get_commit_output_encoding(); > + format_commit_message(commit, fmt, &sb, &ctx); > + free(fmt); Nit: it would reduce the cognitive load slightly if `fmt` is prepared just before it is used rather than being prepared at the top of the block: fmt = xstrfmt("%s! %%s\n\n", fixup_prefix); format_commit_message(commit, fmt, &sb, &ctx); free(fmt); Subjective and not at all worth a re-roll. > @@ -1227,6 +1269,34 @@ static int parse_and_validate_options(int argc, const char *argv[], > + if (fixup_message) { > + /* > + * To check if fixup_message that contains ':' is a commit > + * reference for example: --fixup="HEAD^{/^area: string}" or > + * a suboption of `--fixup`. > + * > + * As `amend` suboption contains only alpha character. > + * So check if first non alpha character in fixup_message > + * is ':'. > + */ I have a tough time figuring out what this comment is trying to say, and I don't think I would have understood it if Junio had not already explained earlier in this thread why this code is as complex as it is (rather than using, say, skip_prefix()). Perhaps the entire comment can be replaced with this: Extract <option> (i.e. `amend`) from `--fixup=<option>:<commit>`, if present. To avoid being fooled by a legitimate ":" in <commit> (i.e. `--fixup="HEAD^{/^area: string}"`), <option> must be composed of only alphabetic characters. Not necessarily worth a re-roll. > + size_t len = get_alpha_len(fixup_message); > + if (len && fixup_message[len] == ':') { > + fixup_message[len++] = '\0'; > + fixup_commit = fixup_message + len; An alternate -- just about as compact and perhaps more idiomatic -- way to write all this without introducing the new get_alpha_len() function: char *p = fixup_mesage; while (isalpha(*p)) p++; if (p > fixup_message && *p == ':') { *p = '\0'; fixup_commit = p + 1; Subjective and not at all worth a re-roll. > + if (!strcmp("amend", fixup_message)) { > + fixup_prefix = "amend"; > + allow_empty = 1; > + } else { > + die(_("unknown option: --fixup=%s:%s"), fixup_message, fixup_commit); > + } > + } else { > + fixup_commit = fixup_message; > + fixup_prefix = "fixup"; > + use_editor = 0; > + } > + }
On Thu, 11 Mar 2021 at 11:55, Eric Sunshine <sunshine@sunshineco.com> wrote: > [...] > Two style nits: > > * opening curly brace of function goes on its own line > > * we don't normally have a blank line at the top of the function body > preceding the declarations > > So: > > static int prepare_amend_commit(...) > { > const char *buffer, *subject, *fmt; > Okay, I will fix it. > > + buffer = get_commit_buffer(commit, NULL); > > + find_commit_subject(buffer, &subject); > > + /* > > + * If we amend the 'amend!' commit then we don't want to > > + * duplicate the subject line. > > + */ > > + fmt = starts_with(subject, "amend!") ? "%b" : "%B"; > > + format_commit_message(commit, fmt, sb, ctx); > > + unuse_commit_buffer(commit, buffer); > > + return 0; > > +} > > What is the significance of this function's return value? At least in > this patch, the single caller of this function ignores the return > value, which suggests that the function need not return any value. > Will a later patch add other possible return values to indicate an > error or something? > No, it will not return another value later. I will remove it from here. > > @@ -745,15 +764,32 @@ static int prepare_to_commit(const char *index_file, const char *prefix, > > + 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_commit); > > ctx.output_encoding = get_commit_output_encoding(); > > + format_commit_message(commit, fmt, &sb, &ctx); > > + free(fmt); > > Nit: it would reduce the cognitive load slightly if `fmt` is prepared > just before it is used rather than being prepared at the top of the > block: > > fmt = xstrfmt("%s! %%s\n\n", fixup_prefix); > format_commit_message(commit, fmt, &sb, &ctx); > free(fmt); > > Subjective and not at all worth a re-roll. > Agree, will fix it. > > @@ -1227,6 +1269,34 @@ static int parse_and_validate_options(int argc, const char *argv[], > > + if (fixup_message) { > > + /* > > + * To check if fixup_message that contains ':' is a commit > > + * reference for example: --fixup="HEAD^{/^area: string}" or > > + * a suboption of `--fixup`. > > + * > > + * As `amend` suboption contains only alpha character. > > + * So check if first non alpha character in fixup_message > > + * is ':'. > > + */ > > I have a tough time figuring out what this comment is trying to say, > and I don't think I would have understood it if Junio had not already > explained earlier in this thread why this code is as complex as it is > (rather than using, say, skip_prefix()). Perhaps the entire comment > can be replaced with this: > I admit, this comment seems confusing... > Extract <option> (i.e. `amend`) from `--fixup=<option>:<commit>`, > if present. To avoid being fooled by a legitimate ":" in <commit> > (i.e. `--fixup="HEAD^{/^area: string}"`), <option> must be > composed of only alphabetic characters. > > Not necessarily worth a re-roll. > .. and I think we can reword it as suggested by Junio in patch[v4 3/6], as it seems more clear. > > + size_t len = get_alpha_len(fixup_message); > > + if (len && fixup_message[len] == ':') { > > + fixup_message[len++] = '\0'; > > + fixup_commit = fixup_message + len; > > An alternate -- just about as compact and perhaps more idiomatic -- > way to write all this without introducing the new get_alpha_len() > function: > > char *p = fixup_mesage; > while (isalpha(*p)) > p++; > if (p > fixup_message && *p == ':') { > *p = '\0'; > fixup_commit = p + 1; > > Subjective and not at all worth a re-roll. > Earlier we had discussed[1] keeping a separate helper function, so that it may re-use it later. But I agree above is easier to get and compact so I think maybe it will be ok, for this patch series to replace it with the above and remove the function. [1] https://lore.kernel.org/git/xmqqpn0xdse8.fsf@gitster.g/
On Thu, Mar 11, 2021 at 10:24 AM Charvi Mendiratta <charvi077@gmail.com> wrote: > On Thu, 11 Mar 2021 at 11:55, Eric Sunshine <sunshine@sunshineco.com> wrote: > > > + size_t len = get_alpha_len(fixup_message); > > > + if (len && fixup_message[len] == ':') { > > > + fixup_message[len++] = '\0'; > > > + fixup_commit = fixup_message + len; > > > > An alternate -- just about as compact and perhaps more idiomatic -- > > way to write all this without introducing the new get_alpha_len() > > function: > > > > char *p = fixup_mesage; > > while (isalpha(*p)) > > p++; > > if (p > fixup_message && *p == ':') { > > *p = '\0'; > > fixup_commit = p + 1; > > Earlier we had discussed[1] keeping a separate helper function, so > that it may re-use it later. But I agree above is easier to get and > compact so I think maybe it will be ok, for this patch series to > replace it with the above and remove the function. I don't have strong feelings one way or the other whether you should use a function or inline it as I showed above, and since our aim is to land this series rather than endlessly re-rolling it, let's not spend a lot of cycles worrying about it. The one thing that does bother me, however, is the name of the function, get_alpha_len(), which tells you (somewhat) literally what it does but doesn't convey to the reader its actual purpose (which is something we should strive for when naming functions and variables). In that previous discussion you referenced, Junio mentioned that a future sub-option might want to have punctuation in its name. If that ever comes about, then the name get_alpha_len() no longer makes sense and needs to be renamed so it doesn't become a lie. Giving the function a better name up front would not only help readers now to understand what is going on, but would also help down the road when or if punctuation (or numbers or whatnot) become valid in a sub-option name. suboption_length() is one possibility. With a slight semantic change, skip_suboption() or latch_suboption() are other possibilities. Or, if you were to open-code the loop as I did above, then you might have a function named is_suboption_char() and use that in place of isalpha(). So, if you do re-roll, I wouldn't mind seeing a better name for the function; but the rest is subjective and not worth spending time refining unless you actually feel that one style has a clear advantage over others.
On Thu, 11 Mar 2021 at 22:38, Eric Sunshine <sunshine@sunshineco.com> wrote: > > On Thu, Mar 11, 2021 at 10:24 AM Charvi Mendiratta <charvi077@gmail.com> wrote: > > On Thu, 11 Mar 2021 at 11:55, Eric Sunshine <sunshine@sunshineco.com> wrote: > > > > + size_t len = get_alpha_len(fixup_message); > > > > + if (len && fixup_message[len] == ':') { > > > > + fixup_message[len++] = '\0'; > > > > + fixup_commit = fixup_message + len; > > > > > > An alternate -- just about as compact and perhaps more idiomatic -- > > > way to write all this without introducing the new get_alpha_len() > > > function: > > > > > > char *p = fixup_mesage; > > > while (isalpha(*p)) > > > p++; > > > if (p > fixup_message && *p == ':') { > > > *p = '\0'; > > > fixup_commit = p + 1; > > > > Earlier we had discussed[1] keeping a separate helper function, so > > that it may re-use it later. But I agree above is easier to get and > > compact so I think maybe it will be ok, for this patch series to > > replace it with the above and remove the function. > > I don't have strong feelings one way or the other whether you should > use a function or inline it as I showed above, and since our aim is to > land this series rather than endlessly re-rolling it, let's not spend > a lot of cycles worrying about it. > > The one thing that does bother me, however, is the name of the > function, get_alpha_len(), which tells you (somewhat) literally what > it does but doesn't convey to the reader its actual purpose (which is > something we should strive for when naming functions and variables). > In that previous discussion you referenced, Junio mentioned that a > future sub-option might want to have punctuation in its name. If that > ever comes about, then the name get_alpha_len() no longer makes sense > and needs to be renamed so it doesn't become a lie. Giving the > function a better name up front would not only help readers now to > understand what is going on, but would also help down the road when or > if punctuation (or numbers or whatnot) become valid in a sub-option > name. suboption_length() is one possibility. With a slight semantic > change, skip_suboption() or latch_suboption() are other possibilities. > Or, if you were to open-code the loop as I did above, then you might > have a function named is_suboption_char() and use that in place of > isalpha(). > > So, if you do re-roll, I wouldn't mind seeing a better name for the > function; but the rest is subjective and not worth spending time > refining unless you actually feel that one style has a clear advantage > over others. Okay, I will rename the get_alpha_len() to skip_suboption(). Thanks and Regards, Charvi
Eric Sunshine <sunshine@sunshineco.com> writes: > The one thing that does bother me, however, is the name of the > function, get_alpha_len(), which tells you (somewhat) literally what > it does but doesn't convey to the reader its actual purpose (which is > something we should strive for when naming functions and variables). I actually think the helper function that is used as a building block the "subcommand parser" uses should be named more directly to represent what it does (i.e. look for a run of alphas) than what it means (i.e. look for a run of letters allowed in a subcommand name). IOW char *end = skip_alphas(ptr); if (*end == ':' && ptr != end) { /* * ptr..end could be a subcommand in * "--fixup=<subcommand>:"; see if it is a known one */ *end = '\0'; if (!strcmp(ptr, "amend")) ... do the amend thing ... else if (!strcmp(ptr, "reword")) ... do the reword thing ... else ... we do not know such a subcommand yet ... } else { /* assume it is --fixup=<command> form */ ... } conveys more information to readers than a variant where you replace "skip_alphas" with "skip_subcommand_chars" without losing any information. Yes, in different contexts, where a helpers are designed to be used by multiple callers that may not even be aware of each other, we do encourage naming them after what they do _means_. But in this codepath, I do not think it applies. Thanks.
On Sun, 14 Mar 2021 at 07:55, Junio C Hamano <gitster@pobox.com> wrote: > > Eric Sunshine <sunshine@sunshineco.com> writes: > > > The one thing that does bother me, however, is the name of the > > function, get_alpha_len(), which tells you (somewhat) literally what > > it does but doesn't convey to the reader its actual purpose (which is > > something we should strive for when naming functions and variables). > > I actually think the helper function that is used as a building > block the "subcommand parser" uses should be named more directly > to represent what it does (i.e. look for a run of alphas) than > what it means (i.e. look for a run of letters allowed in a > subcommand name). IOW > > char *end = skip_alphas(ptr); > if (*end == ':' && ptr != end) { > /* > * ptr..end could be a subcommand in > * "--fixup=<subcommand>:"; see if it is a known one > */ > *end = '\0'; > if (!strcmp(ptr, "amend")) > ... do the amend thing ... > else if (!strcmp(ptr, "reword")) > ... do the reword thing ... > else > ... we do not know such a subcommand yet ... > } else { > /* assume it is --fixup=<command> form */ > ... > } > > conveys more information to readers than a variant where you replace > "skip_alphas" with "skip_subcommand_chars" without losing any > information. > I thought to just rename get_alpha_len() to skip_alpha() that returns alpha length. But even removing the "len" variable and implementing as suggested above seems a better and clear alternative. I also agree to update it. Thanks for the suggestions. Thanks and Regards, Charvi
Charvi Mendiratta <charvi077@gmail.com> writes: > On Sun, 14 Mar 2021 at 07:55, Junio C Hamano <gitster@pobox.com> wrote: >> >> Eric Sunshine <sunshine@sunshineco.com> writes: >> >> > The one thing that does bother me, however, is the name of the >> > function, get_alpha_len(), which tells you (somewhat) literally what >> > it does but doesn't convey to the reader its actual purpose (which is >> > something we should strive for when naming functions and variables). >> >> I actually think the helper function that is used as a building >> block the "subcommand parser" uses should be named more directly >> to represent what it does (i.e. look for a run of alphas) than >> what it means (i.e. look for a run of letters allowed in a >> subcommand name). IOW >> >> char *end = skip_alphas(ptr); >> if (*end == ':' && ptr != end) { >> /* >> * ptr..end could be a subcommand in >> * "--fixup=<subcommand>:"; see if it is a known one >> */ >> *end = '\0'; >> if (!strcmp(ptr, "amend")) >> ... do the amend thing ... >> else if (!strcmp(ptr, "reword")) >> ... do the reword thing ... >> else >> ... we do not know such a subcommand yet ... >> } else { >> /* assume it is --fixup=<command> form */ >> ... >> } >> >> conveys more information to readers than a variant where you replace >> "skip_alphas" with "skip_subcommand_chars" without losing any >> information. >> > > I thought to just rename get_alpha_len() to skip_alpha() that returns > alpha length. But even removing the "len" variable and implementing as > suggested above seems a better and clear alternative. I also agree to > update it. > > Thanks for the suggestions. FWIW I am also fine with Eric's simpler "open code it right there" suggestion in this case. Just like the "skip alphas" suggestion, it makes the logic to parse subcommand name out isolated to a single place without asking readers to refer to the implementation of a helper, and it would be short enough. Thanks.
On Sun, Mar 14, 2021 at 6:43 PM Junio C Hamano <gitster@pobox.com> wrote: > FWIW I am also fine with Eric's simpler "open code it right there" > suggestion in this case. Just like the "skip alphas" suggestion, it > makes the logic to parse subcommand name out isolated to a single > place without asking readers to refer to the implementation of a > helper, and it would be short enough. Likewise. If you're going to re-roll anyhow, the open-coded: char *p = fixup_mesage; while (isalpha(*p)) p++; if (p > fixup_message && *p == ':') { *p = '\0'; fixup_commit = p + 1; would be perfectly fine with me too (or any simple variation on that theme). Whether or not it's worth re-rolling again, I leave up to you and Junio.
On Mon, 15 Mar 2021 at 04:37, Eric Sunshine <sunshine@sunshineco.com> wrote: > > On Sun, Mar 14, 2021 at 6:43 PM Junio C Hamano <gitster@pobox.com> wrote: > > FWIW I am also fine with Eric's simpler "open code it right there" > > suggestion in this case. Just like the "skip alphas" suggestion, it > > makes the logic to parse subcommand name out isolated to a single > > place without asking readers to refer to the implementation of a > > helper, and it would be short enough. > > Likewise. If you're going to re-roll anyhow, the open-coded: > > char *p = fixup_mesage; > while (isalpha(*p)) > p++; > if (p > fixup_message && *p == ':') { > *p = '\0'; > fixup_commit = p + 1; > > would be perfectly fine with me too (or any simple variation on that > theme). Whether or not it's worth re-rolling again, I leave up to you > and Junio. okay, I agree too. I have updated it in the re-roll. Thanks for all the detailed reviews and suggestions. Thanks and Regards, Charvi
On Mon, Mar 15, 2021 at 3:59 AM Charvi Mendiratta <charvi077@gmail.com> wrote: > On Mon, 15 Mar 2021 at 04:37, Eric Sunshine <sunshine@sunshineco.com> wrote: > > Likewise. If you're going to re-roll anyhow, the open-coded: > > would be perfectly fine with me too (or any simple variation on that > > theme). Whether or not it's worth re-rolling again, I leave up to you > > and Junio. > > okay, I agree too. I have updated it in the re-roll. > > Thanks for all the detailed reviews and suggestions. Thanks for patiently putting up with reviewers who sometimes have opposing or contradictory opinions and recommendations.
On Mon, 15 Mar 2021 at 13:46, Eric Sunshine <sunshine@sunshineco.com> wrote: [..] > Thanks for patiently putting up with reviewers who sometimes have > opposing or contradictory opinions and recommendations. It was more a learning path and helpful for me to proceed. I really appreciate all the guidance and suggestions received. Glad to get this merge! Thanks and regards, Charvi
diff --git a/builtin/commit.c b/builtin/commit.c index 505fe60956..05594fa8ab 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; @@ -357,7 +358,8 @@ static const char *prepare_index(const char **argv, const char *prefix, die(_("--pathspec-file-nul requires --pathspec-from-file")); } - if (!pathspec.nr && (also || (only && !amend && !allow_empty))) + if (!pathspec.nr && (also || (only && !allow_empty && + (!amend || (fixup_message && strcmp(fixup_prefix, "amend")))))) die(_("No paths with --include/--only does not make sense.")); if (read_cache_preload(&pathspec) < 0) @@ -681,6 +683,23 @@ 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) { + + const char *buffer, *subject, *fmt; + + buffer = get_commit_buffer(commit, NULL); + find_commit_subject(buffer, &subject); + /* + * If we amend the 'amend!' commit then we don't want to + * duplicate the subject line. + */ + fmt = starts_with(subject, "amend!") ? "%b" : "%B"; + format_commit_message(commit, fmt, sb, ctx); + unuse_commit_buffer(commit, buffer); + return 0; +} + static int prepare_to_commit(const char *index_file, const char *prefix, struct commit *current_head, struct wt_status *s, @@ -745,15 +764,32 @@ 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); + prepare_amend_commit(commit, &sb, &ctx); + } } else if (!stat(git_path_merge_msg(the_repository), &statbuf)) { size_t merge_msg_start; @@ -1152,6 +1188,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 +1212,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 +1269,34 @@ 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) { + /* + * To check if fixup_message that contains ':' is a commit + * reference for example: --fixup="HEAD^{/^area: string}" or + * a suboption of `--fixup`. + * + * 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 (!strcmp("amend", fixup_message)) { + fixup_prefix = "amend"; + allow_empty = 1; + } 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 +1574,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 +1737,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 = commit_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. 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> Helped-by: Eric Sunshine <sunshine@sunshineco.com> Signed-off-by: Charvi Mendiratta <charvi077@gmail.com> --- builtin/commit.c | 107 ++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 97 insertions(+), 10 deletions(-)