threads / patch / 28635

v3Teach merge the '[-e|--edit]' option

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

## tl;dr

6 messages between Oct 8, 2011 and Oct 11, 2011. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Jay Soffian· Oct 8, 2011, 18:39 UTC · lore

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 <jaysoffian@gmail.com>
---
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(-)
Show changes to 3 files +91 −39

Documentation/merge-options.txt, builtin/merge.c, t/t7600-merge.sh

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
Junio C Hamano· Oct 10, 2011, 21:05 UTC · re: Jay Soffian · lore

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

Jay Soffian <jaysoffian@gmail.com> writes:
Show 15 quoted lines
> 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 <jaysoffian@gmail.com>
> ---
Thanks.
Show 21 quoted lines
> +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.

Show 31 quoted lines
> @@ -1015,6 +1041,36 @@ static int setup_with_upstream(const char ***argv)
> ...
> +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);
> +}
Again very nicely done.
Show 14 quoted lines
> 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?

Show 9 quoted lines
> +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(-)
Show changes to 2 files +11 −2

builtin/merge.c, t/t7600-merge.sh

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
 '
 
Jay Soffian· Oct 10, 2011, 22:53 UTC · re: Junio C Hamano · lore

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

On Mon, Oct 10, 2011 at 2:05 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 24 quoted lines
> Jay Soffian <jaysoffian@gmail.com> writes:
>> +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.
Show 18 quoted lines
>> 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.

Show 14 quoted lines
>> +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).

Show 18 quoted lines
>  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);
I'm guessing the '\n' is always needed (per above), but I'm not sure.
Show 27 quoted lines
> 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
>  '

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.
Junio C Hamano· Oct 11, 2011, 00:00 UTC · re: Jay Soffian · lore

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

Jay Soffian <jaysoffian@gmail.com> writes:
Show 5 quoted lines
>> 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.
Jay Soffian· Oct 11, 2011, 00:09 UTC · re: Junio C Hamano · lore

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

On Mon, Oct 10, 2011 at 5:00 PM, Junio C Hamano <gitster@pobox.com> 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.
Peter Krefting· Oct 11, 2011, 13:57 UTC · re: Jay Soffian · lore

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

Jay Soffian:
Show 5 quoted lines
> +--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/

← back to recent threads