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

hIpPy [off-list ref] 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.

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

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

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

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

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 <redacted>
Signed-off-by: Michael J Gruber <redacted>
---
 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