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

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

[hIpPy]

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

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

[Michael J Gruber]

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

[PATCH] merge: save merge state earlier

[Michael J Gruber]

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 <hippy2981@gmail.com>
Signed-off-by: Michael J Gruber <git@grubix.eu>
---
 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

[PATCH v2 0/3] Keep merge during kills

[Michael J Gruber]

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

[PATCH v2 1/3] Documentation/git-merge: explain --continue

[Michael J Gruber]

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:
 
-- 
2.14.1.364.ge466dba5f7

[PATCH v2 2/3] merge: split write_merge_state in two

[Michael J Gruber]

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";
-- 
2.14.1.364.ge466dba5f7

[PATCH v2 3/3] merge: save merge state earlier

[Michael J Gruber]

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 <hippy2981@gmail.com>
Signed-off-by: Michael J Gruber <git@grubix.eu>
---
 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

Re: [PATCH v2 1/3] Documentation/git-merge: explain --continue

[Martin Ågren]

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

Re: [PATCH v2 2/3] merge: split write_merge_state in two

[Junio C Hamano]

Michael J Gruber <git@grubix.eu> writes:

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

"We may want to" leaves readers in suspense.  Hopefully 3/3 makes it
clear why we may in what situation.

One thing this changes is that we used to write merge-heads first,
then message and then finally mode, but now we write merge-heads,
then mode and finally message.  So killing us in the middle may
leave different set of files on the filesystem.  It is unclear from
the above description and the patch text if that is a potential
issue.

Let's keep reading ;-)

Thanks.

>
> 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";

Re: [PATCH v2 1/3] Documentation/git-merge: explain --continue

[Junio C Hamano]

Michael J Gruber <git@grubix.eu> writes:

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

Thanks, looks good.  Will queue.

Re: [PATCH v2 3/3] merge: save merge state earlier

[Junio C Hamano]

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

Re: [PATCH v2 1/3] Documentation/git-merge: explain --continue

[Michael J Gruber]

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.

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

Re: [PATCH v2 3/3] merge: save merge state earlier

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 22.08.2017 02:38:
> 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.

That sounds reasonable. I vaguely remember a failing test for an earlier
version that I tried, but that was before the "split".

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

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

That is from t7502. Sorry for hiding that note in the cover letter, I
should put it into 3/3's message or a test comment.

When testing, I simply used "git merge... &" and "ps" or "jobs" to know
which process to kill. Apparantly, the above is the most portable way to
script that. t7502 went through a few iterations to ensure this.

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

Re: [PATCH v2 1/3] Documentation/git-merge: explain --continue

[Martin Ågren]

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

Re: [PATCH v2 1/3] Documentation/git-merge: explain --continue

[hIpPy]

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 <martin.agren@gmail.com> wrote:
> 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

Re: [PATCH v2 3/3] merge: save merge state earlier

[Junio C Hamano]

Michael J Gruber <michael@grubix.eu> writes:

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

I was sort of expecting that tracing the control flow would give us
the definite answer and that would be much better than any amount of
testing.

In any case, I wasn't even suggesting to remove "if".  It might even
be worth doing

	if (squash)
		BUG("the control must not reach here under --squash");
	write_emrge_heads(...);

if we know the control does not have to reach with "--squash" in
today's code, so that future careless refactoring does not break
this fix.

Re: [PATCH v2 1/3] Documentation/git-merge: explain --continue

[Junio C Hamano]

hIpPy <hippy2981@gmail.com> writes:

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

Perhaps.  "rebase" and "am" (and range pick "cherry-pick A..B") are
operations that works on more than one change, so it makes perfect
sense to have a way to say "I am done with this step. Please go on
and do the rest".

There is no "go on and do the rest" after resolving a conflicted
merge, as "merge" is, unlike the other ones to which "--continue"
legitimately has a reasonable meaning, an operation that does just
one thing.  From that point of view, "git merge --continue" is a
mistaken UI that shouldn't have happened.

[PATCH v3 0/4] Keep merge during kills

[Michael J Gruber]

Compared to the 3-item v2:

1/4 == 1/3

2/4 is new as per Junio's suggestion: clarify the call-chain leading up
to prepare_to_commit()

3/4 == 2/3 with amended subject

4/4 is 3/3 rebased, squash-if removed

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

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

-- 
2.14.1.426.g4352aa77a5

[PATCH v3 1/4] Documentation/git-merge: explain --continue

[Michael J Gruber]

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:
 
-- 
2.14.1.426.g4352aa77a5

[PATCH v3 2/4] merge: clarify call chain

[Michael J Gruber]

prepare_to_commit() cannot be reached in the non-squash case:
It is called by merge_trivial() and finish_automerge() only, but the
calls to the latter are somewhat hard to track:

If option_commit is not set, the code in cmd_merge() uses a fake
conflict return code (ret=1) to avoid writing the tree, which also
avoids setting automerge_was_ok (just as in the proper ret==1 case), so
that finish_automerge() is not called.

To ensure that no code change breaks that assumption, safe-guard
prepare_to_commit() by a BUG() statement.

Suggested-by: junio
Signed-off-by: Michael J Gruber <git@grubix.eu>
---
 builtin/merge.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/builtin/merge.c b/builtin/merge.c
index cc57052993..dafec80fa9 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -763,6 +763,8 @@ static void prepare_to_commit(struct commit_list *remoteheads)
 	struct strbuf msg = STRBUF_INIT;
 	strbuf_addbuf(&msg, &merge_msg);
 	strbuf_addch(&msg, '\n');
+	if (squash)
+		BUG("the control must not reach here under --squash");
 	if (0 < option_edit)
 		strbuf_commented_addf(&msg, _(merge_editor_comment), comment_line_char);
 	if (signoff)
-- 
2.14.1.426.g4352aa77a5

[PATCH v3 3/4] merge: split write_merge_state in two

[Michael J Gruber]

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, except when these non-atomic writes are
interrupted: we write heads-mode-msg now when we used to write
heads-msg-mode.

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 dafec80fa9..db3335b3bf 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -910,7 +910,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;
@@ -926,8 +926,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)
@@ -935,6 +933,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.426.g4352aa77a5

[PATCH v3 4/4] merge: save merge state earlier

[Michael J Gruber]

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.

Demonstrate the problem with a test crafted after the in t7502. The test
requires EXECKEEPSPID (thus does not run under MINGW).

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

Reported-by: hIpPy <hippy2981@gmail.com>
Signed-off-by: Michael J Gruber <git@grubix.eu>
---
 builtin/merge.c  |  2 ++
 t/t7600-merge.sh | 15 +++++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/builtin/merge.c b/builtin/merge.c
index db3335b3bf..2d5c45a904 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;
@@ -769,6 +770,7 @@ 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);
+	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.426.g4352aa77a5