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

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

[Jay Soffian]

Implement "git merge [-e|--edit]" as "git merge --no-commit && git commit"
as a convenience for the user.

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
 Documentation/merge-options.txt |    6 ++++++
 builtin/merge.c                 |   14 ++++++++++++++
 t/t7600-merge.sh                |   15 +++++++++++++++
 3 files changed, 35 insertions(+), 0 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..815e151487 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -46,6 +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 option_edit = 0;
 static int fast_forward_only;
 static int allow_trivial = 1, have_message;
 static struct strbuf merge_msg;
@@ -190,6 +191,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,
@@ -1092,6 +1095,13 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		option_commit = 0;
 	}
 
+	/* if not committing, edit is nonsensical */
+	if (!option_commit)
+		option_edit = 0;
+	/* if editing, invoke 'git commit -e' after successful merge */
+	if (option_edit)
+		option_commit = 0;
+
 	if (!allow_fast_forward && fast_forward_only)
 		die(_("You cannot combine --no-ff with --ff-only."));
 
@@ -1447,6 +1457,10 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	}
 
 	if (merge_was_ok) {
+		if (option_edit) {
+			const char *args[] = {"commit", "-e", NULL};
+			return run_command_v_opt(args, RUN_GIT_CMD);
+		}
 		fprintf(stderr, _("Automatic merge went well; "
 			"stopped before committing as requested\n"));
 		return 0;
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.g00fdf

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

[Junio C Hamano]

Jay Soffian <jaysoffian@gmail.com> writes:

> Implement "git merge [-e|--edit]" as "git merge --no-commit && git commit"
> as a convenience for the user.
>
> Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
> ---
> ...
> @@ -1447,6 +1457,10 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>  	}
>  
>  	if (merge_was_ok) {
> +		if (option_edit) {
> +			const char *args[] = {"commit", "-e", NULL};
> +			return run_command_v_opt(args, RUN_GIT_CMD);
> +		}
>  		fprintf(stderr, _("Automatic merge went well; "
>  			"stopped before committing as requested\n"));
>  		return 0;


I wanted to like this approach, thinking this approach might be safer and
with the least chance of breaking other codepaths, but this feels like an
ugly hack.

Are we still honoring all the hooks "git merge" honors?  More importantly,
isn't this make it impossible for future maintainers of this command to
enhance the command by adding other hooks after the commit is made?

If we wanted to do this properly, we should update builtin/merge.c to call
launch_editor() before it runs commit_tree(), in a way similar to how
prepare_to_commit() in builtin/commit.c does so when e.g. "commit -m foo -e"
is run. An editmsg is prepared (you already have it in MERGE_MSG), the
editor is allowed to update it, and then the original code before such a
patch will run using the updated contents of MERGE_MSG. That way, the _only_
change in behaviour when "-e" is used is to let the user update the message
used in the commit log, and everything else would run exactly the same way
as if no "-e" was given, including the invocation of hooks.

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

[Jay Soffian]

On Fri, Oct 7, 2011 at 1:30 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jay Soffian <jaysoffian@gmail.com> writes:
>
>> Implement "git merge [-e|--edit]" as "git merge --no-commit && git commit"
>> as a convenience for the user.
>>
>> Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
>> ---
>> ...
>> @@ -1447,6 +1457,10 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>>       }
>>
>>       if (merge_was_ok) {
>> +             if (option_edit) {
>> +                     const char *args[] = {"commit", "-e", NULL};
>> +                     return run_command_v_opt(args, RUN_GIT_CMD);
>> +             }
>>               fprintf(stderr, _("Automatic merge went well; "
>>                       "stopped before committing as requested\n"));
>>               return 0;
>
>
> I wanted to like this approach, thinking this approach might be safer and
> with the least chance of breaking other codepaths, but this feels like an
> ugly hack.
>
> Are we still honoring all the hooks "git merge" honors?  More importantly,
> isn't this make it impossible for future maintainers of this command to
> enhance the command by adding other hooks after the commit is made?

Git is already inconsistent with respect to which hooks are called
when. Shouldn't post-merge be called on a merge commit regardless of
whether you use --no-commit or not? Well, it isn't, it's only called
when merge performs the commit internally. The post-merge hook was
probably a mistake -- git calls the post-commit hook passing the
context as an argument, so probably merge should just call post-commit
"merge". But that ship has sailed.

See also 65969d43d1 (merge: honor prepare-commit-msg hook, 2011-02-14).

> If we wanted to do this properly, we should update builtin/merge.c to call
> launch_editor() before it runs commit_tree(), in a way similar to how
> prepare_to_commit() in builtin/commit.c does so when e.g. "commit -m foo -e"
> is run. An editmsg is prepared (you already have it in MERGE_MSG), the
> editor is allowed to update it, and then the original code before such a
> patch will run using the updated contents of MERGE_MSG. That way, the _only_
> change in behaviour when "-e" is used is to let the user update the message
> used in the commit log, and everything else would run exactly the same way
> as if no "-e" was given, including the invocation of hooks.

I find git very difficult to reason about (and inconsistent in its
behavior) due to piecemeal hoisting of some functionality into
porcelain commands (another example, revert.c building in the
recursive merge strategy but not any others).

I actually think a better choice would be to remove commit_tree() from
merge and always have it run commit externally. I'm not seriously
suggesting that be done, but it would make git more consistent. But
I'm not going to send in a patch which makes the situation worse.

j.

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

[Junio C Hamano]

Jay Soffian <jaysoffian@gmail.com> writes:

> I actually think a better choice would be to remove commit_tree() from
> merge and always have it run commit externally. I'm not seriously
> suggesting that be done, but it would make git more consistent. But
> I'm not going to send in a patch which makes the situation worse.

Think about it. What I suggested does no way make the situation
worse. Your patch _does_ make it worse by changing the hook behaviour
between "merge -m 'foo'" vs "merge -m 'foo' -e".

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

[Jay Soffian]

On Fri, Oct 7, 2011 at 3:01 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Think about it. What I suggested does no way make the situation
> worse. Your patch _does_ make it worse by changing the hook behaviour
> between "merge -m 'foo'" vs "merge -m 'foo' -e"

I think it's arguable how -e should behave. With -e opening my editor,
now I really feel like I'm making a commit and would be surprised by
not having the various commit hooks run.

j.

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

[Jay Soffian]

On Fri, Oct 7, 2011 at 2:01 PM, Jay Soffian <jaysoffian@gmail.com> wrote:
> I actually think a better choice would be to remove commit_tree() from
> merge and always have it run commit externally. I'm not seriously
> suggesting that be done, but it would make git more consistent. But
> I'm not going to send in a patch which makes the situation worse.

The other inconsistencies I'm aware of between "merge --no-commit &&
commit" vs "merge" on a clean merge are:

* reflog
  - merge uses either "Merge made by the '...' strategy." OR "In-index merge"
  - commit uses "commit (merge) <subject>"

* hooks
  - merge calls
    1) "prepare-commit-msg MERGE_MSG merge"
    2) "post-merge [0|1]" where [0|1] indicates a squash or not.
  - commit calls
    1) "pre-commit"
    2) "prepare-commit-msg COMMIT_EDITMSG merge"
    3) "commit-msg COMMIT_EDITMSG"
    4) "post-commit"

* gc
  - merge calls "git gc --auto" after a successful merge unless
--squash was used
  - commit does not call "git gc --auto"

* diffstat: merge shows it, commit does not

j.

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

[Junio C Hamano]

Jay Soffian <jaysoffian@gmail.com> writes:

> The other inconsistencies I'm aware of between "merge --no-commit &&
> commit" vs "merge" on a clean merge are:

Perhaps you would want to add these to a list of todo items when gitwiki
comes back.

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

[Jay Soffian]

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

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
On Fri, Oct 7, 2011 at 1:30 PM, Junio C Hamano <gitster@pobox.com> wrote:
> If we wanted to do this properly, we should update builtin/merge.c to call
> launch_editor() before it runs commit_tree(), in a way similar to how

I disagree that this is the proper way to do it. --edit is a new option, there's
no obviously "correct" behavior. You think 'merge --edit' should behave just
like 'merge', I think 'merge --edit' should behave like 'merge --no-commit &&
commit'.

The commit performed internally by git-merge is already wildly inconsistent with
git-commit. If anything, --edit is a perfect excuse to say "we're trying to make
git-merge perform commits more consistently with git-commit, so we've
implemented 'merge --edit' in terms of git-commit."

I didn't bother with the commit status, it's more code than I wanted
to deal with duplicating/refactoring from commit.c.

 Documentation/merge-options.txt |    6 ++
 builtin/merge.c                 |  108 +++++++++++++++++++++++++--------------
 t/t7600-merge.sh                |   15 +++++
 3 files changed, 91 insertions(+), 38 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..0dee53b7e4 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -46,6 +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 option_edit = 0;
 static int fast_forward_only;
 static int allow_trivial = 1, have_message;
 static struct strbuf merge_msg;
@@ -190,6 +191,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 +845,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();
+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 +906,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 +934,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 +1042,36 @@ static int setup_with_upstream(const char ***argv)
 	return i;
 }
 
+static void write_merge_state()
+{
+	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 +1475,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.g3a3ce

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

[Junio C Hamano]

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".
>
> Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
> ---
> On Fri, Oct 7, 2011 at 1:30 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> If we wanted to do this properly, we should update builtin/merge.c to call
>> launch_editor() before it runs commit_tree(), in a way similar to how
>
> I disagree that this is the proper way to do it. --edit is a new option, there's
> no obviously "correct" behavior. You think 'merge --edit' should behave just
> like 'merge', I think 'merge --edit' should behave like 'merge --no-commit &&
> commit'.
>
> The commit performed internally by git-merge is already wildly inconsistent with
> git-commit.

Think and look forward.

You are complaining that the "commit" does not know enough to behave as if
it were a part of the merge command workflow if you split a usual merge
into two steps "merge --no-commit; commit".

How would you make it better? Would you strip all the things usual "merge"
does, so that it would work identically to the split one, losing some hook
support and such, or would you rather make the split case work similar to
the usual merge?

I'd say between "merge" and "merge --no-commit ; commit", the latter is
what needs to be fixed. Viewed that way, why would you even consider
making the new option behave similar to the _wrong_ one?

> I didn't bother with the commit status, it's more code than I wanted
> to deal with duplicating/refactoring from commit.c.

What do you mean by "commit status"? If you mean this patch is incomplete,
it would have been nicer if it were labeled with [PATCH/RFC].

> diff --git a/builtin/merge.c b/builtin/merge.c
> index ee56974371..0dee53b7e4 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -46,6 +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 option_edit = 0;

No need to move this into .data segment when it can be in .bss
segment. Drop the unnecessary " = 0" before ";".

> @@ -842,30 +845,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();

s/()/(void)/;

Thanks.

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

[Jay Soffian]

On Fri, Oct 7, 2011 at 6:15 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Think and look forward.
>
> You are complaining that the "commit" does not know enough to behave as if
> it were a part of the merge command workflow if you split a usual merge
> into two steps "merge --no-commit; commit".

No Junio, you have my argument completely reversed.

I am complaining that git-merge implements commits internally, which
gives it unique behavior from git-{commit/cherry-pick/revert} (the
latter two of which just run external git-commit). I'm saying merge is
fundamentally broken to do it this way. And maybe that's something
that should be fixed in 2.0 -- that git-merge should just call out to
git-commit, just like cherry-pick/revert do.

In case that's not clear: I think that git-merge should eventually
behave identically to "merge --no-commit; commit".

> How would you make it better? Would you strip all the things usual "merge"
> does, so that it would work identically to the split one,

Yes.

> losing some hook support and such.

Yes, I would lose the post-merge hook and such.

>, or would you rather make the split case work similar to the usual merge?

No, I would not do that.

BTW, the same arguments apply to git-am, which uses git-commit-tree,
and so implements its own set of hooks.

> I'd say between "merge" and "merge --no-commit ; commit", the latter is
> what needs to be fixed. Viewed that way, why would you even consider
> making the new option behave similar to the _wrong_ one?

Strongly disagree. I think it would make much more sense for all
commits to flow through git-commit, which would ensure consistent
behavior. I think we've got a mishmash of hooks which evolved over
time.

>> I didn't bother with the commit status, it's more code than I wanted
>> to deal with duplicating/refactoring from commit.c.
>
> What do you mean by "commit status"? If you mean this patch is incomplete,
> it would have been nicer if it were labeled with [PATCH/RFC].

No, I meant that git-commit includes status information about the
commit itself as comments in the commit message (git config
commit.status), and I didn't implement that. I don't think that makes
this patch incomplete however, that could be added by a later patch.

I'll send another iteration with your comments below addressed.

j.

>> diff --git a/builtin/merge.c b/builtin/merge.c
>> index ee56974371..0dee53b7e4 100644
>> --- a/builtin/merge.c
>> +++ b/builtin/merge.c
>> @@ -46,6 +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 option_edit = 0;
>
> No need to move this into .data segment when it can be in .bss
> segment. Drop the unnecessary " = 0" before ";".
>
>> @@ -842,30 +845,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();
>
> s/()/(void)/;
>
> Thanks.
>
>

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

[Junio C Hamano]

[Adding Ram to the Cc: list as this topic has much to do with resuming a
sequencer-driven workflow, and dropping Todd who does not seem to have
much to say on this topic]

Jay Soffian <jaysoffian@gmail.com> writes:

> I am complaining that git-merge implements commits internally, which
> gives it unique behavior from git-{commit/cherry-pick/revert} (the
> latter two of which just run external git-commit). I'm saying merge is
> fundamentally broken to do it this way. And maybe that's something
> that should be fixed in 2.0 -- that git-merge should just call out to
> git-commit, just like cherry-pick/revert do.

The purpose of the plumbing commands, e.g. commit-tree, is to implement a
unit of logical step at the mechanical level well without enforcing policy
decisions that may not apply to all situations.

On the other hand, the purpose of the Porcelain commands is to support a
concrete workflow element. "git commit" should support what users want to
happen when advancing the history by one commit, "git pull" should support
what users want to happen when integrating work done in another repository
to the history you are currently growing, etc. It is where we should and
do allow users to implement their own policy with hooks and configuration
variables when they want to, and it is even fine if we implemented sane
default policies with ways to override them (e.g. "commit --allow-empty",
"merge --no-ff").

Some Porcelain commands cannot complete their workflow element by
themselves in certain situations without getting help from users, and they
give control back to the user when they need such help. "git rebase", "git
am", "git merge", etc. can and do stop and ask the user to help resolving
conflicts.

The unfortunate historical accident that we may want to correct is that
some of these "we stopped in the middle and asked the user to help before
continuing" situation were presented as if "we stopped and aborted in the
middle, leaving the user to fix up the mess", which is a completely wrong
mental model. "Upon conflicts, 'git merge' stops in the middle, and you
complete it with 'git commit'" is a prime example of this. We even wrongly
label such a situation as "failed merge". It is not failed---it merely is
not auto-completed and waiting to be completed with user's help.

To understand why it is a wrong mental model, you need to imagine a world
where the logic to resolve conflicts in "git merge" is improved so that it
needs less help from the users. rerere.autoupdate is half-way there---the
user allows the merge machinery to take advantage of conflict resolutions
that the user has performed previously. Even though we currently do not
let "git merge" proceed to commit the result, it is entirely plausible to
go one step further and treat the resulting tree from applying the rerere
information as the result of the automerge. When that happens, it is very
natural for the user to expect that the rest of what "git merge" does for
a clean automerge to be carried out. After all, from the end user's point
of view, it _is_ a clean auto-merge. The only difference is how the user
helped the automerge machinery.

The root cause of the inconsistencies you are bringing up (which I agree
are annoying and I further agree that it is a worthy thing to address) is
that even though we tell the users "after helping the 'git merge', you
conclude it with 'git commit'", the concluding 'git commit' does _not_
perform what the user configured 'git merge' to do before a merge is
concluded, unlike a cleanly resolved 'git merge'.

This is merely an unfortunate historical accident. Because "git merge" did
not have any user configurable policy decisions (read: hooks) when this
"conclude with commit" was coded, "conclude with commit" was sufficient to
emulate the case where the merge did not need any help from the user.

But it no longer is true with modern Git.

With more recent changes, e.g. the sequencer work and "git cherry-pick"
that takes multiple commits, "conclude with commit" is becoming less and
less correct thing to say. The workflow elements these commands implement
do have "create a commit" as one essential part, but that is not the only
thing they do. If anything, I think the right way forward is to update the
UI with this rule for consistency:

  Some tools can stop in the middle when they cannot automatically compute
  the outcome, and give control back to the user asking for help. After
  helping these tool, the way to resume what was being done is to invoke
  the tool with the "--continue" option. All user level policy decisions
  implemented by hooks and configurations the tool normally obey when it
  does not need such help from the end user are obeyed when continuing.

I wouldn't mind if that is "invoke 'git continue' command", even though I
suspect that may make the implementation slightly more complex (I haven't
thought things through). "git commit" as a way to conclude a merge that
was stopped in the middle due to a conflict should be deprecated in the
longer term, like say in Git 2.0 someday.


[Footnote]

*1* By the way, "git merge --no-commit" is an oddball. It primarily is
used when the user does _not_ want the resulting commit but wants to
further modify the tree state (e.g. cherry picking a part of what was done
in the side branch). At the philosophical level, the user should be using
merge machinery at the "plumbing" level (e.g. merge-recursive backend),
but the interface to invoke the plumbing level merge machinery is so
arcane (they are after all designed for scripts not for humans) that
nobody does so in practice. And for that purpose, I think it is Ok for the
user to do anything after "git merge --no-commit" finishes (either leaving
conflicts or leaving a cleanly merged state), including "git commit".
Because that "git commit" is very different from the "conclude conflicted
merge with commit" which is a poor substitute for "git merge --continue"
in modern Git, I think it is perfectly fine and even preferable if it does
not obey any "git merge" semantics (i.e. user defined policy that pertains
to "merge" operations).

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> To understand why it is a wrong mental model, you need to imagine a world
> where the logic to resolve conflicts in "git merge" is improved so that it
> needs less help from the users. rerere.autoupdate is half-way there---the
> user allows the merge machinery to take advantage of conflict resolutions
> that the user has performed previously. Even though we currently do not
> let "git merge" proceed to commit the result, it is entirely plausible to
> go one step further and treat the resulting tree from applying the rerere
> information as the result of the automerge. When that happens, it is very
> natural for the user to expect that the rest of what "git merge" does for
> a clean automerge to be carried out. After all, from the end user's point
> of view, it _is_ a clean auto-merge. The only difference is how the user
> helped the automerge machinery.

Addendum.

I am not suggesting that we should change rerere.autoupdate to go all the
way and record a merge commit by default automatically when rerere applies
cleanly.

I personally think that it is a sensible default to set rerere.autoupdate
to false (or not to set the variable at all) to ensure that a merge that
conflicts is always inspected by the end user, given that rerere is merely
a heuristic (even though it is a damn good one) and produces a surprising
result.

But that is a policy preference; some people want to trust rerere more
than I do and that is a valid choice for them to make. To support such a
policy preference, I am perfectly fine with introducing a third value to
rerere.autoupdate in addition to yes/no to allow commands (e.g. "merge",
"am", etc.) to continue when rerere resolved conflicts cleanly in a
situation where they would have stopped and asked user to help resolving.

By the way, on the other side of this same coin lies another use case
(different from the one in the footnote in the previous message) for
"merge --no-commit". When you know that a particular merge _will_ need
semantic adjustments, even if it were to textually merge cleanly, you
would want the command to ask you for help to come up with the final tree,
instead of trusting the clean automerge result. This often happens when
the topic branch you are about to merge has changed the semantics of an
existing function (e.g. adding a new parameter) while the branch you are
on has added new callsite to the function (or the other way around). In
such a merge, you would need to adjust the new callsite that does not know
about the additional parameter to the new function signature.  For exactly
the same reason, it is not a kosher advice to give to users of modern Git
to "interfere with the merge with 'merge --no-commit', and then conclude
with 'commit'", as 'commit' has less information than 'merge' itself what
'merge' wants to do in addition to recording the result as a 'commit'.

Either the 'commit' command needs to detect that it is conclusing the
merge and trigger the merge hooks the same way as 'merge' itself does,
(which is a bad design, as 'commit' will need to know about the clean-up
operations of all the other commands that may ask users to help and let
'commit' conclude it), or the end user instruction needs to be updated so
that 'merge --continue' is used in such a situation to give 'merge' a
chance to finish up. Again we could have "git continue" wrapper that knows
how to tell what operation was in progress and invokes "merge --continue"
when it detects that it was a 'merge' that was in progress, but that is a
mere fluff.

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

[Jakub Narebski]

Junio C Hamano <gitster@pobox.com> writes:

> By the way, on the other side of this same coin lies another use case
> (different from the one in the footnote in the previous message) for
> "merge --no-commit". When you know that a particular merge _will_ need
> semantic adjustments, even if it were to textually merge cleanly, you
> would want the command to ask you for help to come up with the final tree,
> instead of trusting the clean automerge result. This often happens when
> the topic branch you are about to merge has changed the semantics of an
> existing function (e.g. adding a new parameter) while the branch you are
> on has added new callsite to the function (or the other way around). In
> such a merge, you would need to adjust the new callsite that does not know
> about the additional parameter to the new function signature.  For exactly
> the same reason, it is not a kosher advice to give to users of modern Git
> to "interfere with the merge with 'merge --no-commit', and then conclude
> with 'commit'", as 'commit' has less information than 'merge' itself what
> 'merge' wants to do in addition to recording the result as a 'commit'.

Yet another issue is if we should blindly trust automatic merge resolution.
It is considered a good practice by some to always check (e.g. by compiling
and possibly also running tests) the result of merge, whether it required
merge conflict resolution or not.

IIRC Linus lately said that making "git merge" automatically commit
was one of bad design decisions of git, for the above reason...

-- 
Jakub Narębski

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

[Matthieu Moy]

Jakub Narebski <jnareb@gmail.com> writes:

> Yet another issue is if we should blindly trust automatic merge resolution.
> It is considered a good practice by some to always check (e.g. by compiling
> and possibly also running tests) the result of merge, whether it required
> merge conflict resolution or not.

I agree that trusting merge blindly is bad, but still, if there are no
merge conflicts, and if the merge is broken, I'd prefer commiting a
fixup patch right after the merge than fixing it before committing.
Because if the merge needs a fix, it usually means something tricky that
deserves its own patch and commit message. At worse, one can still reset
--merge HEAD^.

One other issue with not committing automatically is for beginners. I
see that all the time when the merge has conflicts. newbies fix the
conflicts, and when they're done: "fine, conflicts solved, let's
continue hacking" without committing. The resulting history is totally
messy because it mixes merges and actual edits. For these users, not
committing automatically in the absence of conflict would make the
situation even worse.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

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

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> Yet another issue is if we should blindly trust automatic merge resolution.
> It is considered a good practice by some to always check (e.g. by compiling
> and possibly also running tests) the result of merge, whether it required
> merge conflict resolution or not.
>
> IIRC Linus lately said that making "git merge" automatically commit
> was one of bad design decisions of git, for the above reason...

I think your recalling this discussion

    http://thread.gmane.org/gmane.linux.kernel/1191100/focus=181362

While I agree with what Linus said in the message, I think you are not
remembering the discussion correctly. It was about bad commit _message_,
and an improvement is not to let users tweak a cleanly automerged result,
but is to allow users or force them to always write their own message,
perhaps with "merge --[no-]edit", which is exactly the point of Jay's
patch in this thread.