git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3] Teach merge the '[-e|--edit]' option
@ 2011-10-08 18:39 Jay Soffian
  2011-10-10 21:05 ` Junio C Hamano
  2011-10-11 13:57 ` Peter Krefting
  0 siblings, 2 replies; 6+ messages in thread
From: Jay Soffian @ 2011-10-08 18:39 UTC (permalink / raw)
  To: git; +Cc: Jay Soffian, Junio C Hamano, Todd A. Jacobs

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(-)

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

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v3] Teach merge the '[-e|--edit]' option
  2011-10-08 18:39 [PATCH v3] Teach merge the '[-e|--edit]' option Jay Soffian
@ 2011-10-10 21:05 ` Junio C Hamano
  2011-10-10 22:53   ` Jay Soffian
  2011-10-11 13:57 ` Peter Krefting
  1 sibling, 1 reply; 6+ messages in thread
From: Junio C Hamano @ 2011-10-10 21:05 UTC (permalink / raw)
  To: Jay Soffian; +Cc: git, Todd A. Jacobs

Jay Soffian <jaysoffian@gmail.com> 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 <jaysoffian@gmail.com>
> ---

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.

> @@ -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.

> 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?

> +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
 '
 

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v3] Teach merge the '[-e|--edit]' option
  2011-10-10 21:05 ` Junio C Hamano
@ 2011-10-10 22:53   ` Jay Soffian
  2011-10-11  0:00     ` Junio C Hamano
  0 siblings, 1 reply; 6+ messages in thread
From: Jay Soffian @ 2011-10-10 22:53 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Todd A. Jacobs

On Mon, Oct 10, 2011 at 2:05 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

>> 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.

>> +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).

>  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.

> 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.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3] Teach merge the '[-e|--edit]' option
  2011-10-10 22:53   ` Jay Soffian
@ 2011-10-11  0:00     ` Junio C Hamano
  2011-10-11  0:09       ` Jay Soffian
  0 siblings, 1 reply; 6+ messages in thread
From: Junio C Hamano @ 2011-10-11  0:00 UTC (permalink / raw)
  To: Jay Soffian; +Cc: git, Todd A. Jacobs

Jay Soffian <jaysoffian@gmail.com> writes:

>> 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.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3] Teach merge the '[-e|--edit]' option
  2011-10-11  0:00     ` Junio C Hamano
@ 2011-10-11  0:09       ` Jay Soffian
  0 siblings, 0 replies; 6+ messages in thread
From: Jay Soffian @ 2011-10-11  0:09 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Todd A. Jacobs

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.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3] Teach merge the '[-e|--edit]' option
  2011-10-08 18:39 [PATCH v3] Teach merge the '[-e|--edit]' option Jay Soffian
  2011-10-10 21:05 ` Junio C Hamano
@ 2011-10-11 13:57 ` Peter Krefting
  1 sibling, 0 replies; 6+ messages in thread
From: Peter Krefting @ 2011-10-11 13:57 UTC (permalink / raw)
  To: Jay Soffian; +Cc: Git Mailing List, Junio C Hamano, Todd A. Jacobs

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/

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2011-10-11 13:58 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-10-08 18:39 [PATCH v3] Teach merge the '[-e|--edit]' option Jay Soffian
2011-10-10 21:05 ` Junio C Hamano
2011-10-10 22:53   ` Jay Soffian
2011-10-11  0:00     ` Junio C Hamano
2011-10-11  0:09       ` Jay Soffian
2011-10-11 13:57 ` Peter Krefting

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).