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)

> Currently, 'git merge --continue' is mentioned but not explained.
>
> Explain it.
>
> Signed-off-by: Michael J Gruber <git@grubix.eu>
> ---
>  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:

> On 21 August 2017 at 14:53, Michael J Gruber <git@grubix.eu> wrote:
>> Currently, 'git merge --continue' is mentioned but not explained.
>>
>> Explain it.
>>
>> Signed-off-by: Michael J Gruber <git@grubix.eu>
>> ---
>>  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).

> 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 <git@grubix.eu> wrote:
>>> Currently, 'git merge --continue' is mentioned but not explained.
>>>
>>> Explain it.
>>>
>>> Signed-off-by: Michael J Gruber <git@grubix.eu>
>>> ---
>>>  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.

> On 22 August 2017 at 11:26, Michael J Gruber <git@grubix.eu> wrote:
>> Martin Ågren venit, vidit, dixit 21.08.2017 18:43:
>>> On 21 August 2017 at 14:53, Michael J Gruber <git@grubix.eu> wrote:
>>>> Currently, 'git merge --continue' is mentioned but not explained.
>>>>
>>>> Explain it.
>>>>
>>>> Signed-off-by: Michael J Gruber <git@grubix.eu>
>>>> ---
>>>>  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

> Currently, 'git merge --continue' is mentioned but not explained.
>
> Explain it.
>
> Signed-off-by: Michael J Gruber <git@grubix.eu>
> ---
>  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:

>  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

> Michael J Gruber <git@grubix.eu> writes:
> 
>>  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))
> 
> I can understand that you would never want to write out MERGE_HEAD
> while squashing, but I somehow think it would be a bug in the caller
> to call prepare_to_commit(), whose point is to prepare the merge
> message to be recorded in the resulting merge commit, when the user
> gave us the "--squash" option, which is an explicit instruction that
> the user does not want the merge commit the message is used.

> Can squash ever be true in this function?
> 
> This function has two callsites: merge_trivial() and
> finish_automerge().
> 
> I think merge_trivial() will not be called under "--squash", which
> turns option_commit off and the only callsite of it is inside an
> else-if clause that requres option_commit to be true.  You can do a
> similar deduction around the "automerge_was_ok" variable to see if
> finish_automerge() can be called when "--squash" is given; I suspect
> the answer may be no.

>> 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'\'' &&
> 
> This is a tricky construct.  You "reserve" a process ID by using a
> shell, arrange it to be killed and then using "exec" to make it the
> "git merge" program to be killed.  I kind of like the convolutedness.

>> +	git merge --continue &&
>> +	verify_parents $c0 $c1
>> +'
>> +
>>  test_done

>> Can squash ever be true in this function?
>> 
>> This function has two callsites: merge_trivial() and
>> finish_automerge().
>> 
>> I think merge_trivial() will not be called under "--squash", which
>> turns option_commit off and the only callsite of it is inside an
>> else-if clause that requres option_commit to be true.  You can do a
>> similar deduction around the "automerge_was_ok" variable to see if
>> finish_automerge() can be called when "--squash" is given; I suspect
>> the answer may be no.
>
> I'll go without the if, after more testing.

> 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 <git@grubix.eu>
> ---
>  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";