git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v8 08/11] merge: cleanup messages like commit

From
Denton Liu <liu.denton@gmail.com>
Date
Mar 20, 2019, 06:32 UTC
Message-ID
<20190320063215.GA9432@archbookpro.localdomain>
In-Reply-To
<304e99ef-553c-f538-f1d8-aaa0937da62f@gmail.com>
Hi Phillip,
On Tue, Mar 19, 2019 at 11:13:37AM +0000, Phillip Wood wrote:
Show 18 quoted lines
> Hi Denton
> 
> On 17/03/2019 10:16, Denton Liu wrote:
> > This change allows git-merge messages to be cleaned up with the
> > commit.cleanup configuration or --cleanup option, just like how
> > git-commit does it.
> > 
> > We also give git-pull the passthrough option of --cleanup so that it can
> > also take advantage of this change.
> > 
> > Finally, add testing to ensure that messages are properly cleaned up.
> > Note that some newlines that were added to the commit message were
> > removed so that if a file were read via -F, it would be copied
> > faithfully.
> > 
> > Reviewed-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> 
> I'd echo Eric's comments about Reviewed-by tags.
Will do.
Show 23 quoted lines
> 
> > Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>
> > Signed-off-by: Denton Liu <liu.denton@gmail.com>
> > ---
> > 
> > Phillip Wood wrote:
> > > cleanup needs to take an argument so PARSE_OPT_NOARG does not look
> > > right. Also I think it would be bettor from the user's point of view if
> > > the value of the argument was checked by pull before it does any work
> > > rather otherwise if they pass in invalid value pull mostly runs and then
> > > merge errors out at the end.
> > 
> > I opted not to do a check on the validity of the value of the
> > --cleanup-mode argument because the strategy options that existed before
> > also didn't verify the validity of their values. In the future, it might
> > be a good idea to check the values of both cleanup-mode and the
> > strategy options but for now, I think we can leave it as it is.
> 
> With --strategy-option the valid values depend on the --strategy option
> which may invoke an external command so in general there is no way to check
> the values are valid (it could do for the strategies we know about). With
> --cleanup we know the valid values and have a function that can check for
> them so I think it would be worth doing.
Makes sense, I'll do it (and throw in a test case for free ;) )
Show 21 quoted lines
> 
> >   Documentation/merge-options.txt |  5 +++
> >   builtin/merge.c                 | 31 +++++++++++----
> >   builtin/pull.c                  |  6 +++
> >   t/t7604-merge-custom-message.sh | 67 +++++++++++++++++++++++++++++++++
> >   wt-status.c                     | 12 ++++--
> >   wt-status.h                     |  1 +
> >   6 files changed, 112 insertions(+), 10 deletions(-)
> > 
> > diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
> > index 92a7d936c1..646100ea9a 100644
> > --- a/Documentation/merge-options.txt
> > +++ b/Documentation/merge-options.txt
> > @@ -32,6 +32,11 @@ they run `git merge`. To make it easier to adjust such scripts to the
> >   updated behaviour, the environment variable `GIT_MERGE_AUTOEDIT` can be
> >   set to `no` at the beginning of them.
> > +--cleanup=<mode>::
> > +	This option determines how the merge message will be cleaned up
> > +	before commiting or being passed on. See linkgit:git-commit[1] for more
> 
> As I commented before I don't understand 'passed on'

I changed it in git-revert.txt and git-cherry-pick.txt but I forgot to change it here. What do you think about this:

	--cleanup=<mode>::
		This option determines how the merge message will be cleaned up
		before commiting or being passed on to the commit machinery. See
		linkgit:git-commit[1] for more details.
Show 77 quoted lines
> 
> > +	details.
> > +
> >   --ff::
> >   	When the merge resolves as a fast-forward, only update the branch
> >   	pointer, without creating a merge commit.  This is the default
> > diff --git a/builtin/merge.c b/builtin/merge.c
> > index 5ce8946d39..7be03a2610 100644
> > --- a/builtin/merge.c
> > +++ b/builtin/merge.c
> > @@ -38,6 +38,7 @@
> >   #include "tag.h"
> >   #include "alias.h"
> >   #include "commit-reach.h"
> > +#include "wt-status.h"
> >   #define DEFAULT_TWOHEAD (1<<0)
> >   #define DEFAULT_OCTOPUS (1<<1)
> > @@ -98,6 +99,9 @@ enum ff_type {
> >   static enum ff_type fast_forward = FF_ALLOW;
> > +static const char *cleanup_arg;
> > +static enum commit_msg_cleanup_mode cleanup_mode;
> > +
> >   static int option_parse_message(const struct option *opt,
> >   				const char *arg, int unset)
> >   {
> > @@ -249,6 +253,7 @@ static struct option builtin_merge_options[] = {
> >   		N_("perform a commit if the merge succeeds (default)")),
> >   	OPT_BOOL('e', "edit", &option_edit,
> >   		N_("edit message before committing")),
> > +	OPT_CLEANUP(&cleanup_arg),
> >   	OPT_SET_INT(0, "ff", &fast_forward, N_("allow fast-forward (default)"), FF_ALLOW),
> >   	OPT_SET_INT_F(0, "ff-only", &fast_forward,
> >   		      N_("abort if fast-forward is not possible"),
> > @@ -612,6 +617,8 @@ static int git_merge_config(const char *k, const char *v, void *cb)
> >   		return git_config_string(&pull_twohead, k, v);
> >   	else if (!strcmp(k, "pull.octopus"))
> >   		return git_config_string(&pull_octopus, k, v);
> > +	else if (!strcmp(k, "commit.cleanup"))
> > +		return git_config_string(&cleanup_arg, k, v);
> >   	else if (!strcmp(k, "merge.renormalize"))
> >   		option_renormalize = git_config_bool(k, v);
> >   	else if (!strcmp(k, "merge.ff")) {
> > @@ -797,23 +804,32 @@ static void abort_commit(struct commit_list *remoteheads, const char *err_msg)
> >   	exit(1);
> >   }
> > +static const char comment_line_explanation[] =
> > +N_("Lines starting with '%c' will be ignored.\n");
> > +
> >   static const char merge_editor_comment[] =
> >   N_("Please enter a commit message to explain why this merge is necessary,\n"
> >      "especially if it merges an updated upstream into a topic branch.\n"
> >      "\n"
> > -   "Lines starting with '%c' will be ignored, and an empty message aborts\n"
> > -   "the commit.\n");
> > +   "An empty message aborts the commit.\n");
> >   static void write_merge_heads(struct commit_list *);
> >   static void prepare_to_commit(struct commit_list *remoteheads)
> >   {
> >   	struct strbuf msg = STRBUF_INIT;
> >   	strbuf_addbuf(&msg, &merge_msg);
> > -	strbuf_addch(&msg, '\n');
> >   	if (squash)
> >   		BUG("the control must not reach here under --squash");
> > -	if (0 < option_edit)
> > -		strbuf_commented_addf(&msg, _(merge_editor_comment), comment_line_char);
> > +	if (0 < option_edit) {
> > +		strbuf_addch(&msg, '\n');
> > +		if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS)
> > +			wt_status_append_cut_line(&msg);
> > +		else
> > +			strbuf_commented_addf(&msg, _(comment_line_explanation), comment_line_char);
> > +
> > +		strbuf_commented_addf(&msg, "\n");
> > +		strbuf_commented_addf(&msg, _(merge_editor_comment));
> > +	}
> 
> This still changes the wording of the message cf https://public-inbox.org/git/cover.1552275703.git.liu.denton@gmail.com/T/#m09cb1a05eb3bffb47ee9f25572904a7279efa362
Will do.
Thanks again for reviewing carefully!
Show 175 quoted lines
> 
> Best Wishes
> 
> Phillip
> 
> >   	if (signoff)
> >   		append_signoff(&msg, ignore_non_trailer(msg.buf, msg.len), 0);
> >   	write_merge_heads(remoteheads);
> > @@ -832,7 +848,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)
> >   		abort_commit(remoteheads, NULL);
> >   	read_merge_msg(&msg);
> > -	strbuf_stripspace(&msg, 0 < option_edit);
> > +	cleanup_message(&msg, cleanup_mode, 0);
> >   	if (!msg.len)
> >   		abort_commit(remoteheads, _("Empty commit message."));
> >   	strbuf_release(&merge_msg);
> > @@ -880,7 +896,6 @@ static int finish_automerge(struct commit *head,
> >   	parents = remoteheads;
> >   	if (!head_subsumed || fast_forward == FF_NO)
> >   		commit_list_insert(head, &parents);
> > -	strbuf_addch(&merge_msg, '\n');
> >   	prepare_to_commit(remoteheads);
> >   	if (commit_tree(merge_msg.buf, merge_msg.len, result_tree, parents,
> >   			&result_commit, NULL, sign_commit))
> > @@ -1389,6 +1404,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
> >   	if (option_edit < 0)
> >   		option_edit = default_edit_option();
> > +	cleanup_mode = get_cleanup_mode(cleanup_arg, 0 < option_edit, 1);
> > +
> >   	if (!use_strategies) {
> >   		if (!remoteheads)
> >   			; /* already up-to-date */
> > diff --git a/builtin/pull.c b/builtin/pull.c
> > index 33db889955..292c1dac27 100644
> > --- a/builtin/pull.c
> > +++ b/builtin/pull.c
> > @@ -101,6 +101,7 @@ static char *opt_signoff;
> >   static char *opt_squash;
> >   static char *opt_commit;
> >   static char *opt_edit;
> > +static char *opt_cleanup;
> >   static char *opt_ff;
> >   static char *opt_verify_signatures;
> >   static int opt_autostash = -1;
> > @@ -168,6 +169,9 @@ static struct option pull_options[] = {
> >   	OPT_PASSTHRU(0, "edit", &opt_edit, NULL,
> >   		N_("edit message before committing"),
> >   		PARSE_OPT_NOARG),
> > +	OPT_PASSTHRU(0, "cleanup", &opt_cleanup, NULL,
> > +		N_("how to strip spaces and #comments from message"),
> > +		0),
> >   	OPT_PASSTHRU(0, "ff", &opt_ff, NULL,
> >   		N_("allow fast-forward"),
> >   		PARSE_OPT_NOARG),
> > @@ -644,6 +648,8 @@ static int run_merge(void)
> >   		argv_array_push(&args, opt_commit);
> >   	if (opt_edit)
> >   		argv_array_push(&args, opt_edit);
> > +	if (opt_cleanup)
> > +		argv_array_push(&args, opt_cleanup);
> >   	if (opt_ff)
> >   		argv_array_push(&args, opt_ff);
> >   	if (opt_verify_signatures)
> > diff --git a/t/t7604-merge-custom-message.sh b/t/t7604-merge-custom-message.sh
> > index b045fdb413..c9685a318d 100755
> > --- a/t/t7604-merge-custom-message.sh
> > +++ b/t/t7604-merge-custom-message.sh
> > @@ -51,4 +51,71 @@ test_expect_success 'merge --log appends to custom message' '
> >   	test_cmp exp.log actual
> >   '
> > +mesg_with_comment_and_newlines='
> > +# text
> > +
> > +'
> > +
> > +test_expect_success 'prepare file with comment line and trailing newlines'  '
> > +	printf "%s" "$mesg_with_comment_and_newlines" >expect
> > +'
> > +
> > +test_expect_success 'cleanup commit messages (verbatim option)' '
> > +	git reset --hard c1 &&
> > +	git merge --cleanup=verbatim -F expect c2 &&
> > +	git cat-file commit HEAD >actual &&
> > +	sed -e "1,/^$/d" <actual >tmp &&
> > +	mv tmp actual &&
> > +	test_cmp expect actual
> > +'
> > +
> > +test_expect_success 'cleanup commit messages (whitespace option)' '
> > +	git reset --hard c1 &&
> > +	test_write_lines "" "# text" "" >text &&
> > +	echo "# text" >expect &&
> > +	git merge --cleanup=whitespace -F text c2 &&
> > +	git cat-file commit HEAD >actual &&
> > +	sed -e "1,/^$/d" <actual >tmp &&
> > +	mv tmp actual &&
> > +	test_cmp expect actual
> > +'
> > +
> > +test_expect_success 'cleanup merge messages (scissors option)' '
> > +	git reset --hard c1 &&
> > +	cat >text <<-\EOF &&
> > +
> > +	# to be kept
> > +
> > +	  # ------------------------ >8 ------------------------
> > +	# to be kept, too
> > +	# ------------------------ >8 ------------------------
> > +	to be removed
> > +	# ------------------------ >8 ------------------------
> > +	to be removed, too
> > +	EOF
> > +
> > +	cat >expect <<-\EOF &&
> > +	# to be kept
> > +
> > +	  # ------------------------ >8 ------------------------
> > +	# to be kept, too
> > +	EOF
> > +	git merge --cleanup=scissors -e -F text c2 &&
> > +	git cat-file commit HEAD >actual &&
> > +	sed -e "1,/^$/d" <actual >tmp &&
> > +	mv tmp actual &&
> > +	test_cmp expect actual
> > +'
> > +
> > +test_expect_success 'cleanup commit messages (strip option)' '
> > +	git reset --hard c1 &&
> > +	test_write_lines "" "# text" "sample" "" >text &&
> > +	echo sample >expect &&
> > +	git merge --cleanup=strip -F text c2 &&
> > +	git cat-file commit HEAD >actual &&
> > +	sed -e "1,/^$/d" <actual >tmp &&
> > +	mv tmp actual &&
> > +	test_cmp expect actual
> > +'
> > +
> >   test_done
> > diff --git a/wt-status.c b/wt-status.c
> > index 445a36204a..b81fcd428d 100644
> > --- a/wt-status.c
> > +++ b/wt-status.c
> > @@ -1006,13 +1006,19 @@ size_t wt_status_locate_end(const char *s, size_t len)
> >   	return len;
> >   }
> > -void wt_status_add_cut_line(FILE *fp)
> > +void wt_status_append_cut_line(struct strbuf *buf)
> >   {
> >   	const char *explanation = _("Do not modify or remove the line above.\nEverything below it will be ignored.");
> > +
> > +	strbuf_commented_addf(buf, "%s", cut_line);
> > +	strbuf_add_commented_lines(buf, explanation, strlen(explanation));
> > +}
> > +
> > +void wt_status_add_cut_line(FILE *fp)
> > +{
> >   	struct strbuf buf = STRBUF_INIT;
> > -	fprintf(fp, "%c %s", comment_line_char, cut_line);
> > -	strbuf_add_commented_lines(&buf, explanation, strlen(explanation));
> > +	wt_status_append_cut_line(&buf);
> >   	fputs(buf.buf, fp);
> >   	strbuf_release(&buf);
> >   }
> > diff --git a/wt-status.h b/wt-status.h
> > index 3a95975032..64f1ddc9fd 100644
> > --- a/wt-status.h
> > +++ b/wt-status.h
> > @@ -129,6 +129,7 @@ struct wt_status {
> >   };
> >   size_t wt_status_locate_end(const char *s, size_t len);
> > +void wt_status_append_cut_line(struct strbuf *buf);
> >   void wt_status_add_cut_line(FILE *fp);
> >   void wt_status_prepare(struct repository *r, struct wt_status *s);
> >   void wt_status_print(struct wt_status *s);
> >
Previous: Phillip WoodNext: Denton Liu
Message 62 of 105 in “Fix scissors bug during merge conflict”
  1. 0/2 Fix scissors bug during merge conflictDenton Liu, Nov 14, 2018
  2. 1/2 commit: don't add scissors line if one existsDenton Liu, Nov 14, 2018
  3. Junio C HamanoNov 14, 2018
  4. Denton LiuNov 14, 2018
  5. Junio C HamanoNov 16, 2018
  6. 2/2 merge: add scissors line on merge conflictDenton Liu, Nov 14, 2018
  7. Junio C HamanoNov 14, 2018
  8. Denton LiuNov 14, 2018
  9. 0/2 Fix scissors bug during merge conflictDenton Liu, Nov 16, 2018
  10. 2/2 merge: add scissors line on merge conflictDenton Liu, Nov 16, 2018
  11. 0/1 Fix scissors bug during merge conflictDenton Liu, Nov 17, 2018
  12. 1/1 merge: add scissors line on merge conflictDenton Liu, Nov 17, 2018
  13. SZEDER GáborNov 18, 2018
  14. Junio C HamanoNov 18, 2018
  15. 0/2 Fix scissors bug during merge conflictDenton Liu, Nov 21, 2018
  16. 1/2 t7600: clean up 'merge --squash c3 with c7' testDenton Liu, Nov 21, 2018
  17. 2/2 merge: add scissors line on merge conflictDenton Liu, Nov 21, 2018
  18. Junio C HamanoNov 21, 2018
  19. Denton LiuNov 22, 2018
  20. Junio C HamanoNov 24, 2018
  21. 0/4 Add git-merge --cleanup supportDenton Liu, Dec 25, 2018
  22. 1/4 commit: extract cleanup_mode functions to sequencerDenton Liu, Dec 25, 2018
  23. 2/4 t7600: clean up 'merge --squash c3 with c7' testDenton Liu, Dec 25, 2018
  24. 3/4 merge: cleanup messages like commitDenton Liu, Dec 25, 2018
  25. 4/4 merge: add scissors line on merge conflictDenton Liu, Dec 25, 2018
  26. 0/4 Add git-merge --cleanup supportDenton Liu, Jan 23, 2019
  27. 1/4 commit: extract cleanup_mode functions to sequencerDenton Liu, Jan 23, 2019
  28. 2/4 t7600: clean up 'merge --squash c3 with c7' testDenton Liu, Jan 23, 2019
  29. 3/4 merge: cleanup messages like commitDenton Liu, Jan 23, 2019
  30. 4/4 merge: add scissors line on merge conflictDenton Liu, Jan 23, 2019
  31. 0/8 Fix scissors bug during conflictDenton Liu, Mar 11, 2019
  32. 1/8 t7600: clean up 'merge --squash c3 with c7' testDenton Liu, Mar 11, 2019
  33. Junio C HamanoMar 12, 2019
  34. 2/8 t3507: cleanup space after redirection operatorsDenton Liu, Mar 11, 2019
  35. 3/8 commit: extract cleanup_mode functions to sequencerDenton Liu, Mar 11, 2019
  36. 4/8 sequencer.c: remove duplicate codeDenton Liu, Mar 11, 2019
  37. Phillip WoodMar 11, 2019
  38. 5/8 merge: cleanup messages like commitDenton Liu, Mar 11, 2019
  39. Eric SunshineMar 11, 2019
  40. Phillip WoodMar 11, 2019
  41. Eric SunshineMar 11, 2019
  42. Phillip WoodMar 11, 2019
  43. Junio C HamanoMar 12, 2019
  44. 6/8 merge: add scissors line on merge conflictDenton Liu, Mar 11, 2019
  45. Eric SunshineMar 11, 2019
  46. 7/8 sequencer.c: define get_config_from_cleanupDenton Liu, Mar 11, 2019
  47. Eric SunshineMar 11, 2019
  48. Junio C HamanoMar 12, 2019
  49. 8/8 cherry-pick/revert: add scissors line on merge conflictDenton Liu, Mar 11, 2019
  50. Phillip WoodMar 12, 2019
  51. Junio C HamanoMar 11, 2019
  52. 00/11 Fix scissors bug during conflictDenton Liu, Mar 17, 2019
  53. 01/11 t7600: clean up styleDenton Liu, Mar 17, 2019
  54. 02/11 t3507: cleanup space after redirection operatorsDenton Liu, Mar 17, 2019
  55. 03/11 t7604: refactor out Git commands upstream of pipeDenton Liu, Mar 17, 2019
  56. 04/11 t7502: clean up test styleDenton Liu, Mar 17, 2019
  57. 05/11 commit: extract cleanup_mode functions to sequencerDenton Liu, Mar 17, 2019
  58. 06/11 parse-options.h: extract common --cleanup optionDenton Liu, Mar 17, 2019
  59. 07/11 sequencer.c: remove duplicate codeDenton Liu, Mar 17, 2019
  60. 08/11 merge: cleanup messages like commitDenton Liu, Mar 17, 2019
  61. Phillip WoodMar 19, 2019
  62. Denton LiuMar 20, 2019
  63. 09/11 merge: add scissors line on merge conflictDenton Liu, Mar 17, 2019
  64. 10/11 sequencer.c: define describe_cleanup_modeDenton Liu, Mar 17, 2019
  65. Eric SunshineMar 18, 2019
  66. Denton LiuMar 18, 2019
  67. Eric SunshineMar 18, 2019
  68. Ramsay JonesMar 19, 2019
  69. 11/11 cherry-pick/revert: add scissors line on merge conflictDenton Liu, Mar 17, 2019
  70. SZEDER GáborMar 17, 2019
  71. Denton LiuMar 18, 2019
  72. Junio C HamanoMar 18, 2019
  73. Denton LiuMar 18, 2019
  74. Junio C HamanoMar 18, 2019
  75. 00/11 Fix scissors bug during conflictDenton Liu, Mar 21, 2019
  76. 01/11 t7600: clean up styleDenton Liu, Mar 21, 2019
  77. 02/11 t3507: clean up styleDenton Liu, Mar 21, 2019
  78. 03/11 t7604: clean up styleDenton Liu, Mar 21, 2019
  79. 04/11 t7502: clean up styleDenton Liu, Mar 21, 2019
  80. 05/11 commit: extract cleanup_mode functions to sequencerDenton Liu, Mar 21, 2019
  81. 06/11 parse-options.h: extract common --cleanup optionDenton Liu, Mar 21, 2019
  82. 07/11 sequencer.c: remove duplicate codeDenton Liu, Mar 21, 2019
  83. Phillip WoodMar 26, 2019
  84. 08/11 merge: cleanup messages like commitDenton Liu, Mar 21, 2019
  85. 09/11 merge: add scissors line on merge conflictDenton Liu, Mar 21, 2019
  86. 10/11 sequencer.c: define describe_cleanup_modeDenton Liu, Mar 21, 2019
  87. 11/11 cherry-pick/revert: add scissors line on merge conflictDenton Liu, Mar 21, 2019
  88. 00/10 Fix scissors bug during conflictPhillip Wood, Apr 17, 2019
  89. 01/10 t7600: clean up stylePhillip Wood, Apr 17, 2019
  90. 04/10 t7502: clean up stylePhillip Wood, Apr 17, 2019
  91. 03/10 t7604: clean up stylePhillip Wood, Apr 17, 2019
  92. 02/10 t3507: clean up stylePhillip Wood, Apr 17, 2019
  93. 05/10 commit: extract cleanup_mode functions to sequencerPhillip Wood, Apr 17, 2019
  94. 06/10 parse-options.h: extract common --cleanup optionPhillip Wood, Apr 17, 2019
  95. 07/10 merge: cleanup messages like commitPhillip Wood, Apr 17, 2019
  96. 08/10 merge: add scissors line on merge conflictPhillip Wood, Apr 17, 2019
  97. 09/10 sequencer.c: save and restore cleanup modePhillip Wood, Apr 17, 2019
  98. Denton LiuApr 17, 2019
  99. Phillip WoodApr 17, 2019
  100. Denton LiuApr 18, 2019
  101. 10/10 cherry-pick/revert: add scissors line on merge conflictPhillip Wood, Apr 17, 2019
  102. Junio C HamanoApr 18, 2019
  103. Phillip WoodApr 18, 2019
  104. 1/2 commit: don't add scissors line if one exists in MERGE_MSGDenton Liu, Nov 16, 2018
  105. Junio C HamanoNov 17, 2018

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.