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:
Quoted 27 lines
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.
Quoted 5 lines
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 1/1 merge: save merge state earlier
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.
--- builtin/merge.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)
builtin/merge.c +11 -3
@@ -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" ;
Signature · 1 line
2.14.1.364.ge466dba5f7
PATCH v2 1/3 Documentation/git-merge: explain --continue
Currently, 'git merge --continue' is mentioned but not explained.
Explain it.
--- Documentation/git-merge.txt | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
Documentation/git-merge.txt +4 -1
@@ -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:
Signature · 1 line
2.14.1.364.ge466dba5f7
Re: [PATCH v2 1/3] Documentation/git-merge: explain --continue
Quoted 14 lines · Michael J Gruber
Currently, 'git merge --continue' is mentioned but not explained.
Explain it.
Signed-off-by Michael J Gruber <[email protected]>--- Documentation/git-merge.txt | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)Documentation/git-merge.txt +0
-0
Documentation/git-merge.txt After seeing a conflict, you can do two things: · line 296
Quoted 10 lines
Documentation/git-merge.txt +4
-1@@ -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/[email protected]/
Martin
Re: [PATCH v2 1/3] Documentation/git-merge: explain --continue
Martin Ågren venit, vidit, dixit 21.08.2017 18:43:
Quoted 24 lines · Michael J Gruber
Quoted 23 lines
Currently, 'git merge --continue' is mentioned but not explained.
Explain it.
Signed-off-by Michael J Gruber <[email protected]>--- Documentation/git-merge.txt | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)Documentation/git-merge.txt +4
-1@@ -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'.
Quoted 5 lines
Documentation/git-merge.txt After seeing a conflict, you can do two things: · line 296
Quoted 1 line
Documentation/git-merge.txt +0
-0You 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.
Quoted 7 lines
When git merge --continue originally appeared, this part of the docs was discussed briefly. Maybe interesting:
https://public-inbox.org/git/[email protected]/
Martin
Re: [PATCH v2 1/3] Documentation/git-merge: explain --continue
Quoted 31 lines · Michael J Gruber
Martin Ågren venit, vidit, dixit 21.08.2017 18:43:
Quoted 24 lines · Michael J Gruber
Quoted 23 lines
Currently, 'git merge --continue' is mentioned but not explained.
Explain it.
Signed-off-by Michael J Gruber <[email protected]>--- Documentation/git-merge.txt | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)Documentation/git-merge.txt +4
-1@@ -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'.Quoted 5 lines
Documentation/git-merge.txt After seeing a conflict, you can do two things: · line 296
Quoted 1 line
Documentation/git-merge.txt +0
-0You 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).
Quoted 6 lines
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
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
Quoted 55 lines · Martin Ågren
Quoted 31 lines · Michael J Gruber
Martin Ågren venit, vidit, dixit 21.08.2017 18:43:
Quoted 24 lines · Michael J Gruber
Quoted 23 lines
Currently, 'git merge --continue' is mentioned but not explained.
Explain it.
Signed-off-by Michael J Gruber <[email protected]>--- Documentation/git-merge.txt | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)Documentation/git-merge.txt +4
-1@@ -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'.Quoted 5 lines
Documentation/git-merge.txt After seeing a conflict, you can do two things: · line 296
Quoted 1 line
Documentation/git-merge.txt +0
-0You 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).
Quoted 6 lines
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
Quoted 4 lines · 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.
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 1/4 Documentation/git-merge: explain --continue
Currently, 'git merge --continue' is mentioned but not explained.
Explain it.
--- Documentation/git-merge.txt | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
Documentation/git-merge.txt +4 -1
@@ -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:
Signature · 1 line
2.14.1.426.g4352aa77a5
v3 cover Keep merge during kills
In reply to Junio C HamanoCompared 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(-)
Signature · 1 line
2.14.1.426.g4352aa77a5
PATCH v3 2/4 merge: clarify call chain
In reply to Junio C Hamanoprepare_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.
--- builtin/merge.c | 2 ++ 1 file changed, 2 insertions(+)
builtin/merge.c +2 -0
@@ -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)
Signature · 1 line
2.14.1.426.g4352aa77a5
PATCH v3 3/4 merge: split write_merge_state in two
In reply to Junio C Hamanowrite_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.
--- builtin/merge.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
builtin/merge.c +8 -3
@@ -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" ;
Signature · 1 line
2.14.1.426.g4352aa77a5
PATCH v3 4/4 merge: save merge state earlier
In reply to Junio C HamanoIf 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.
--- builtin/merge.c | 2 ++ t/t7600-merge.sh | 15 +++++++++++++++ 2 files changed, 17 insertions(+)
builtin/merge.c +2 -0
@@ -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 ))
t/t7600-merge.sh +15 -0
@@ -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
Signature · 1 line
2.14.1.426.g4352aa77a5
Re: [PATCH v2 1/3] Documentation/git-merge: explain --continue
In reply to Michael J GruberQuoted 14 lines · Michael J Gruber
Currently, 'git merge --continue' is mentioned but not explained.
Explain it.
Signed-off-by Michael J Gruber <[email protected]>--- Documentation/git-merge.txt | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)Documentation/git-merge.txt +0
-0
Documentation/git-merge.txt After seeing a conflict, you can do two things: · line 296
Quoted 10 lines
Documentation/git-merge.txt +4
-1@@ -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.
PATCH v2 2/3 merge: split write_merge_state in two
In reply to Michael J Gruberwrite_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.
--- builtin/merge.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
builtin/merge.c +8 -3
@@ -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" ;
Signature · 1 line
2.14.1.364.ge466dba5f7
Re: [PATCH v2 2/3] merge: split write_merge_state in two
Quoted 6 lines · 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 functionwrite_merge_heads()that is called bywrite_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.
Quoted 42 lines
Signed-off-by Michael J Gruber <[email protected]>--- builtin/merge.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)builtin/merge.c +8
-3@@ -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" ;
v2 cover Keep merge during kills
In reply to Michael J GruberSo 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(-)
Signature · 1 line
2.14.1.364.ge466dba5f7
PATCH v2 3/3 merge: save merge state earlier
In reply to Michael J GruberIf 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.
--- builtin/merge.c | 3 +++ t/t7600-merge.sh | 15 +++++++++++++++ 2 files changed, 18 insertions(+)
builtin/merge.c +3 -0
@@ -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 ))
t/t7600-merge.sh +15 -0
@@ -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
Signature · 1 line
2.14.1.364.ge466dba5f7
Re: [PATCH v2 3/3] merge: save merge state earlier
builtin/merge.c static void prepare_to_commit(struct commit_list *remoteheads) · line 775
Quoted 12 lines · Michael J Gruber
static void
prepare_to_commit(struct commit_list *remoteheads){builtin/merge.c +2
-0struct 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.
Quoted 9 lines
t/t7600-merge.sh +1
-0@@ -774,4 +774,19 @@ test_expect_success 'merge can be completed with --continue' ' verify_parents $c0 $c1 ' +write_script .git/FAKE_EDITOR <<EOF
t/t7600-merge.sh test_expect_success 'merge can be completed with --continue' ' · line 787
Quoted 10 lines
t/t7600-merge.sh +10
-0+# 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.
Quoted 5 lines
t/t7600-merge.sh +4
-0+ git merge --continue && + verify_parents $c0 $c1 +' + test_done
Re: [PATCH v2 3/3] merge: save merge state earlier
Junio C Hamano venit, vidit, dixit 22.08.2017 02:38:
Quoted 13 lines
Quoted 11 lines · Michael J Gruber
static void
prepare_to_commit(struct commit_list *remoteheads){builtin/merge.c +2
-0struct 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",
Quoted 8 lines
builtin/merge.c static void prepare_to_commit(struct commit_list *remoteheads) · line 775
Quoted 1 line
builtin/merge.c +0
-0git_path_merge_msg(), "merge", NULL))I can understand that you would never want to write out
MERGE_HEADwhile squashing, but I somehow think it would be a bug in the caller to callprepare_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".
Quoted 11 lines
Can squash ever be true in this function?
This function has two callsites:
merge_trivial()andfinish_automerge().I think
merge_trivial()will not be called under "--squash", which turnsoption_commitoff and the only callsite of it is inside an else-if clause that requresoption_committo be true. You can do a similar deduction around the "automerge_was_ok" variable to see iffinish_automerge()can be called when "--squash" is given; I suspect the answer may be no.
I'll go without the if, after more testing.
Quoted 8 lines
Quoted 8 lines
t/t7600-merge.sh +0
-0@@ -774,4 +774,19 @@ test_expect_success 'merge can be completed with --continue' ' verify_parents $c0 $c1 '
Quoted 15 lines
t/t7600-merge.sh test_expect_success 'merge can be completed with --continue' ' · line 787
Quoted 11 lines
t/t7600-merge.sh +11
-0+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.
Quoted 5 lines
Quoted 5 lines
t/t7600-merge.sh +4
-0+ git merge --continue && + verify_parents $c0 $c1 +' + test_done
Re: [PATCH v2 3/3] merge: save merge state earlier
Quoted 5 lines · Michael J Gruber
Quoted 5 lines
Can squash ever be true in this function?
This function has two callsites:
merge_trivial()andfinish_automerge().
Quoted 8 lines
Quoted 6 lines
I think
merge_trivial()will not be called under "--squash", which turnsoption_commitoff and the only callsite of it is inside an else-if clause that requresoption_committo be true. You can do a similar deduction around the "automerge_was_ok" variable to see iffinish_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.