Git makes a merge commit but as a normal (non-merge) commit

While merging if I do certain actions then the merge commit is made
with the merge message but as a normal (non-merge) commit.

Repro steps:
- Set GIT_MERGE_AUTOEDIT=yes (set other than "no") in .bashrc
- Make a merge commit with no conflicts.
  (external text editor shows the generated merge message)
- Focus on Git Bash and Ctrl-C.
- Commit (git commit).

Actual behavior:
Git makes a normal (non-merge) commit (squash merge) but with the
merge commit message.

It looks like a bug to me. This is very confusing later on as the repo
topology would show that the branch is not merged in and there is not
an easy way to find out when the merge was made.

Expected behavior:
Git should stay in a MERGING state. The user can choose to either
abort the merge or continue the merge (git merge --continue OR git
commit).

This does not happen in case of conflicts (at least I'm not able to
repro). I get a (master|MERGING) prompt till I resolve the conflicts
and commit, which goes through correctly as a merge commit.

Environment:
$ git version
git version 2.14.0.windows.2
$ bash --version
GNU bash, version 4.4.12(1)-release (x86_64-pc-msys)

Thanks,
RM

hIpPy venit, vidit, dixit 19.08.2017 00:35:

While merging if I do certain actions then the merge commit is made
with the merge message but as a normal (non-merge) commit.

Repro steps:
- Set GIT_MERGE_AUTOEDIT=yes (set other than "no") in .bashrc
- Make a merge commit with no conflicts.
  (external text editor shows the generated merge message)
- Focus on Git Bash and Ctrl-C.
- Commit (git commit).

Actual behavior:
Git makes a normal (non-merge) commit (squash merge) but with the
merge commit message.

It looks like a bug to me. This is very confusing later on as the repo
topology would show that the branch is not merged in and there is not
an easy way to find out when the merge was made.

Expected behavior:
Git should stay in a MERGING state. The user can choose to either
abort the merge or continue the merge (git merge --continue OR git
commit).

This does not happen in case of conflicts (at least I'm not able to
repro). I get a (master|MERGING) prompt till I resolve the conflicts
and commit, which goes through correctly as a merge commit.

Environment:
$ git version
git version 2.14.0.windows.2
$ bash --version
GNU bash, version 4.4.12(1)-release (x86_64-pc-msys)

Thanks for the detailed info!

It turns out that his is not Windows specific. The recipe is:
- make "git merge" call and wait for an editor
- kill "git merge" while it waits for the editor

The effect is that prepare_to_commit() has no chance to call
abort_commit() any more (which would call write_merge_state())..

Now, I'm not sure why we only do half of write_merge_state() (write out
the merge_msg, but not merge_head) before launching the editor.

In any case, the following patch solves the problem and passes the
existing tests. But I'll wait for more comments (and think about a test
meanwhile).

Michael

If the `git merge` process is killed while waiting for the editor to
finish, the merge state is lost but the prepared merge msg and tree is kept.
So, a subsequent `git commit` creates a squashed merge even when the
user asked for proper merge commit originally.

Save the merge state earlier (in the non-squash case) so that it does
not get lost.

Reported-by: hIpPy <redacted>
Signed-off-by: Michael J Gruber <redacted>
---
 builtin/merge.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index cc57052993..5379b08824 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c

@@ -758,6 +758,7 @@ N_("Please enter a commit message to explain why this merge is necessary,\n"
    "Lines starting with '%c' will be ignored, and an empty message aborts\n"
    "the commit.\n");
 
+static void write_merge_heads(struct commit_list *);
 static void prepare_to_commit(struct commit_list *remoteheads)
 {
 	struct strbuf msg = STRBUF_INIT;

@@ -767,6 +768,8 @@ static void prepare_to_commit(struct commit_list *remoteheads)
 		strbuf_commented_addf(&msg, _(merge_editor_comment), comment_line_char);
 	if (signoff)
 		append_signoff(&msg, ignore_non_trailer(msg.buf, msg.len), 0);
+	if (!squash)
+		write_merge_heads(remoteheads);
 	write_file_buf(git_path_merge_msg(), msg.buf, msg.len);
 	if (run_commit_hook(0 < option_edit, get_index_file(), "prepare-commit-msg",
 			    git_path_merge_msg(), "merge", NULL))

@@ -908,7 +911,7 @@ static int setup_with_upstream(const char ***argv)
 	return i;
 }
 
-static void write_merge_state(struct commit_list *remoteheads)
+static void write_merge_heads(struct commit_list *remoteheads)
 {
 	struct commit_list *j;
 	struct strbuf buf = STRBUF_INIT;

@@ -924,8 +927,6 @@ static void write_merge_state(struct commit_list *remoteheads)
 		strbuf_addf(&buf, "%s\n", oid_to_hex(oid));
 	}
 	write_file_buf(git_path_merge_head(), buf.buf, buf.len);
-	strbuf_addch(&merge_msg, '\n');
-	write_file_buf(git_path_merge_msg(), merge_msg.buf, merge_msg.len);
 
 	strbuf_reset(&buf);
 	if (fast_forward == FF_NO)

@@ -933,6 +934,13 @@ static void write_merge_state(struct commit_list *remoteheads)
 	write_file_buf(git_path_merge_mode(), buf.buf, buf.len);
 }
 
+static void write_merge_state(struct commit_list *remoteheads)
+{
+	write_merge_heads(remoteheads);
+	strbuf_addch(&merge_msg, '\n');
+	write_file_buf(git_path_merge_msg(), merge_msg.buf, merge_msg.len);
+}
+
 static int default_edit_option(void)
 {
 	static const char name[] = "GIT_MERGE_AUTOEDIT";

-- 
2.14.1.364.ge466dba5f7

Currently, 'git merge --continue' is mentioned but not explained.

Explain it.

Signed-off-by: Michael J Gruber <redacted>
---
 Documentation/git-merge.txt | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index 6b308ab6d0..615e6bacde 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt

@@ -288,7 +288,10 @@ After seeing a conflict, you can do two things:
 
  * Resolve the conflicts.  Git will mark the conflicts in
    the working tree.  Edit the files into shape and
-   'git add' them to the index.  Use 'git commit' to seal the deal.
+   'git add' them to the index.  Use 'git commit' or
+   'git merge --continue' to seal the deal. The latter command
+   checks whether there is a (interrupted) merge in progress
+   before calling 'git commit'.
 
 You can work through the conflict with a number of tools:
 

-- 
2.14.1.364.ge466dba5f7

On 21 August 2017 at 14:53, Michael J Gruber [off-list ref] wrote:

There are actually two things going on here. First, this mentions git
merge --continue. Second, it explains what that command does. But the
latter is done earlier (not exactly like here, but still).

When git merge --continue originally appeared, this part of the docs was
discussed briefly. Maybe interesting:

https://public-inbox.org/git/xmqq60mn671x.fsf@gitster.mtv.corp.google.com/

Martin

Martin Ågren venit, vidit, dixit 21.08.2017 18:43:

On 21 August 2017 at 14:53, Michael J Gruber [off-list ref] wrote:

quoted

Currently, 'git merge --continue' is mentioned but not explained.

Explain it.

Signed-off-by: Michael J Gruber <redacted>
---
 Documentation/git-merge.txt | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index 6b308ab6d0..615e6bacde 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt

@@ -288,7 +288,10 @@ After seeing a conflict, you can do two things:

  * Resolve the conflicts.  Git will mark the conflicts in
    the working tree.  Edit the files into shape and
-   'git add' them to the index.  Use 'git commit' to seal the deal.
+   'git add' them to the index.  Use 'git commit' or
+   'git merge --continue' to seal the deal. The latter command
+   checks whether there is a (interrupted) merge in progress
+   before calling 'git commit'.

 You can work through the conflict with a number of tools:

There are actually two things going on here. First, this mentions git
merge --continue. Second, it explains what that command does. But the
latter is done earlier (not exactly like here, but still).

I didn't see that explained in the man page at all - on the contrary, I
only saw a forward reference (see section...), but then only an
explanation of what "resolving" means (including the "git commit"-step).
It is unclear to me from the man page which steps of "resolving" the
command "git merge --continue" does - you could think it does "git
commit -a", for example.

When git merge --continue originally appeared, this part of the docs was
discussed briefly. Maybe interesting:

https://public-inbox.org/git/xmqq60mn671x.fsf@gitster.mtv.corp.google.com/

Martin

On 22 August 2017 at 11:26, Michael J Gruber [off-list ref] wrote:

Martin Ågren venit, vidit, dixit 21.08.2017 18:43:

quoted

On 21 August 2017 at 14:53, Michael J Gruber [off-list ref] wrote:

quoted

Currently, 'git merge --continue' is mentioned but not explained.

Explain it.

Signed-off-by: Michael J Gruber <redacted>
---
 Documentation/git-merge.txt | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index 6b308ab6d0..615e6bacde 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt

@@ -288,7 +288,10 @@ After seeing a conflict, you can do two things:

  * Resolve the conflicts.  Git will mark the conflicts in
    the working tree.  Edit the files into shape and
-   'git add' them to the index.  Use 'git commit' to seal the deal.
+   'git add' them to the index.  Use 'git commit' or
+   'git merge --continue' to seal the deal. The latter command
+   checks whether there is a (interrupted) merge in progress
+   before calling 'git commit'.

 You can work through the conflict with a number of tools:

There are actually two things going on here. First, this mentions git
merge --continue. Second, it explains what that command does. But the
latter is done earlier (not exactly like here, but still).

I didn't see that explained in the man page at all - on the contrary, I
only saw a forward reference (see section...), but then only an
explanation of what "resolving" means (including the "git commit"-step).
It is unclear to me from the man page which steps of "resolving" the
command "git merge --continue" does - you could think it does "git
commit -a", for example.

That's very true, and your change helps immensely. I thought that once
git merge --continue was mentioned, e.g.,

        Use 'git commit' or 'git merge --continue' to seal the deal.

or

        Use 'git commit' to conclude (you can also say 'git merge
        --continue').

then things are in some sense "complete". But you might be right that
further stressing that the latter is basically an alias helps avoid some
confusion. "Oh, great, so now I have two commands to choose from -- which
one should I be using?" :-)

Martin

I think 'git merge --continue' should be advertised more that 'git
commit' as typically one is familiar with 'git rebase --continue' and
'git cherry-pick --continue'. I for a long time did not know I could
also use 'git commit' to continue a merge but that's just me. Now,
'git commit' is easier to remember if it works in all cases (merge,
rebase, cherry-pick).

RM


On Tue, Aug 22, 2017 at 3:06 AM, Martin Ågren [off-list ref] wrote:

On 22 August 2017 at 11:26, Michael J Gruber [off-list ref] wrote:

quoted

Martin Ågren venit, vidit, dixit 21.08.2017 18:43:

quoted

On 21 August 2017 at 14:53, Michael J Gruber [off-list ref] wrote:

quoted

Currently, 'git merge --continue' is mentioned but not explained.

Explain it.

Signed-off-by: Michael J Gruber <redacted>
---
 Documentation/git-merge.txt | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index 6b308ab6d0..615e6bacde 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt

@@ -288,7 +288,10 @@ After seeing a conflict, you can do two things:

  * Resolve the conflicts.  Git will mark the conflicts in
    the working tree.  Edit the files into shape and
-   'git add' them to the index.  Use 'git commit' to seal the deal.
+   'git add' them to the index.  Use 'git commit' or
+   'git merge --continue' to seal the deal. The latter command
+   checks whether there is a (interrupted) merge in progress
+   before calling 'git commit'.

 You can work through the conflict with a number of tools:

There are actually two things going on here. First, this mentions git
merge --continue. Second, it explains what that command does. But the
latter is done earlier (not exactly like here, but still).

I didn't see that explained in the man page at all - on the contrary, I
only saw a forward reference (see section...), but then only an
explanation of what "resolving" means (including the "git commit"-step).
It is unclear to me from the man page which steps of "resolving" the
command "git merge --continue" does - you could think it does "git
commit -a", for example.

That's very true, and your change helps immensely. I thought that once
git merge --continue was mentioned, e.g.,

        Use 'git commit' or 'git merge --continue' to seal the deal.

or

        Use 'git commit' to conclude (you can also say 'git merge
        --continue').

then things are in some sense "complete". But you might be right that
further stressing that the latter is basically an alias helps avoid some
confusion. "Oh, great, so now I have two commands to choose from -- which
one should I be using?" :-)

Martin

If the `git merge` process is killed while waiting for the editor to
finish, the merge state is lost but the prepared merge msg and tree is kept.
So, a subsequent `git commit` creates a squashed merge even when the
user asked for proper merge commit originally.

Save the merge state earlier (in the non-squash case) so that it does
not get lost.

Reported-by: hIpPy <redacted>
Signed-off-by: Michael J Gruber <redacted>
---
 builtin/merge.c  |  3 +++
 t/t7600-merge.sh | 15 +++++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/builtin/merge.c b/builtin/merge.c
index 86f0adde3b..5379b08824 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c

@@ -758,6 +758,7 @@ N_("Please enter a commit message to explain why this merge is necessary,\n"
    "Lines starting with '%c' will be ignored, and an empty message aborts\n"
    "the commit.\n");
 
+static void write_merge_heads(struct commit_list *);
 static void prepare_to_commit(struct commit_list *remoteheads)
 {
 	struct strbuf msg = STRBUF_INIT;

@@ -767,6 +768,8 @@ static void prepare_to_commit(struct commit_list *remoteheads)
 		strbuf_commented_addf(&msg, _(merge_editor_comment), comment_line_char);
 	if (signoff)
 		append_signoff(&msg, ignore_non_trailer(msg.buf, msg.len), 0);
+	if (!squash)
+		write_merge_heads(remoteheads);
 	write_file_buf(git_path_merge_msg(), msg.buf, msg.len);
 	if (run_commit_hook(0 < option_edit, get_index_file(), "prepare-commit-msg",
 			    git_path_merge_msg(), "merge", NULL))

diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index 2ebda509ac..80194b79f9 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh

@@ -774,4 +774,19 @@ test_expect_success 'merge can be completed with --continue' '
 	verify_parents $c0 $c1
 '
 
+write_script .git/FAKE_EDITOR <<EOF
+# kill -TERM command added below.
+EOF
+
+test_expect_success EXECKEEPSPID 'killed merge can be completed with --continue' '
+	git reset --hard c0 &&
+	! "$SHELL_PATH" -c '\''
+	  echo kill -TERM $$ >> .git/FAKE_EDITOR
+	  GIT_EDITOR=.git/FAKE_EDITOR
+	  export GIT_EDITOR
+	  exec git merge --no-ff --edit c1'\'' &&
+	git merge --continue &&
+	verify_parents $c0 $c1
+'
+
 test_done

-- 
2.14.1.364.ge466dba5f7

So here's a little series.

1/3 I just noted along the way

2/3 splits the merge state writing into parts as a preparatory step.
This is something we may want to carry further and get rid of some other
places which write merge_msg unneccessarily so that merge performance
does not suffer because of the additional write.

3/3 comes with a test now. It's crafted after t7502 and does not work
under MINGW, unfortunately. But the fix does :)

I'm still asking for comments whether this has other side effects. I've
been told there are people who do a lot of merges regularly, and they're
not the kind of people that I'd like to make angry - at least not
without a good reason.

Michael J Gruber (3):
  Documentation/git-merge: explain --continue
  merge: split write_merge_state in two
  merge: save merge state earlier

 Documentation/git-merge.txt |  5 ++++-
 builtin/merge.c             | 14 +++++++++++---
 t/t7600-merge.sh            | 15 +++++++++++++++
 3 files changed, 30 insertions(+), 4 deletions(-)

-- 
2.14.1.364.ge466dba5f7

write_merge_state() writes out the merge heads, mode, and msg. But we
may want to write out heads, mode without the msg. So, split out heads
(+mode) into a separate function write_merge_heads() that is called by
write_merge_state().

No funtional change so far.

Signed-off-by: Michael J Gruber <redacted>
---
 builtin/merge.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index cc57052993..86f0adde3b 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c

@@ -908,7 +908,7 @@ static int setup_with_upstream(const char ***argv)
 	return i;
 }
 
-static void write_merge_state(struct commit_list *remoteheads)
+static void write_merge_heads(struct commit_list *remoteheads)
 {
 	struct commit_list *j;
 	struct strbuf buf = STRBUF_INIT;

@@ -924,8 +924,6 @@ static void write_merge_state(struct commit_list *remoteheads)
 		strbuf_addf(&buf, "%s\n", oid_to_hex(oid));
 	}
 	write_file_buf(git_path_merge_head(), buf.buf, buf.len);
-	strbuf_addch(&merge_msg, '\n');
-	write_file_buf(git_path_merge_msg(), merge_msg.buf, merge_msg.len);
 
 	strbuf_reset(&buf);
 	if (fast_forward == FF_NO)

@@ -933,6 +931,13 @@ static void write_merge_state(struct commit_list *remoteheads)
 	write_file_buf(git_path_merge_mode(), buf.buf, buf.len);
 }
 
+static void write_merge_state(struct commit_list *remoteheads)
+{
+	write_merge_heads(remoteheads);
+	strbuf_addch(&merge_msg, '\n');
+	write_file_buf(git_path_merge_msg(), merge_msg.buf, merge_msg.len);
+}
+
 static int default_edit_option(void)
 {
 	static const char name[] = "GIT_MERGE_AUTOEDIT";

-- 
2.14.1.364.ge466dba5f7