| Message ID | 20210225100855.25530-3-charvi077@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
Charvi Mendiratta <charvi077@gmail.com> 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; > + } > + }
On Fri, 26 Feb 2021 at 02:30, Junio C Hamano <gitster@pobox.com> wrote: > > Charvi Mendiratta <charvi077@gmail.com> writes: > > 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. > Okay, I will fix the above changes. > > + /* > > + * 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? > This is applied for more than one "amend!" or in other words, "amend!" commit chain. On the other hand we don't need to skip any subject if we --fixup another fixup! commit because the resulting commit message is just one liner. But yes if "fixup!" commit is combined with `--squash` then it comments the complete "fixup!" commit line by finding its length and increasing pointer. > 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? > I am not sure about this, because I think in the way you have suggested, we need to strip off the complete subject line instead of prefix. I am saying this because the commit message body of "amend!" commit contains the complete commit message of the commit we are fixing up. for example : $ git commit --fixup=amend:HEAD will create commit with log message : amend! subject of head subject of head body of head and again if we `--fixup:amend` the HEAD commit then : $ git commit --fixup=amend:HEAD (by default) will create commit with log message: amend! amend! subject of head amend! subject of head /* we need to comment this complete line */ subject of head body of head So, I am not sure about the other option to implement it ? > > ... > > + 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). Apologies for this. I will take care of it. > > > + 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? > yes, those options are caught earlier and give the error as below: "Only one of -c/-C/-F/--fixup can be used." and only `-m` is checked over 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; > I agree and will update it . Thanks a lot for the reviews. I will do the fixes and update in the next revision. Thanks and Regards, Charvi
> subject of head > body of head > > So, I am not sure about the other option to implement it ? Thaks, and OK. >> > + 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? > > yes, those options are caught earlier and give the error as below: > "Only one of -c/-C/-F/--fixup can be used." > and only `-m` is checked over here. And the reason why -m cannot be checked early is because we do not recognize which kind of "fixup" we are doing when "only one of -c/-C/-F/--fixup" check is made before this function is called? OK. I wonder if we can tell which kind of fixup we are doing much earlier, though. Then we could extend it to say "Only one of -c/-C/-F/-m/--fixup=amend:<commit> can be used", etc., and we do not have to have this "only -m is checked here, everything else is checked earlier" curiosity. But I do not know if such a change is necessarily an improvement. I guess a better "fix" would probably be to add a comment to this function where it only checks for "-m" and tell readers why -c/-C/-F do not have to be checked here. Thanks.
On Sat, 27 Feb 2021 at 01:02, Junio C Hamano <gitster@pobox.com> wrote: [...] > > yes, those options are caught earlier and give the error as below: > > "Only one of -c/-C/-F/--fixup can be used." > > and only `-m` is checked over here. > > And the reason why -m cannot be checked early is because we do not > recognize which kind of "fixup" we are doing when "only one of > -c/-C/-F/--fixup" check is made before this function is called? > > OK. I wonder if we can tell which kind of fixup we are doing much > earlier, though. Then we could extend it to say "Only one of > -c/-C/-F/-m/--fixup=amend:<commit> can be used", etc., and we do not > have to have this "only -m is checked here, everything else is > checked earlier" curiosity. But I do not know if such a change is > necessarily an improvement. I guess a better "fix" would probably > be to add a comment to this function where it only checks for "-m" > and tell readers why -c/-C/-F do not have to be checked here. > I agree, earlier I even asked myself why only `-m`, but this was the only option that was allowing to write/append commit message in ` --fixup`. And I agree to add a comment, to make it more clear. Thanks and Regards, Charvi
diff --git a/builtin/commit.c b/builtin/commit.c index 505fe60956..56ae15a762 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,24 @@ 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"; + + 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 +1177,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 +1201,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 +1258,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 +1557,12 @@ 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: please do not translate [amend:] + * Here "amend" is an option to the --fixup command + * line flag, that creates amend! 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 +1721,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. Inorder 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 | 89 +++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 80 insertions(+), 9 deletions(-)