[PATCH v3] Teach merge the '[-e|--edit]' option

Implemented internally instead of as "git merge --no-commit && git commit"
so that "merge --edit" is otherwise consistent (hooks, etc) with "merge".

Note: the edit message does not include the status information that one
gets with "commit --status" and it is cleaned up after editing like one
gets with "commit --cleanup=default". A later patch could add the status
information if desired.

Note: previously we were not calling stripspace() after running the
prepare-commit-msg hook. Now we are, stripping comments and
leading/trailing whitespace lines if --edit is given, otherwise only
stripping leading/trailing whitespace lines if not given --edit.

Signed-off-by: Jay Soffian <redacted>
---
I probably can't spend more time on this patch any time soon. Hopefully
this iteration is close enough. If not, maybe Todd can help.

 Documentation/merge-options.txt |    6 ++
 builtin/merge.c                 |  109 +++++++++++++++++++++++++--------------
 t/t7600-merge.sh                |   15 +++++
 3 files changed, 91 insertions(+), 39 deletions(-)

diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index b613d4ed08..6bd0b041c3 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt

@@ -7,6 +7,12 @@ With --no-commit perform the merge but pretend the merge
 failed and do not autocommit, to give the user a chance to
 inspect and further tweak the merge result before committing.
 
+--edit::
+-e::
++
+	Invoke editor before committing successful merge to further
+	edit the default merge message.
+
 --ff::
 --no-ff::
 	Do not generate a merge commit if the merge resolved as

diff --git a/builtin/merge.c b/builtin/merge.c
index ee56974371..fcb7a60bfa 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c

@@ -46,7 +46,7 @@ static const char * const builtin_merge_usage[] = {
 
 static int show_diffstat = 1, shortlog_len, squash;
 static int option_commit = 1, allow_fast_forward = 1;
-static int fast_forward_only;
+static int fast_forward_only, option_edit;
 static int allow_trivial = 1, have_message;
 static struct strbuf merge_msg;
 static struct commit_list *remoteheads;

@@ -190,6 +190,8 @@ static struct option builtin_merge_options[] = {
 		"create a single commit instead of doing a merge"),
 	OPT_BOOLEAN(0, "commit", &option_commit,
 		"perform a commit if the merge succeeds (default)"),
+	OPT_BOOLEAN('e', "edit", &option_edit,
+		"edit message before committing"),
 	OPT_BOOLEAN(0, "ff", &allow_fast_forward,
 		"allow fast-forward (default)"),
 	OPT_BOOLEAN(0, "ff-only", &fast_forward_only,

@@ -842,30 +844,54 @@ static void add_strategies(const char *string, unsigned attr)
 
 }
 
-static void write_merge_msg(void)
+static void write_merge_msg(struct strbuf *msg)
 {
 	int fd = open(git_path("MERGE_MSG"), O_WRONLY | O_CREAT, 0666);
 	if (fd < 0)
 		die_errno(_("Could not open '%s' for writing"),
 			  git_path("MERGE_MSG"));
-	if (write_in_full(fd, merge_msg.buf, merge_msg.len) != merge_msg.len)
+	if (write_in_full(fd, msg->buf, msg->len) != msg->len)
 		die_errno(_("Could not write to '%s'"), git_path("MERGE_MSG"));
 	close(fd);
 }
 
-static void read_merge_msg(void)
+static void read_merge_msg(struct strbuf *msg)
 {
-	strbuf_reset(&merge_msg);
-	if (strbuf_read_file(&merge_msg, git_path("MERGE_MSG"), 0) < 0)
+	strbuf_reset(msg);
+	if (strbuf_read_file(msg, git_path("MERGE_MSG"), 0) < 0)
 		die_errno(_("Could not read from '%s'"), git_path("MERGE_MSG"));
 }
 
-static void run_prepare_commit_msg(void)
+static void write_merge_state(void);
+static void abort_commit(const char *err_msg)
 {
-	write_merge_msg();
+	if (err_msg)
+		error("%s", err_msg);
+	fprintf(stderr,
+		_("Not committing merge; use 'git commit' to complete the merge.\n"));
+	write_merge_state();
+	exit(1);
+}
+
+static void prepare_to_commit(void)
+{
+	struct strbuf msg = STRBUF_INIT;
+	strbuf_addbuf(&msg, &merge_msg);
+	strbuf_addch(&msg, '\n');
+	write_merge_msg(&msg);
 	run_hook(get_index_file(), "prepare-commit-msg",
 		 git_path("MERGE_MSG"), "merge", NULL, NULL);
-	read_merge_msg();
+	if (option_edit) {
+		if (launch_editor(git_path("MERGE_MSG"), NULL, NULL))
+			abort_commit(NULL);
+	}
+	read_merge_msg(&msg);
+	stripspace(&msg, option_edit);
+	if (!msg.len)
+		abort_commit(_("Empty commit message."));
+	strbuf_release(&merge_msg);
+	strbuf_addbuf(&merge_msg, &msg);
+	strbuf_release(&msg);
 }
 
 static int merge_trivial(void)

@@ -879,7 +905,7 @@ static int merge_trivial(void)
 	parent->next = xmalloc(sizeof(*parent->next));
 	parent->next->item = remoteheads->item;
 	parent->next->next = NULL;
-	run_prepare_commit_msg();
+	prepare_to_commit();
 	commit_tree(merge_msg.buf, result_tree, parent, result_commit, NULL);
 	finish(result_commit, "In-index merge");
 	drop_save();

@@ -907,9 +933,9 @@ static int finish_automerge(struct commit_list *common,
 		for (j = remoteheads; j; j = j->next)
 			pptr = &commit_list_insert(j->item, pptr)->next;
 	}
-	free_commit_list(remoteheads);
 	strbuf_addch(&merge_msg, '\n');
-	run_prepare_commit_msg();
+	prepare_to_commit();
+	free_commit_list(remoteheads);
 	commit_tree(merge_msg.buf, result_tree, parents, result_commit, NULL);
 	strbuf_addf(&buf, "Merge made by the '%s' strategy.", wt_strategy);
 	finish(result_commit, buf.buf);

@@ -1015,6 +1041,36 @@ static int setup_with_upstream(const char ***argv)
 	return i;
 }
 
+static void write_merge_state(void)
+{
+	int fd;
+	struct commit_list *j;
+	struct strbuf buf = STRBUF_INIT;
+
+	for (j = remoteheads; j; j = j->next)
+		strbuf_addf(&buf, "%s\n",
+			sha1_to_hex(j->item->object.sha1));
+	fd = open(git_path("MERGE_HEAD"), O_WRONLY | O_CREAT, 0666);
+	if (fd < 0)
+		die_errno(_("Could not open '%s' for writing"),
+			  git_path("MERGE_HEAD"));
+	if (write_in_full(fd, buf.buf, buf.len) != buf.len)
+		die_errno(_("Could not write to '%s'"), git_path("MERGE_HEAD"));
+	close(fd);
+	strbuf_addch(&merge_msg, '\n');
+	write_merge_msg(&merge_msg);
+	fd = open(git_path("MERGE_MODE"), O_WRONLY | O_CREAT | O_TRUNC, 0666);
+	if (fd < 0)
+		die_errno(_("Could not open '%s' for writing"),
+			  git_path("MERGE_MODE"));
+	strbuf_reset(&buf);
+	if (!allow_fast_forward)
+		strbuf_addf(&buf, "no-ff");
+	if (write_in_full(fd, buf.buf, buf.len) != buf.len)
+		die_errno(_("Could not write to '%s'"), git_path("MERGE_MODE"));
+	close(fd);
+}
+
 int cmd_merge(int argc, const char **argv, const char *prefix)
 {
 	unsigned char result_tree[20];

@@ -1418,33 +1474,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 
 	if (squash)
 		finish(NULL, NULL);
-	else {
-		int fd;
-		struct commit_list *j;
-
-		for (j = remoteheads; j; j = j->next)
-			strbuf_addf(&buf, "%s\n",
-				sha1_to_hex(j->item->object.sha1));
-		fd = open(git_path("MERGE_HEAD"), O_WRONLY | O_CREAT, 0666);
-		if (fd < 0)
-			die_errno(_("Could not open '%s' for writing"),
-				  git_path("MERGE_HEAD"));
-		if (write_in_full(fd, buf.buf, buf.len) != buf.len)
-			die_errno(_("Could not write to '%s'"), git_path("MERGE_HEAD"));
-		close(fd);
-		strbuf_addch(&merge_msg, '\n');
-		write_merge_msg();
-		fd = open(git_path("MERGE_MODE"), O_WRONLY | O_CREAT | O_TRUNC, 0666);
-		if (fd < 0)
-			die_errno(_("Could not open '%s' for writing"),
-				  git_path("MERGE_MODE"));
-		strbuf_reset(&buf);
-		if (!allow_fast_forward)
-			strbuf_addf(&buf, "no-ff");
-		if (write_in_full(fd, buf.buf, buf.len) != buf.len)
-			die_errno(_("Could not write to '%s'"), git_path("MERGE_MODE"));
-		close(fd);
-	}
+	else
+		write_merge_state();
 
 	if (merge_was_ok) {
 		fprintf(stderr, _("Automatic merge went well; "

diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index 87aac835a1..8c6b811718 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh

@@ -643,4 +643,19 @@ test_expect_success 'amending no-ff merge commit' '
 
 test_debug 'git log --graph --decorate --oneline --all'
 
+cat >editor <<\EOF
+#!/bin/sh
+# strip comments and blank lines from end of message
+sed -e '/^#/d' < "$1" | sed -e :a -e '/^\n*$/{$d;N;ba' -e '}' > expected
+EOF
+chmod 755 editor
+
+test_expect_success 'merge --no-ff --edit' '
+	git reset --hard c0 &&
+	EDITOR=./editor git merge --no-ff --edit c1 &&
+	verify_parents $c0 $c1 &&
+	git cat-file commit HEAD | sed "1,/^$/d" > actual &&
+	test_cmp actual expected
+'
+
 test_done

-- 
1.7.7.147.g4f6dc9

Jay Soffian [off-list ref] writes:

Implemented internally instead of as "git merge --no-commit && git commit"
so that "merge --edit" is otherwise consistent (hooks, etc) with "merge".

Note: the edit message does not include the status information that one
gets with "commit --status" and it is cleaned up after editing like one
gets with "commit --cleanup=default". A later patch could add the status
information if desired.

Note: previously we were not calling stripspace() after running the
prepare-commit-msg hook. Now we are, stripping comments and
leading/trailing whitespace lines if --edit is given, otherwise only
stripping leading/trailing whitespace lines if not given --edit.

Signed-off-by: Jay Soffian <redacted>
---

Thanks.

+static void prepare_to_commit(void)
+{
+	struct strbuf msg = STRBUF_INIT;
+	strbuf_addbuf(&msg, &merge_msg);
+	strbuf_addch(&msg, '\n');
+	write_merge_msg(&msg);
 	run_hook(get_index_file(), "prepare-commit-msg",
 		 git_path("MERGE_MSG"), "merge", NULL, NULL);
-	read_merge_msg();
+	if (option_edit) {
+		if (launch_editor(git_path("MERGE_MSG"), NULL, NULL))
+			abort_commit(NULL);
+	}
+	read_merge_msg(&msg);
+	stripspace(&msg, option_edit);
+	if (!msg.len)
+		abort_commit(_("Empty commit message."));
+	strbuf_release(&merge_msg);
+	strbuf_addbuf(&merge_msg, &msg);
+	strbuf_release(&msg);
 }

An abstraction very nicely done.

I am not sure about the '\n' you unconditionally added at the end of the
existing message.

I think running stripspace(&msg, option_edit) is a good change, even
though some people might feel it is a regression. "git commit" also cleans
up the whitespace cruft left by prepare-commit-message hook when the
editor is not in use, and this change makes it consistent.

Again very nicely done.

I am not sure about this one. Wouldn't this want to be editing the given
file to make sure that the edited content appear in the result, not just
testing the additional stripspace() call you added in the codepath?

+test_expect_success 'merge --no-ff --edit' '
+	git reset --hard c0 &&
+	EDITOR=./editor git merge --no-ff --edit c1 &&
+	verify_parents $c0 $c1 &&
+	git cat-file commit HEAD | sed "1,/^$/d" > actual &&
+	test_cmp actual expected
+'
+
 test_done

So perhaps this one on top? I am just suspecting that your additional '\n'
is to make sure we do not write out a file with an incomplete line with
this patch, but that change is not explained in your commit log message,
so I am not sure.

 builtin/merge.c  |    3 ++-
 t/t7600-merge.sh |   10 +++++++++-
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index a8dbf4a..09ffc07 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c

@@ -867,7 +867,8 @@ static void prepare_to_commit(void)
 {
 	struct strbuf msg = STRBUF_INIT;
 	strbuf_addbuf(&msg, &merge_msg);
-	strbuf_addch(&msg, '\n');
+	if (msg.len && msg.buf[msg.len-1] != '\n')
+		strbuf_addch(&msg, '\n');
 	write_merge_msg(&msg);
 	run_hook(get_index_file(), "prepare-commit-msg",
 		 git_path("MERGE_MSG"), "merge", NULL, NULL);

diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index 8c6b811..3008e4e 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh

@@ -645,6 +645,12 @@ test_debug 'git log --graph --decorate --oneline --all'
 
 cat >editor <<\EOF
 #!/bin/sh
+# Add a new message string that was not in the template
+(
+	echo "Merge work done on the side branch c1"
+	echo
+	cat <"$1"
+) >"$1.tmp" && mv "$1.tmp" "$1"
 # strip comments and blank lines from end of message
 sed -e '/^#/d' < "$1" | sed -e :a -e '/^\n*$/{$d;N;ba' -e '}' > expected
 EOF

@@ -654,7 +660,9 @@ test_expect_success 'merge --no-ff --edit' '
 	git reset --hard c0 &&
 	EDITOR=./editor git merge --no-ff --edit c1 &&
 	verify_parents $c0 $c1 &&
-	git cat-file commit HEAD | sed "1,/^$/d" > actual &&
+	git cat-file commit HEAD >raw &&
+	grep "work done on the side branch" raw &&
+	sed "1,/^$/d" >actual raw &&
 	test_cmp actual expected
 '
 

On Mon, Oct 10, 2011 at 2:05 PM, Junio C Hamano [off-list ref] wrote:

Jay Soffian [off-list ref] writes:

quoted

+static void prepare_to_commit(void)
+{
+     struct strbuf msg = STRBUF_INIT;
+     strbuf_addbuf(&msg, &merge_msg);
+     strbuf_addch(&msg, '\n');
+     write_merge_msg(&msg);
      run_hook(get_index_file(), "prepare-commit-msg",
               git_path("MERGE_MSG"), "merge", NULL, NULL);
-     read_merge_msg();
+     if (option_edit) {
+             if (launch_editor(git_path("MERGE_MSG"), NULL, NULL))
+                     abort_commit(NULL);
+     }
+     read_merge_msg(&msg);
+     stripspace(&msg, option_edit);
+     if (!msg.len)
+             abort_commit(_("Empty commit message."));
+     strbuf_release(&merge_msg);
+     strbuf_addbuf(&merge_msg, &msg);
+     strbuf_release(&msg);
 }

An abstraction very nicely done.

<blush> :-)

I am not sure about the '\n' you unconditionally added at the end of the
existing message.

Right, the old code does that when the merge fails, counting on (I
think) git-commit to then take care of any extra newlines. My
reasoning was tack it on before running prepare-commit-msg, then run
stripspace() after the hook and and editor, which will take care of
any excess newlines. I guess this would be a regression if someone's
prepare-commit-msg hook blindly appends to the commit message.

I think running stripspace(&msg, option_edit) is a good change, even
though some people might feel it is a regression. "git commit" also cleans
up the whitespace cruft left by prepare-commit-message hook when the
editor is not in use, and this change makes it consistent.

Correct.

quoted

diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index 87aac835a1..8c6b811718 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh

@@ -643,4 +643,19 @@ test_expect_success 'amending no-ff merge commit' '

 test_debug 'git log --graph --decorate --oneline --all'

+cat >editor <<\EOF
+#!/bin/sh
+# strip comments and blank lines from end of message
+sed -e '/^#/d' < "$1" | sed -e :a -e '/^\n*$/{$d;N;ba' -e '}' > expected
+EOF
+chmod 755 editor

I am not sure about this one. Wouldn't this want to be editing the given
file to make sure that the edited content appear in the result, not just
testing the additional stripspace() call you added in the codepath?

Yep.

I added this test under the previous iteration of the patch when I was
concerned that commit message make it through the external commit code
path correctly. It doesn't really make sense with this iteration now
that I think about it. The part about stripping comments and newlines
is no longer needed.

quoted

+test_expect_success 'merge --no-ff --edit' '
+     git reset --hard c0 &&
+     EDITOR=./editor git merge --no-ff --edit c1 &&
+     verify_parents $c0 $c1 &&
+     git cat-file commit HEAD | sed "1,/^$/d" > actual &&
+     test_cmp actual expected
+'
+
 test_done

So perhaps this one on top? I am just suspecting that your additional '\n'
is to make sure we do not write out a file with an incomplete line with
this patch, but that change is not explained in your commit log message,
so I am not sure.

I assumed the '\n' was needed as it's added (unconditionally) before
writing MERGE_MSG when the merge fails. I didn't notice that when I
added the prepare-commit-msg hook support to merge.c (65969d43d1).

I'm guessing the '\n' is always needed (per above), but I'm not sure.

Okay. A test that the merge is aborted if the message is empty would
also be good.

I'll try to find time to send another iteration with better tests. May
not be till next week though.

j.

Jay Soffian [off-list ref] writes:

quoted

I am not sure about the '\n' you unconditionally added at the end of the
existing message.

Right, the old code does that when the merge fails, counting on (I
think) git-commit to then take care of any extra newlines.

Ahh, that explains it. I was originally about to suggest running the
stripspace() only when we run the editor, and saw a failure from an
unrelated test and realized that running stripspace() on the result of
prepare-commit-msg is the right thing to do after all, because that is
what is done by "git commit". Yes, the current code does rely on the
stripspace to remove it, so there is no need to make it conditional.

So if we drop the "conditionally add '\n'" part in builtin/merge.c from my
"how about this on top" patch and we should be ready to go, right?

Thanks.

On Mon, Oct 10, 2011 at 5:00 PM, Junio C Hamano [off-list ref] wrote:

So if we drop the "conditionally add '\n'" part in builtin/merge.c from my
"how about this on top" patch and we should be ready to go, right?

Yes. I can send a followup patch adding an additional test case next
week (for the case where the editor zeros out the message).

Thanks Junio!

j.

Jay Soffian:

+--edit::
+-e::
++
+	Invoke editor before committing successful merge to further
+	edit the default merge message.

I have a feature request, and that is to also add a configuration option to 
make this the default behaviour.

-- 
\\// Peter - http://www.softwolves.pp.se/