Commit message not helpful after merge squash with conflicts
Hi,
after a "git merge --squash" with a conflict the commit message is not helpful as it only includes the conflicted files information, however, I expect to see the content of SQUASH_MSG which contains the summary of the merged commits. SQUASH_MSG seems to be just ignored.
I think git should either read SQUASH_MSG and append COMMIT_MSG or add the conflict information into SQUASH_MSG and create no COMMIT_MSG.
References:
*
https://stackoverflow.com/questions/3605385/merge-conflicts-ruin-my-commit-message-while-squashing-commits
* https://gitlab.com/tortoisegit/tortoisegit/issues/1902
Signature · 3 lines
Best regards, Sven Strickroth PGP key id F5A9D4C4 @ any key-server
PATCH 1/1 Also read SQUASH_MSG if a conflict on a merge squash occurred
After a merge --squash with a conflict the commit message did not contain the information about the squashed commits, but only the "# Conflicts:" information.
--- builtin/commit.c | 6 ++++++ 1 file changed, 6 insertions(+)
builtin/commit.c +6 -0
@@ -729,6 +729,12 @@ static int prepare_to_commit(const char *index_file, const char *prefix,if (strbuf_read_file (&sb,git_path_merge_msg (),0 ) <0 )die_errno (_ ("could not read MERGE_MSG" )); hook_arg1 ="merge" ; +/* append SQUASH_MSG here if it exists and a merge --squash was originally performed */ +if (!stat (git_path_squash_msg (), &statbuf)) { +if (strbuf_read_file (&sb,git_path_squash_msg (),0 ) <0 ) +die_errno (_ ("could not read SQUASH_MSG" )); + hook_arg1 ="squash" ; + } }else if (!stat (git_path_squash_msg (), &statbuf)) {if (strbuf_read_file (&sb,git_path_squash_msg (),0 ) <0 )die_errno (_ ("could not read SQUASH_MSG" ));
Signature · 3 lines
Best regards, Sven Strickroth PGP key id F5A9D4C4 @ any key-server
Re: [PATCH] Also read SQUASH_MSG if a conflict on a merge squash occurred
Quoted 1 line · Sven Strickroth
Subject: Re: [PATCH] Also read
SQUASH_MSGif a conflict on a merge squash occurred
A reader sees this line in the output of "git shortlog --no-merges"; does it sufficiently tell her which Git subcommand is affected by this change, if this is a bugfix or a new feature, i.e. enough for her to decide how important the change is?
We often prefix our log message with the name of the area followed by a colon and describe the purpose of the change, not the means how the objective is achieved, e.g.
Subject: [PATCH] commit: do not lose SQUASH_MSG contents
When concluding a conflicted "git merge --squash", the command failed to read SQUASH_MSG that was prepared by "git merge", and showed only the "# Conflicts:" list of conflicted paths.
builtin/commit.c static int prepare_to_commit(const char *index_file, const char *prefix, · line 732
Quoted 9 lines
builtin/commit.c +1
-0@@ -729,6 +729,12 @@ static int prepare_to_commit(const char *index_file, const char *prefix,if (strbuf_read_file (&sb,git_path_merge_msg (),0 ) <0 )die_errno (_ ("could not read MERGE_MSG" )); hook_arg1 ="merge" ; +/* append SQUASH_MSG here if it exists and a merge --squash was originally performed */
/*
* Our multi-line comment reads more like
* this. That is, the first slash-asterisk is on its
* own line, so is the last asterisk-slash. */
builtin/commit.c static int prepare_to_commit(const char *index_file, const char *prefix, · line 740
Quoted 8 lines
builtin/commit.c +5
-0+if (!stat (git_path_squash_msg (), &statbuf)) { +if (strbuf_read_file (&sb,git_path_squash_msg (),0 ) <0 ) +die_errno (_ ("could not read SQUASH_MSG" )); + hook_arg1 ="squash" ; + } }else if (!stat (git_path_squash_msg (), &statbuf)) {if (strbuf_read_file (&sb,git_path_squash_msg (),0 ) <0 )die_errno (_ ("could not read SQUASH_MSG" ));
This reads MERGE_MSG first and then SQUASH_MSG; is that what we really want? When you are resolving a conflicted rebase, you would see the original log message and then conflicts section. What is in the SQUASH_MSG is the moral equivalent of the "original log message" but in a less summarized form, so I suspect that the list of conflicts should come to end.
The duplicated code to read the same file bothers me somewhat.
I wondered if it makes the result easier to follow (and easier to update) if this part of the code is restructured like this:
if (file_exists(git_path_merge_msg()) ||
file_exists(git_path_squash_msg())) {
if (file_exists(git_path_squash_msg())) {
read SQUASH_MSG;
}
if (file_exists(git_path_merge_msg()))
read MERGE_MSG;
}
hook_arg1 = "merge";
}but I am not sure if that structure is better.
Thanks.
Re: [PATCH] Also read SQUASH_MSG if a conflict on a merge squash occurred
Am 08.03.2016 um 19:32 schrieb Junio C Hamano:
Quoted 7 lines
Quoted 7 lines
builtin/commit.c +5
-0+if (!stat (git_path_squash_msg (), &statbuf)) { +if (strbuf_read_file (&sb,git_path_squash_msg (),0 ) <0 ) +die_errno (_ ("could not read SQUASH_MSG" )); + hook_arg1 ="squash" ; + } }else if (!stat (git_path_squash_msg (), &statbuf)) {if (strbuf_read_file (&sb,git_path_squash_msg (),0 ) <0 )
Quoted 8 lines
Quoted 1 line
die_errno(_("could not read SQUASH_MSG"));This reads
MERGE_MSGfirst and thenSQUASH_MSG; is that what we really want? When you are resolving a conflicted rebase, you would see the original log message and then conflicts section. What is in theSQUASH_MSGis the moral equivalent of the "original log message" but in a less summarized form, so I suspect that the list of conflicts should come to end.
I put them first because the squash commit list could be really long. I'll put MERGE_MSG at the end...
Quoted 5 lines
The duplicated code to read the same file bothers me somewhat.
I wondered if it makes the result easier to follow (and easier to update) if this part of the code is restructured like this:
Quoted 10 lines
if (file_exists(git_path_merge_msg()) ||
file_exists(git_path_squash_msg())) {
if (file_exists(git_path_squash_msg())) {
read SQUASH_MSG;
}
if (file_exists(git_path_merge_msg()))
read MERGE_MSG;
}
hook_arg1 = "merge";
}Here hook_arg1 would be always "merge" and never "squash"... Before my change it was only "squash" if no conflict occurred.
Signature · 3 lines
Best regards, Sven Strickroth PGP key id F5A9D4C4 @ any key-server
Re: [PATCH] Also read SQUASH_MSG if a conflict on a merge squash occurred
Quoted 2 lines · Sven Strickroth
Here
hook_arg1would be always "merge" and never "squash"... Before my change it was only "squash" if no conflict occurred.
Oh, that wasn't an intended change. It was merely an illustration of a possible restructuring of the flow to avoid having to have identical read functions in two separate places.
PATCH 1/1 commit: do not lose SQUASH_MSG contents
In reply to Junio C HamanoWhen concluding a conflicted "git merge --squash", the command failed to read SQUASH_MSG that was prepared by "git merge", and showed only the "# Conflicts:" list of conflicted paths.
--- builtin/commit.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)
builtin/commit.c +12 -8
@@ -725,14 +725,18 @@ static int prepare_to_commit(const char *index_file, const char *prefix,format_commit_message (commit,"fixup! %s\n\n" , &sb, &ctx); hook_arg1 ="message" ; - }else if (!stat (git_path_merge_msg (), &statbuf)) { -if (strbuf_read_file (&sb,git_path_merge_msg (),0 ) <0 ) -die_errno (_ ("could not read MERGE_MSG" )); - hook_arg1 ="merge" ; - }else if (!stat (git_path_squash_msg (), &statbuf)) { -if (strbuf_read_file (&sb,git_path_squash_msg (),0 ) <0 ) -die_errno (_ ("could not read SQUASH_MSG" )); - hook_arg1 ="squash" ; + }else if (!stat (git_path_squash_msg (), &statbuf) || + !stat (git_path_merge_msg (), &statbuf)) { +if (!stat (git_path_squash_msg (), &statbuf)) { +if (strbuf_read_file (&sb,git_path_squash_msg (),0 ) <0 ) +die_errno (_ ("could not read SQUASH_MSG" )); + hook_arg1 ="squash" ; + }else + hook_arg1 ="merge" ; +if (!stat (git_path_merge_msg (), &statbuf)) { +if (strbuf_read_file (&sb,git_path_merge_msg (),0 ) <0 ) +die_errno (_ ("could not read MERGE_MSG" )); + } }else if (template_file) {if (strbuf_read_file (&sb, template_file,0 ) <0 )die_errno (_ ("could not read '%s'" ), template_file);
Signature · 1 line
2.7.0.windows.1
Re: [PATCH] Also read SQUASH_MSG if a conflict on a merge squash occurred
In reply to Junio C HamanoQuoted 16 lines · Junio C Hamano
The duplicated code to read the same file bothers me somewhat.
I wondered if it makes the result easier to follow (and easier to update) if this part of the code is restructured like this:
if (file_exists(git_path_merge_msg()) || file_exists(git_path_squash_msg())) { if (file_exists(git_path_squash_msg())) { read SQUASH_MSG; } if (file_exists(git_path_merge_msg())) read MERGE_MSG; } hook_arg1 = "merge"; }
Quoted 1 line
but I am not sure if that structure is better.
... as this duplicates file_exists() call to the same thing, which is no better than duplicated calls to read *_MSG files.
Re: [PATCH] Also read SQUASH_MSG if a conflict on a merge squash occurred
Quoted 18 lines · Junio C Hamano
Quoted 16 lines · Junio C Hamano
The duplicated code to read the same file bothers me somewhat.
I wondered if it makes the result easier to follow (and easier to update) if this part of the code is restructured like this:
if (file_exists(git_path_merge_msg()) || file_exists(git_path_squash_msg())) { if (file_exists(git_path_squash_msg())) { read SQUASH_MSG; } if (file_exists(git_path_merge_msg())) read MERGE_MSG; } hook_arg1 = "merge"; }
Quoted 4 lines
Quoted 1 line
but I am not sure if that structure is better.
... as this duplicates
file_exists()call to the same thing, which is no better than duplicated calls to read*_MSGfiles.
So, let's take the program structure from your original, but fix the order of the inclusion (and the log message), perhaps like the attached patch.
Don't we also want to have a new test so that this "contents from both files are included in the result in the expected order" feature will not get broken in the future?
-- >8 --
Subject: [PATCH] commit: do not lose SQUASH_MSG contents
When concluding a conflicted "git merge --squash", the command failed to read SQUASH_MSG that was prepared by "git merge", and showed only the "# Conflicts:" list of conflicted paths.
Place the contents from SQUASH_MSG at the beginning, just like we show the commit log skeleton first when concluding a normal merge, and then show the "# Conflicts:" list, to help the user write the log message for the resulting commit.
---
builtin/commit.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)
builtin/commit.c +11 -1
@@ -726,9 +726,19 @@ static int prepare_to_commit(const char *index_file, const char *prefix, &sb, &ctx); hook_arg1 ="message" ; }else if (!stat (git_path_merge_msg (), &statbuf)) { + hook_arg1 ="merge" ; + +/* +* In a conflicted 'merge squash', the material to help +* writing the log message is found in SQUASH_MSG. +*/ +if (!stat (git_path_squash_msg (), &statbuf)) { +if (strbuf_read_file (&sb,git_path_squash_msg (),0 ) <0 ) +die_errno (_ ("could not read SQUASH_MSG" )); + hook_arg1 ="squash" ; + }if (strbuf_read_file (&sb,git_path_merge_msg (),0 ) <0 )die_errno (_ ("could not read MERGE_MSG" )); - hook_arg1 ="merge" ; }else if (!stat (git_path_squash_msg (), &statbuf)) {if (strbuf_read_file (&sb,git_path_squash_msg (),0 ) <0 )die_errno (_ ("could not read SQUASH_MSG" ));
Signature · 1 line
2.8.0-rc1-141-gbaa22e3
PATCH 1/1 commit: do not lose SQUASH_MSG contents
When concluding a conflicted "git merge --squash", the command failed to read SQUASH_MSG that was prepared by "git merge", and showed only the "# Conflicts:" list of conflicted paths.
Place the contents from SQUASH_MSG at the beginning, just like we show the commit log skeleton first when concluding a normal merge, and then show the "# Conflicts:" list, to help the user write the log message for the resulting commit.
--- builtin/commit.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
builtin/commit.c +10 -1
@@ -726,9 +726,18 @@ static int prepare_to_commit(const char *index_file, const char *prefix, &sb, &ctx); hook_arg1 ="message" ; }else if (!stat (git_path_merge_msg (), &statbuf)) { +/* +* prepend SQUASH_MSG here if it exists and a +* "merge --squash" was originally performed +*/ +if (!stat (git_path_squash_msg (), &statbuf)) { +if (strbuf_read_file (&sb,git_path_squash_msg (),0 ) <0 ) +die_errno (_ ("could not read SQUASH_MSG" )); + hook_arg1 ="squash" ; + }else + hook_arg1 ="merge" ;if (strbuf_read_file (&sb,git_path_merge_msg (),0 ) <0 )die_errno (_ ("could not read MERGE_MSG" )); - hook_arg1 ="merge" ; }else if (!stat (git_path_squash_msg (), &statbuf)) {if (strbuf_read_file (&sb,git_path_squash_msg (),0 ) <0 )die_errno (_ ("could not read SQUASH_MSG" ));
Signature · 3 lines
Best regards, Sven Strickroth PGP key id F5A9D4C4 @ any key-server
Re: [PATCH] commit: do not lose SQUASH_MSG contents
Quoted 9 lines · Sven Strickroth
When concluding a conflicted "git merge --squash", the command failed to read
SQUASH_MSGthat was prepared by "git merge", and showed only the "# Conflicts:" list of conflicted paths.Place the contents from
SQUASH_MSGat the beginning, just like we show the commit log skeleton first when concluding a normal merge, and then show the "# Conflicts:" list, to help the user write the log message for the resulting commit.
Quoted 4 lines
Signed-off-by Sven Strickroth <[email protected]>--- builtin/commit.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
The updated code looks good to me; sorry for misleading you with fuzzy comments earlier.
We may want to have a test to prevent this from getting broken in the future updates.
Quoted 24 lines
builtin/commit.c +10
-1@@ -726,9 +726,18 @@ static int prepare_to_commit(const char *index_file, const char *prefix, &sb, &ctx); hook_arg1 ="message" ; }else if (!stat (git_path_merge_msg (), &statbuf)) { +/* +* prepend SQUASH_MSG here if it exists and a +* "merge --squash" was originally performed +*/ +if (!stat (git_path_squash_msg (), &statbuf)) { +if (strbuf_read_file (&sb,git_path_squash_msg (),0 ) <0 ) +die_errno (_ ("could not read SQUASH_MSG" )); + hook_arg1 ="squash" ; + }else + hook_arg1 ="merge" ;if (strbuf_read_file (&sb,git_path_merge_msg (),0 ) <0 )die_errno (_ ("could not read MERGE_MSG" )); - hook_arg1 ="merge" ; }else if (!stat (git_path_squash_msg (), &statbuf)) {if (strbuf_read_file (&sb,git_path_squash_msg (),0 ) <0 )die_errno (_ ("could not read SQUASH_MSG" ));
Re: [PATCH] commit: do not lose SQUASH_MSG contents
Quoted 10 lines · Junio C Hamano
Quoted 9 lines
Place the contents from
SQUASH_MSGat the beginning, just like we show the commit log skeleton first when concluding a normal merge, and then show the "# Conflicts:" list, to help the user write the log message for the resulting commit.Signed-off-by Sven Strickroth <[email protected]>--- builtin/commit.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
Quoted 5 lines
The updated code looks good to me; sorry for misleading you with fuzzy comments earlier.
We may want to have a test to prevent this from getting broken in the future updates.
Perhaps like so:
t/t7600-merge.sh | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+)
t/t7600-merge.sh +28 -0
@@ -33,9 +33,11 @@ printf '%s\n' 1 2 3 4 5 6 7 8 9 >file printf '%s\n' '1 X' 2 3 4 5 6 7 8 9 >file.1 printf '%s\n' 1 2 3 4 '5 X' 6 7 8 9 >file.5 printf '%s\n' 1 2 3 4 5 6 7 8 '9 X' >file.9 +printf '%s\n' 1 2 3 4 5 6 7 8 '9 Y' >file.9y printf '%s\n' '1 X' 2 3 4 5 6 7 8 9 >result.1 printf '%s\n' '1 X' 2 3 4 '5 X' 6 7 8 9 >result.1-5 printf '%s\n' '1 X' 2 3 4 '5 X' 6 7 8 '9 X' >result.1-5-9 +printf '%s\n' 1 2 3 4 5 6 7 8 '9 Z' >result.9z >empty create_merge_msgs () {
@@ -128,6 +130,12 @@ test_expect_success 'setup' ' git tag c2 && c2=$(git rev-parse HEAD) && git reset --hard "$c0" && + cp file.9y file && + git add file && + test_tick && + git commit -m "commit 7" && + git tag c7 && + git reset --hard "$c0" && cp file.9 file && git add file && test_tick &&
@@ -218,6 +226,26 @@ test_expect_success 'merge c1 with c2' ' verify_parents $c1 $c2 ' +test_expect_success 'merge --squash c3 with c7' ' + git reset --hard c3 && + test_must_fail git merge --squash c7 && + cat result.9z >file && + git commit --no-edit -a && + + { + cat <<-EOF + Squashed commit of the following: + + $(git show -s c7) + + # Conflicts: + # file + EOF + } >expect && + git cat-file commit HEAD | sed -e '1,/^$/d' >actual && + test_cmp expect actual +' + test_debug 'git log --graph --decorate --oneline --all' test_expect_success 'merge c1 with c2 and c3' '
Signature · 1 line
2.8.0-rc2-154-gd146b22
PATCH 1/1 commit: do not lose SQUASH_MSG contents
When concluding a conflicted "git merge --squash", the command failed to read SQUASH_MSG that was prepared by "git merge", and showed only the "# Conflicts:" list of conflicted paths.
Place the contents from SQUASH_MSG at the beginning, just like we show the commit log skeleton first when concluding a normal merge, and then show the "# Conflicts:" list, to help the user write the log message for the resulting commit.
Test by Junio C Hamano <[email protected]>.
--- builtin/commit.c | 11 ++++++++++- t/t7600-merge.sh | 28 ++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-)
builtin/commit.c +10 -1
@@ -726,9 +726,18 @@ static int prepare_to_commit(const char *index_file, const char *prefix, &sb, &ctx); hook_arg1 ="message" ; }else if (!stat (git_path_merge_msg (), &statbuf)) { +/* +* prepend SQUASH_MSG here if it exists and a +* "merge --squash" was originally performed +*/ +if (!stat (git_path_squash_msg (), &statbuf)) { +if (strbuf_read_file (&sb,git_path_squash_msg (),0 ) <0 ) +die_errno (_ ("could not read SQUASH_MSG" )); + hook_arg1 ="squash" ; + }else + hook_arg1 ="merge" ;if (strbuf_read_file (&sb,git_path_merge_msg (),0 ) <0 )die_errno (_ ("could not read MERGE_MSG" )); - hook_arg1 ="merge" ; }else if (!stat (git_path_squash_msg (), &statbuf)) {if (strbuf_read_file (&sb,git_path_squash_msg (),0 ) <0 )die_errno (_ ("could not read SQUASH_MSG" ));
t/t7600-merge.sh +28 -0
@@ -33,9 +33,11 @@ printf '%s\n' 1 2 3 4 5 6 7 8 9 >file printf '%s\n' '1 X' 2 3 4 5 6 7 8 9 >file.1 printf '%s\n' 1 2 3 4 '5 X' 6 7 8 9 >file.5 printf '%s\n' 1 2 3 4 5 6 7 8 '9 X' >file.9 +printf '%s\n' 1 2 3 4 5 6 7 8 '9 Y' >file.9y printf '%s\n' '1 X' 2 3 4 5 6 7 8 9 >result.1 printf '%s\n' '1 X' 2 3 4 '5 X' 6 7 8 9 >result.1-5 printf '%s\n' '1 X' 2 3 4 '5 X' 6 7 8 '9 X' >result.1-5-9 +printf '%s\n' 1 2 3 4 5 6 7 8 '9 Z' >result.9z >empty create_merge_msgs () {
@@ -128,6 +130,12 @@ test_expect_success 'setup' ' git tag c2 && c2=$(git rev-parse HEAD) && git reset --hard "$c0" && + cp file.9y file && + git add file && + test_tick && + git commit -m "commit 7" && + git tag c7 && + git reset --hard "$c0" && cp file.9 file && git add file && test_tick &&
@@ -218,6 +226,26 @@ test_expect_success 'merge c1 with c2' ' verify_parents $c1 $c2 ' +test_expect_success 'merge --squash c3 with c7' ' + git reset --hard c3 && + test_must_fail git merge --squash c7 && + cat result.9z >file && + git commit --no-edit -a && + + { + cat <<-EOF + Squashed commit of the following: + + $(git show -s c7) + + # Conflicts: + # file + EOF + } >expect && + git cat-file commit HEAD | sed -e '1,/^$/d' >actual && + test_cmp expect actual +' + test_debug 'git log --graph --decorate --oneline --all' test_expect_success 'merge c1 with c2 and c3' '
Signature · 1 line
2.7.4.windows.1
Re: [PATCH] commit: do not lose SQUASH_MSG contents
Quoted 9 lines · Sven Strickroth
When concluding a conflicted "git merge --squash", the command failed to read
SQUASH_MSGthat was prepared by "git merge", and showed only the "# Conflicts:" list of conflicted paths.Place the contents from
SQUASH_MSGat the beginning, just like we show the commit log skeleton first when concluding a normal merge, and then show the "# Conflicts:" list, to help the user write the log message for the resulting commit.
Quoted 4 lines
Test by Junio C Hamano <[email protected]>.
Signed-off-by Sven Strickroth <[email protected]>---
You must somehow read my mind, as I was about to send a friendly ping to you saying "unless you have a reroll, I'll squash the test in" ;-)
Will replace those two commits with this one (after fixing one nit).
Thanks.
Quoted 9 lines
builtin/commit.c | 11 ++++++++++- t/t7600-merge.sh | 28 ++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-)builtin/commit.c +0
-0
builtin/commit.c static int prepare_to_commit(const char *index_file, const char *prefix, · line 732
Quoted 7 lines
builtin/commit.c +4
-0@@ -726,9 +726,18 @@ static int prepare_to_commit(const char *index_file, const char *prefix, &sb, &ctx); hook_arg1 ="message" ; }else if (!stat (git_path_merge_msg (), &statbuf)) { +/* +* prepend SQUASH_MSG here if it exists and a +* "merge --squash" was originally performed +*/
Here is a nit ("*/" needs one more space indent to align).