| Message ID | 70a508ca0b2d837b311afefcc2b0ffb6cfbd34fb.1551867827.git.liu.denton@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cherry-pick/revert cleanup scissors fix | expand |
Hi Denton On 06/03/2019 10:30, Denton Liu wrote: > Fix a bug where the scissors line is placed after the Conflicts: > section, in the case where a merge conflict occurs and > commit.cleanup = scissors. Was that the case with cherry-pick and revert as well as merge, or were they missing the scissors line entirely as the subject line suggests? > Note that the removal of the if-else tower in git_sequencer_config may > appear to be a no-op refactor but it actually isn't. First of all, we > now accept "default" as a configuration option and also we die on an > invalid cleanup mode. Most importantly, though, if > commit.cleanup = scissors, the cleanup enum will be set to > COMMIT_MSG_CLEANUP_SCISSORS instead of COMMIT_MSG_CLEANUP_SPACE. This > allows us to append scissors to MERGE_MSG in the case of a conflict. I've got some comments about this change below > Signed-off-by: Denton Liu <liu.denton@gmail.com> > --- > Documentation/git-cherry-pick.txt | 7 +++ > Documentation/git-revert.txt | 7 +++ > builtin/merge.c | 13 ++--- > builtin/revert.c | 5 ++ > sequencer.c | 22 ++++---- > sequencer.h | 3 +- > t/t3507-cherry-pick-conflict.sh | 88 +++++++++++++++++++++++++++++++ > 7 files changed, 121 insertions(+), 24 deletions(-) > > diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt > index d35d771fc8..5c086d78c8 100644 > --- a/Documentation/git-cherry-pick.txt > +++ b/Documentation/git-cherry-pick.txt > @@ -57,6 +57,13 @@ OPTIONS > With this option, 'git cherry-pick' will let you edit the commit > message prior to committing. > > +--cleanup=<mode>:: > + This option determines how the commit message will be cleaned up before > + being passed on. See linkgit:git-commit[1] for more details. In > + addition, if the '<mode>' is given a value of `scissors`, scissors will > + be prepended to MERGE_MSG before being passed on in the case of a > + conflict. > + > -x:: > When recording the commit, append a line that says > "(cherry picked from commit ...)" to the original commit > diff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt > index 837707a8fd..1894010e60 100644 > --- a/Documentation/git-revert.txt > +++ b/Documentation/git-revert.txt > @@ -66,6 +66,13 @@ more details. > With this option, 'git revert' will not start the commit > message editor. > > +--cleanup=<mode>:: > + This option determines how the commit message will be cleaned up before > + being passed on. See linkgit:git-commit[1] for more details. In > + addition, if the '<mode>' is given a value of `scissors`, scissors will > + be prepended to MERGE_MSG before being passed on in the case of a > + conflict. > + > -n:: > --no-commit:: > Usually the command automatically creates some commits with > diff --git a/builtin/merge.c b/builtin/merge.c > index 92efc3d8fa..d4217ebcf5 100644 > --- a/builtin/merge.c > +++ b/builtin/merge.c > @@ -913,17 +913,10 @@ static int suggest_conflicts(void) > * We can't use cleanup_mode because if we're not using the editor, > * get_cleanup_mode will return COMMIT_MSG_CLEANUP_SPACE instead, even > * though the message is meant to be processed later by git-commit. > - * Thus, we will get the cleanup mode is returned we _are_ using an > - * editor. > + * Thus, we will get the cleanup mode which is returned when we _are_ using > + * an editor. > */ > - if (get_cleanup_mode(cleanup_arg, 1) == COMMIT_MSG_CLEANUP_SCISSORS) { > - fputc('\n', fp); > - wt_status_add_cut_line(fp); > - /* comments out the newline from append_conflicts_hint */ > - fputc(comment_line_char, fp); > - } > - > - append_conflicts_hint(&msgbuf); > + append_conflicts_hint(&msgbuf, get_cleanup_mode(cleanup_arg, 1)); > fputs(msgbuf.buf, fp); > strbuf_release(&msgbuf); > fclose(fp); > diff --git a/builtin/revert.c b/builtin/revert.c > index 9a66720cfc..fe18036be7 100644 > --- a/builtin/revert.c > +++ b/builtin/revert.c > @@ -95,11 +95,13 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts) > { > const char * const * usage_str = revert_or_cherry_pick_usage(opts); > const char *me = action_name(opts); > + const char *cleanup_arg = NULL; > int cmd = 0; > struct option base_options[] = { > OPT_CMDMODE(0, "quit", &cmd, N_("end revert or cherry-pick sequence"), 'q'), > OPT_CMDMODE(0, "continue", &cmd, N_("resume revert or cherry-pick sequence"), 'c'), > OPT_CMDMODE(0, "abort", &cmd, N_("cancel revert or cherry-pick sequence"), 'a'), > + OPT_STRING(0, "cleanup", &cleanup_arg, N_("default"), N_("how to strip spaces and #comments from message")), > OPT_BOOL('n', "no-commit", &opts->no_commit, N_("don't automatically commit")), > OPT_BOOL('e', "edit", &opts->edit, N_("edit the commit message")), > OPT_NOOP_NOARG('r', NULL), > @@ -136,6 +138,9 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts) > if (opts->keep_redundant_commits) > opts->allow_empty = 1; > > + if (cleanup_arg) > + opts->default_msg_cleanup = get_cleanup_mode(cleanup_arg, 1); > + > /* Check for incompatible command line arguments */ > if (cmd) { > char *this_operation; > diff --git a/sequencer.c b/sequencer.c > index 707e72fb39..85ad58555d 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -165,17 +165,7 @@ static int git_sequencer_config(const char *k, const char *v, void *cb) > if (status) > return status; > > - if (!strcmp(s, "verbatim")) > - opts->default_msg_cleanup = COMMIT_MSG_CLEANUP_NONE; > - else if (!strcmp(s, "whitespace")) > - opts->default_msg_cleanup = COMMIT_MSG_CLEANUP_SPACE; > - else if (!strcmp(s, "strip")) > - opts->default_msg_cleanup = COMMIT_MSG_CLEANUP_ALL; > - else if (!strcmp(s, "scissors")) > - opts->default_msg_cleanup = COMMIT_MSG_CLEANUP_SPACE; > - else > - warning(_("invalid commit message cleanup mode '%s'"), > - s); > + opts->default_msg_cleanup = get_cleanup_mode(s, 1); If we're rebasing then I'm not sure we want to pass 1 here, we won't be using an editor unless the user is rewording a commit message. Also I'm not clear about the implications of changing the cleanup mode for scissors - will whitespace and comments still be cleaned up? I'm not terribly happy with the idea that this will now die while reading the config file if there's on invalid cleanup option. If you really want to enforce a valid value then it would be better to return an error from get_cleanup_mode(). Best Wishes Phillip > > free((char *)s); > return status; > @@ -516,10 +506,16 @@ enum commit_msg_cleanup_mode get_cleanup_mode(const char *cleanup_arg, > die(_("Invalid cleanup mode %s"), cleanup_arg); > } > > -void append_conflicts_hint(struct strbuf *msgbuf) > +void append_conflicts_hint(struct strbuf *msgbuf, enum commit_msg_cleanup_mode cleanup_mode) > { > int i; > > + if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS) { > + strbuf_addch(msgbuf, '\n'); > + wt_status_append_cut_line(msgbuf); > + strbuf_addch(msgbuf, comment_line_char); > + } > + > strbuf_addch(msgbuf, '\n'); > strbuf_commented_addf(msgbuf, "Conflicts:\n"); > for (i = 0; i < active_nr;) { > @@ -586,7 +582,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next, > _(action_name(opts))); > > if (!clean) > - append_conflicts_hint(msgbuf); > + append_conflicts_hint(msgbuf, opts->default_msg_cleanup); > > return !clean; > } > diff --git a/sequencer.h b/sequencer.h > index 5690e0c27e..aa99503dd7 100644 > --- a/sequencer.h > +++ b/sequencer.h > @@ -91,7 +91,8 @@ int rearrange_squash(void); > extern const char sign_off_header[]; > > void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag); > -void append_conflicts_hint(struct strbuf *msgbuf); > +void append_conflicts_hint(struct strbuf *msgbuf, > + enum commit_msg_cleanup_mode cleanup_mode); > enum commit_msg_cleanup_mode get_cleanup_mode(const char *cleanup_arg, > int use_editor); > > diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh > index 74ff925526..c94e44dad0 100755 > --- a/t/t3507-cherry-pick-conflict.sh > +++ b/t/t3507-cherry-pick-conflict.sh > @@ -189,6 +189,48 @@ test_expect_success 'failed cherry-pick registers participants in index' ' > test_cmp expected actual > ' > > +test_expect_success \ > + 'cherry-pick conflict, ensure commit.cleanup = scissors places scissors line properly' ' > + pristine_detach initial && > + git config commit.cleanup scissors && > + cat <<-EOF >expected && > + picked > + > + # ------------------------ >8 ------------------------ > + # Do not modify or remove the line above. > + # Everything below it will be ignored. > + # > + # Conflicts: > + # foo > + EOF > + > + test_must_fail git cherry-pick picked && > + > + sed "s/[a-f0-9]\{40\}/objid/" .git/MERGE_MSG >actual && > + test_i18ncmp expected actual > +' > + > +test_expect_success \ > + 'cherry-pick conflict, ensure cleanup=scissors places scissors line properly' ' > + pristine_detach initial && > + git config --unset commit.cleanup && > + cat <<-EOF >expected && > + picked > + > + # ------------------------ >8 ------------------------ > + # Do not modify or remove the line above. > + # Everything below it will be ignored. > + # > + # Conflicts: > + # foo > + EOF > + > + test_must_fail git cherry-pick --cleanup=scissors picked && > + > + sed "s/[a-f0-9]\{40\}/objid/" .git/MERGE_MSG >actual && > + test_i18ncmp expected actual > +' > + > test_expect_success 'failed cherry-pick describes conflict in work tree' ' > pristine_detach initial && > cat <<-EOF >expected && > @@ -335,6 +377,52 @@ test_expect_success 'revert conflict, diff3 -m style' ' > test_cmp expected actual > ' > > +test_expect_success \ > + 'revert conflict, ensure commit.cleanup = scissors places scissors line properly' ' > + pristine_detach initial && > + git config commit.cleanup scissors && > + cat >expected <<-EOF && > + Revert "picked" > + > + This reverts commit objid. > + > + # ------------------------ >8 ------------------------ > + # Do not modify or remove the line above. > + # Everything below it will be ignored. > + # > + # Conflicts: > + # foo > + EOF > + > + test_must_fail git revert picked && > + > + sed "s/[a-f0-9]\{40\}/objid/" .git/MERGE_MSG >actual && > + test_i18ncmp expected actual > +' > + > +test_expect_success \ > + 'revert conflict, ensure cleanup=scissors places scissors line properly' ' > + pristine_detach initial && > + git config --unset commit.cleanup && > + cat >expected <<-EOF && > + Revert "picked" > + > + This reverts commit objid. > + > + # ------------------------ >8 ------------------------ > + # Do not modify or remove the line above. > + # Everything below it will be ignored. > + # > + # Conflicts: > + # foo > + EOF > + > + test_must_fail git revert --cleanup=scissors picked && > + > + sed "s/[a-f0-9]\{40\}/objid/" .git/MERGE_MSG >actual && > + test_i18ncmp expected actual > +' > + > test_expect_success 'failed cherry-pick does not forget -s' ' > pristine_detach initial && > test_must_fail git cherry-pick -s picked && >
Hi Phillip, On Wed, Mar 06, 2019 at 04:29:20PM +0000, Phillip Wood wrote: > Hi Denton > > On 06/03/2019 10:30, Denton Liu wrote: > >Fix a bug where the scissors line is placed after the Conflicts: > >section, in the case where a merge conflict occurs and > >commit.cleanup = scissors. > > Was that the case with cherry-pick and revert as well as merge, or were they > missing the scissors line entirely as the subject line suggests? > > >Note that the removal of the if-else tower in git_sequencer_config may > >appear to be a no-op refactor but it actually isn't. First of all, we > >now accept "default" as a configuration option and also we die on an > >invalid cleanup mode. Most importantly, though, if > >commit.cleanup = scissors, the cleanup enum will be set to > >COMMIT_MSG_CLEANUP_SCISSORS instead of COMMIT_MSG_CLEANUP_SPACE. This > >allows us to append scissors to MERGE_MSG in the case of a conflict. > > I've got some comments about this change below > > >Signed-off-by: Denton Liu <liu.denton@gmail.com> > >--- > > Documentation/git-cherry-pick.txt | 7 +++ > > Documentation/git-revert.txt | 7 +++ > > builtin/merge.c | 13 ++--- > > builtin/revert.c | 5 ++ > > sequencer.c | 22 ++++---- > > sequencer.h | 3 +- > > t/t3507-cherry-pick-conflict.sh | 88 +++++++++++++++++++++++++++++++ > > 7 files changed, 121 insertions(+), 24 deletions(-) > > > >diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt > >index d35d771fc8..5c086d78c8 100644 > >--- a/Documentation/git-cherry-pick.txt > >+++ b/Documentation/git-cherry-pick.txt > >@@ -57,6 +57,13 @@ OPTIONS > > With this option, 'git cherry-pick' will let you edit the commit > > message prior to committing. > >+--cleanup=<mode>:: > >+ This option determines how the commit message will be cleaned up before > >+ being passed on. See linkgit:git-commit[1] for more details. In > >+ addition, if the '<mode>' is given a value of `scissors`, scissors will > >+ be prepended to MERGE_MSG before being passed on in the case of a > >+ conflict. > >+ > > -x:: > > When recording the commit, append a line that says > > "(cherry picked from commit ...)" to the original commit > >diff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt > >index 837707a8fd..1894010e60 100644 > >--- a/Documentation/git-revert.txt > >+++ b/Documentation/git-revert.txt > >@@ -66,6 +66,13 @@ more details. > > With this option, 'git revert' will not start the commit > > message editor. > >+--cleanup=<mode>:: > >+ This option determines how the commit message will be cleaned up before > >+ being passed on. See linkgit:git-commit[1] for more details. In > >+ addition, if the '<mode>' is given a value of `scissors`, scissors will > >+ be prepended to MERGE_MSG before being passed on in the case of a > >+ conflict. > >+ > > -n:: > > --no-commit:: > > Usually the command automatically creates some commits with > >diff --git a/builtin/merge.c b/builtin/merge.c > >index 92efc3d8fa..d4217ebcf5 100644 > >--- a/builtin/merge.c > >+++ b/builtin/merge.c > >@@ -913,17 +913,10 @@ static int suggest_conflicts(void) > > * We can't use cleanup_mode because if we're not using the editor, > > * get_cleanup_mode will return COMMIT_MSG_CLEANUP_SPACE instead, even > > * though the message is meant to be processed later by git-commit. > >- * Thus, we will get the cleanup mode is returned we _are_ using an > >- * editor. > >+ * Thus, we will get the cleanup mode which is returned when we _are_ using > >+ * an editor. > > */ > >- if (get_cleanup_mode(cleanup_arg, 1) == COMMIT_MSG_CLEANUP_SCISSORS) { > >- fputc('\n', fp); > >- wt_status_add_cut_line(fp); > >- /* comments out the newline from append_conflicts_hint */ > >- fputc(comment_line_char, fp); > >- } > >- > >- append_conflicts_hint(&msgbuf); > >+ append_conflicts_hint(&msgbuf, get_cleanup_mode(cleanup_arg, 1)); > > fputs(msgbuf.buf, fp); > > strbuf_release(&msgbuf); > > fclose(fp); > >diff --git a/builtin/revert.c b/builtin/revert.c > >index 9a66720cfc..fe18036be7 100644 > >--- a/builtin/revert.c > >+++ b/builtin/revert.c > >@@ -95,11 +95,13 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts) > > { > > const char * const * usage_str = revert_or_cherry_pick_usage(opts); > > const char *me = action_name(opts); > >+ const char *cleanup_arg = NULL; > > int cmd = 0; > > struct option base_options[] = { > > OPT_CMDMODE(0, "quit", &cmd, N_("end revert or cherry-pick sequence"), 'q'), > > OPT_CMDMODE(0, "continue", &cmd, N_("resume revert or cherry-pick sequence"), 'c'), > > OPT_CMDMODE(0, "abort", &cmd, N_("cancel revert or cherry-pick sequence"), 'a'), > >+ OPT_STRING(0, "cleanup", &cleanup_arg, N_("default"), N_("how to strip spaces and #comments from message")), > > OPT_BOOL('n', "no-commit", &opts->no_commit, N_("don't automatically commit")), > > OPT_BOOL('e', "edit", &opts->edit, N_("edit the commit message")), > > OPT_NOOP_NOARG('r', NULL), > >@@ -136,6 +138,9 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts) > > if (opts->keep_redundant_commits) > > opts->allow_empty = 1; > >+ if (cleanup_arg) > >+ opts->default_msg_cleanup = get_cleanup_mode(cleanup_arg, 1); > >+ > > /* Check for incompatible command line arguments */ > > if (cmd) { > > char *this_operation; > >diff --git a/sequencer.c b/sequencer.c > >index 707e72fb39..85ad58555d 100644 > >--- a/sequencer.c > >+++ b/sequencer.c > >@@ -165,17 +165,7 @@ static int git_sequencer_config(const char *k, const char *v, void *cb) > > if (status) > > return status; > >- if (!strcmp(s, "verbatim")) > >- opts->default_msg_cleanup = COMMIT_MSG_CLEANUP_NONE; > >- else if (!strcmp(s, "whitespace")) > >- opts->default_msg_cleanup = COMMIT_MSG_CLEANUP_SPACE; > >- else if (!strcmp(s, "strip")) > >- opts->default_msg_cleanup = COMMIT_MSG_CLEANUP_ALL; > >- else if (!strcmp(s, "scissors")) > >- opts->default_msg_cleanup = COMMIT_MSG_CLEANUP_SPACE; > >- else > >- warning(_("invalid commit message cleanup mode '%s'"), > >- s); > >+ opts->default_msg_cleanup = get_cleanup_mode(s, 1); > > If we're rebasing then I'm not sure we want to pass 1 here, we won't be > using an editor unless the user is rewording a commit message. Also I'm not > clear about the implications of changing the cleanup mode for scissors - > will whitespace and comments still be cleaned up? I believe that rebase -i should be safe with the change but to be sure, I'll change the line to read: opts->default_msg_cleanup = get_cleanup_mode(s, !is_rebase_i(opts)); (Note to self, this will require a change in rebase--helper.c too.) > I'm not terribly happy with the idea that this will now die while reading > the config file if there's on invalid cleanup option. If you really want to > enforce a valid value then it would be better to return an error from > get_cleanup_mode(). The reason why I made it die was because I wanted to reuse that function as much as possible and it already happened to be implemented that way. However, there is prescedent in the code for dieing on invalid configuration options, namely in date.c: if (mode->type == DATE_STRFTIME) { if (!skip_prefix(p, ":", &p)) die("date format missing colon separator: %s", format); mode->strftime_fmt = xstrdup(p); } else if (*p) die("unknown date format %s", format); That being said, I'm pretty ambivalent between making it a warning and a die. If you have any strong opinions on this, I can change it to a warning. Thanks, Denton > > Best Wishes > > Phillip > > > free((char *)s); > > return status; > >@@ -516,10 +506,16 @@ enum commit_msg_cleanup_mode get_cleanup_mode(const char *cleanup_arg, > > die(_("Invalid cleanup mode %s"), cleanup_arg); > > } > >-void append_conflicts_hint(struct strbuf *msgbuf) > >+void append_conflicts_hint(struct strbuf *msgbuf, enum commit_msg_cleanup_mode cleanup_mode) > > { > > int i; > >+ if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS) { > >+ strbuf_addch(msgbuf, '\n'); > >+ wt_status_append_cut_line(msgbuf); > >+ strbuf_addch(msgbuf, comment_line_char); > >+ } > >+ > > strbuf_addch(msgbuf, '\n'); > > strbuf_commented_addf(msgbuf, "Conflicts:\n"); > > for (i = 0; i < active_nr;) { > >@@ -586,7 +582,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next, > > _(action_name(opts))); > > if (!clean) > >- append_conflicts_hint(msgbuf); > >+ append_conflicts_hint(msgbuf, opts->default_msg_cleanup); > > return !clean; > > } > >diff --git a/sequencer.h b/sequencer.h > >index 5690e0c27e..aa99503dd7 100644 > >--- a/sequencer.h > >+++ b/sequencer.h > >@@ -91,7 +91,8 @@ int rearrange_squash(void); > > extern const char sign_off_header[]; > > void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag); > >-void append_conflicts_hint(struct strbuf *msgbuf); > >+void append_conflicts_hint(struct strbuf *msgbuf, > >+ enum commit_msg_cleanup_mode cleanup_mode); > > enum commit_msg_cleanup_mode get_cleanup_mode(const char *cleanup_arg, > > int use_editor); > >diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh > >index 74ff925526..c94e44dad0 100755 > >--- a/t/t3507-cherry-pick-conflict.sh > >+++ b/t/t3507-cherry-pick-conflict.sh > >@@ -189,6 +189,48 @@ test_expect_success 'failed cherry-pick registers participants in index' ' > > test_cmp expected actual > > ' > >+test_expect_success \ > >+ 'cherry-pick conflict, ensure commit.cleanup = scissors places scissors line properly' ' > >+ pristine_detach initial && > >+ git config commit.cleanup scissors && > >+ cat <<-EOF >expected && > >+ picked > >+ > >+ # ------------------------ >8 ------------------------ > >+ # Do not modify or remove the line above. > >+ # Everything below it will be ignored. > >+ # > >+ # Conflicts: > >+ # foo > >+ EOF > >+ > >+ test_must_fail git cherry-pick picked && > >+ > >+ sed "s/[a-f0-9]\{40\}/objid/" .git/MERGE_MSG >actual && > >+ test_i18ncmp expected actual > >+' > >+ > >+test_expect_success \ > >+ 'cherry-pick conflict, ensure cleanup=scissors places scissors line properly' ' > >+ pristine_detach initial && > >+ git config --unset commit.cleanup && > >+ cat <<-EOF >expected && > >+ picked > >+ > >+ # ------------------------ >8 ------------------------ > >+ # Do not modify or remove the line above. > >+ # Everything below it will be ignored. > >+ # > >+ # Conflicts: > >+ # foo > >+ EOF > >+ > >+ test_must_fail git cherry-pick --cleanup=scissors picked && > >+ > >+ sed "s/[a-f0-9]\{40\}/objid/" .git/MERGE_MSG >actual && > >+ test_i18ncmp expected actual > >+' > >+ > > test_expect_success 'failed cherry-pick describes conflict in work tree' ' > > pristine_detach initial && > > cat <<-EOF >expected && > >@@ -335,6 +377,52 @@ test_expect_success 'revert conflict, diff3 -m style' ' > > test_cmp expected actual > > ' > >+test_expect_success \ > >+ 'revert conflict, ensure commit.cleanup = scissors places scissors line properly' ' > >+ pristine_detach initial && > >+ git config commit.cleanup scissors && > >+ cat >expected <<-EOF && > >+ Revert "picked" > >+ > >+ This reverts commit objid. > >+ > >+ # ------------------------ >8 ------------------------ > >+ # Do not modify or remove the line above. > >+ # Everything below it will be ignored. > >+ # > >+ # Conflicts: > >+ # foo > >+ EOF > >+ > >+ test_must_fail git revert picked && > >+ > >+ sed "s/[a-f0-9]\{40\}/objid/" .git/MERGE_MSG >actual && > >+ test_i18ncmp expected actual > >+' > >+ > >+test_expect_success \ > >+ 'revert conflict, ensure cleanup=scissors places scissors line properly' ' > >+ pristine_detach initial && > >+ git config --unset commit.cleanup && > >+ cat >expected <<-EOF && > >+ Revert "picked" > >+ > >+ This reverts commit objid. > >+ > >+ # ------------------------ >8 ------------------------ > >+ # Do not modify or remove the line above. > >+ # Everything below it will be ignored. > >+ # > >+ # Conflicts: > >+ # foo > >+ EOF > >+ > >+ test_must_fail git revert --cleanup=scissors picked && > >+ > >+ sed "s/[a-f0-9]\{40\}/objid/" .git/MERGE_MSG >actual && > >+ test_i18ncmp expected actual > >+' > >+ > > test_expect_success 'failed cherry-pick does not forget -s' ' > > pristine_detach initial && > > test_must_fail git cherry-pick -s picked && > >
Hi Phillip, sorry I forgot to address one of your replies. On Wed, Mar 06, 2019 at 04:29:20PM +0000, Phillip Wood wrote: > Hi Denton > > On 06/03/2019 10:30, Denton Liu wrote: > >Fix a bug where the scissors line is placed after the Conflicts: > >section, in the case where a merge conflict occurs and > >commit.cleanup = scissors. > > Was that the case with cherry-pick and revert as well as merge, or were they > missing the scissors line entirely as the subject line suggests? The bug is that git-{cherry-pick,revert,merge} all place the conflicts hint into MERGE_MSG, which then gets read by git-commit which will place a scissors line under all that stuff. The fix is to make git-{cherry-pick,revert,merge} put the scissors line before the conflicts hint, so the scissors are actually placed in that command instead of in git-commit. Is the commit message still unclear or does it make sense now? Thanks, Denton
diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt index d35d771fc8..5c086d78c8 100644 --- a/Documentation/git-cherry-pick.txt +++ b/Documentation/git-cherry-pick.txt @@ -57,6 +57,13 @@ OPTIONS With this option, 'git cherry-pick' will let you edit the commit message prior to committing. +--cleanup=<mode>:: + This option determines how the commit message will be cleaned up before + being passed on. See linkgit:git-commit[1] for more details. In + addition, if the '<mode>' is given a value of `scissors`, scissors will + be prepended to MERGE_MSG before being passed on in the case of a + conflict. + -x:: When recording the commit, append a line that says "(cherry picked from commit ...)" to the original commit diff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt index 837707a8fd..1894010e60 100644 --- a/Documentation/git-revert.txt +++ b/Documentation/git-revert.txt @@ -66,6 +66,13 @@ more details. With this option, 'git revert' will not start the commit message editor. +--cleanup=<mode>:: + This option determines how the commit message will be cleaned up before + being passed on. See linkgit:git-commit[1] for more details. In + addition, if the '<mode>' is given a value of `scissors`, scissors will + be prepended to MERGE_MSG before being passed on in the case of a + conflict. + -n:: --no-commit:: Usually the command automatically creates some commits with diff --git a/builtin/merge.c b/builtin/merge.c index 92efc3d8fa..d4217ebcf5 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -913,17 +913,10 @@ static int suggest_conflicts(void) * We can't use cleanup_mode because if we're not using the editor, * get_cleanup_mode will return COMMIT_MSG_CLEANUP_SPACE instead, even * though the message is meant to be processed later by git-commit. - * Thus, we will get the cleanup mode is returned we _are_ using an - * editor. + * Thus, we will get the cleanup mode which is returned when we _are_ using + * an editor. */ - if (get_cleanup_mode(cleanup_arg, 1) == COMMIT_MSG_CLEANUP_SCISSORS) { - fputc('\n', fp); - wt_status_add_cut_line(fp); - /* comments out the newline from append_conflicts_hint */ - fputc(comment_line_char, fp); - } - - append_conflicts_hint(&msgbuf); + append_conflicts_hint(&msgbuf, get_cleanup_mode(cleanup_arg, 1)); fputs(msgbuf.buf, fp); strbuf_release(&msgbuf); fclose(fp); diff --git a/builtin/revert.c b/builtin/revert.c index 9a66720cfc..fe18036be7 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -95,11 +95,13 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts) { const char * const * usage_str = revert_or_cherry_pick_usage(opts); const char *me = action_name(opts); + const char *cleanup_arg = NULL; int cmd = 0; struct option base_options[] = { OPT_CMDMODE(0, "quit", &cmd, N_("end revert or cherry-pick sequence"), 'q'), OPT_CMDMODE(0, "continue", &cmd, N_("resume revert or cherry-pick sequence"), 'c'), OPT_CMDMODE(0, "abort", &cmd, N_("cancel revert or cherry-pick sequence"), 'a'), + OPT_STRING(0, "cleanup", &cleanup_arg, N_("default"), N_("how to strip spaces and #comments from message")), OPT_BOOL('n', "no-commit", &opts->no_commit, N_("don't automatically commit")), OPT_BOOL('e', "edit", &opts->edit, N_("edit the commit message")), OPT_NOOP_NOARG('r', NULL), @@ -136,6 +138,9 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts) if (opts->keep_redundant_commits) opts->allow_empty = 1; + if (cleanup_arg) + opts->default_msg_cleanup = get_cleanup_mode(cleanup_arg, 1); + /* Check for incompatible command line arguments */ if (cmd) { char *this_operation; diff --git a/sequencer.c b/sequencer.c index 707e72fb39..85ad58555d 100644 --- a/sequencer.c +++ b/sequencer.c @@ -165,17 +165,7 @@ static int git_sequencer_config(const char *k, const char *v, void *cb) if (status) return status; - if (!strcmp(s, "verbatim")) - opts->default_msg_cleanup = COMMIT_MSG_CLEANUP_NONE; - else if (!strcmp(s, "whitespace")) - opts->default_msg_cleanup = COMMIT_MSG_CLEANUP_SPACE; - else if (!strcmp(s, "strip")) - opts->default_msg_cleanup = COMMIT_MSG_CLEANUP_ALL; - else if (!strcmp(s, "scissors")) - opts->default_msg_cleanup = COMMIT_MSG_CLEANUP_SPACE; - else - warning(_("invalid commit message cleanup mode '%s'"), - s); + opts->default_msg_cleanup = get_cleanup_mode(s, 1); free((char *)s); return status; @@ -516,10 +506,16 @@ enum commit_msg_cleanup_mode get_cleanup_mode(const char *cleanup_arg, die(_("Invalid cleanup mode %s"), cleanup_arg); } -void append_conflicts_hint(struct strbuf *msgbuf) +void append_conflicts_hint(struct strbuf *msgbuf, enum commit_msg_cleanup_mode cleanup_mode) { int i; + if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS) { + strbuf_addch(msgbuf, '\n'); + wt_status_append_cut_line(msgbuf); + strbuf_addch(msgbuf, comment_line_char); + } + strbuf_addch(msgbuf, '\n'); strbuf_commented_addf(msgbuf, "Conflicts:\n"); for (i = 0; i < active_nr;) { @@ -586,7 +582,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next, _(action_name(opts))); if (!clean) - append_conflicts_hint(msgbuf); + append_conflicts_hint(msgbuf, opts->default_msg_cleanup); return !clean; } diff --git a/sequencer.h b/sequencer.h index 5690e0c27e..aa99503dd7 100644 --- a/sequencer.h +++ b/sequencer.h @@ -91,7 +91,8 @@ int rearrange_squash(void); extern const char sign_off_header[]; void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag); -void append_conflicts_hint(struct strbuf *msgbuf); +void append_conflicts_hint(struct strbuf *msgbuf, + enum commit_msg_cleanup_mode cleanup_mode); enum commit_msg_cleanup_mode get_cleanup_mode(const char *cleanup_arg, int use_editor); diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh index 74ff925526..c94e44dad0 100755 --- a/t/t3507-cherry-pick-conflict.sh +++ b/t/t3507-cherry-pick-conflict.sh @@ -189,6 +189,48 @@ test_expect_success 'failed cherry-pick registers participants in index' ' test_cmp expected actual ' +test_expect_success \ + 'cherry-pick conflict, ensure commit.cleanup = scissors places scissors line properly' ' + pristine_detach initial && + git config commit.cleanup scissors && + cat <<-EOF >expected && + picked + + # ------------------------ >8 ------------------------ + # Do not modify or remove the line above. + # Everything below it will be ignored. + # + # Conflicts: + # foo + EOF + + test_must_fail git cherry-pick picked && + + sed "s/[a-f0-9]\{40\}/objid/" .git/MERGE_MSG >actual && + test_i18ncmp expected actual +' + +test_expect_success \ + 'cherry-pick conflict, ensure cleanup=scissors places scissors line properly' ' + pristine_detach initial && + git config --unset commit.cleanup && + cat <<-EOF >expected && + picked + + # ------------------------ >8 ------------------------ + # Do not modify or remove the line above. + # Everything below it will be ignored. + # + # Conflicts: + # foo + EOF + + test_must_fail git cherry-pick --cleanup=scissors picked && + + sed "s/[a-f0-9]\{40\}/objid/" .git/MERGE_MSG >actual && + test_i18ncmp expected actual +' + test_expect_success 'failed cherry-pick describes conflict in work tree' ' pristine_detach initial && cat <<-EOF >expected && @@ -335,6 +377,52 @@ test_expect_success 'revert conflict, diff3 -m style' ' test_cmp expected actual ' +test_expect_success \ + 'revert conflict, ensure commit.cleanup = scissors places scissors line properly' ' + pristine_detach initial && + git config commit.cleanup scissors && + cat >expected <<-EOF && + Revert "picked" + + This reverts commit objid. + + # ------------------------ >8 ------------------------ + # Do not modify or remove the line above. + # Everything below it will be ignored. + # + # Conflicts: + # foo + EOF + + test_must_fail git revert picked && + + sed "s/[a-f0-9]\{40\}/objid/" .git/MERGE_MSG >actual && + test_i18ncmp expected actual +' + +test_expect_success \ + 'revert conflict, ensure cleanup=scissors places scissors line properly' ' + pristine_detach initial && + git config --unset commit.cleanup && + cat >expected <<-EOF && + Revert "picked" + + This reverts commit objid. + + # ------------------------ >8 ------------------------ + # Do not modify or remove the line above. + # Everything below it will be ignored. + # + # Conflicts: + # foo + EOF + + test_must_fail git revert --cleanup=scissors picked && + + sed "s/[a-f0-9]\{40\}/objid/" .git/MERGE_MSG >actual && + test_i18ncmp expected actual +' + test_expect_success 'failed cherry-pick does not forget -s' ' pristine_detach initial && test_must_fail git cherry-pick -s picked &&
Fix a bug where the scissors line is placed after the Conflicts: section, in the case where a merge conflict occurs and commit.cleanup = scissors. Note that the removal of the if-else tower in git_sequencer_config may appear to be a no-op refactor but it actually isn't. First of all, we now accept "default" as a configuration option and also we die on an invalid cleanup mode. Most importantly, though, if commit.cleanup = scissors, the cleanup enum will be set to COMMIT_MSG_CLEANUP_SCISSORS instead of COMMIT_MSG_CLEANUP_SPACE. This allows us to append scissors to MERGE_MSG in the case of a conflict. Signed-off-by: Denton Liu <liu.denton@gmail.com> --- Documentation/git-cherry-pick.txt | 7 +++ Documentation/git-revert.txt | 7 +++ builtin/merge.c | 13 ++--- builtin/revert.c | 5 ++ sequencer.c | 22 ++++---- sequencer.h | 3 +- t/t3507-cherry-pick-conflict.sh | 88 +++++++++++++++++++++++++++++++ 7 files changed, 121 insertions(+), 24 deletions(-)