{"thread":{"id":"49129","subject":"[PATCH 0/2] rebase -i: fix SIGSEGV when 'merge ' fails","startedAt":"2018-08-15T09:39:52Z","lastAt":"2018-08-27T12:49:34Z","messageCount":6,"participants":["Phillip Wood","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"355680","messageId":"20180815093935.10222-1-phillip.wood@talktalk.net","threadId":"49129","inReplyTo":null,"subject":"[PATCH 0/2] rebase -i: fix SIGSEGV when 'merge ' fails","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-08-15T09:39:33Z","receivedAt":"2018-08-15T09:39:52Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nAs they fix a bug these patches are based on maint. Unfortunately\nthe second patch has semantic conflicts with master\n\ns/git_path_merge_msg()/git_path_merge_msg(the_repository)/\n\nThere are additional textual conflicts with pu/next due to some\nmessages being marked for translation. The new message here should be\nmarked for translation to match them.\n\nPhillip Wood (2):\n t3430: add conflicting commit\n rebase -i: fix SIGSEGV when 'merge ' fails\n\n sequencer.c | 24 +++++++++++++++++++-----\n t/t3430-rebase-merges.sh | 30 +++++++++++++++++++++++-------\n 2 files changed, 42 insertions(+), 12 deletions(-)\n\n-- \n2.18.0\n\n"},{"id":"355681","messageId":"20180815093935.10222-3-phillip.wood@talktalk.net","threadId":"49129","inReplyTo":"20180815093935.10222-1-phillip.wood@talktalk.net","subject":"[PATCH 2/2] rebase -i: fix SIGSEGV when 'merge ' fails","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-08-15T09:39:35Z","receivedAt":"2018-08-15T09:39:53Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nIf a merge command in the todo list specifies just a branch to merge\nwith no -C/-c argument then item->commit is NULL. This means that if\nthere are merge conflicts error_with_patch() is passed a NULL commit\nwhich causes a segmentation fault when make_patch() tries to look it up.\n\nThis commit implements a minimal fix which fixes the crash and allows\nthe user to successfully commit a conflict resolution with 'git rebase\n--continue'. It does not write .git/rebase-merge/patch,\n.git/rebase-merge/stopped-sha or update REBASE_HEAD. To sensibly get the\nhashes of the merge parents would require refactoring do_merge() to\nextract the code that parses the merge parents into a separate function\nwhich error_with_patch() could then use to write the parents into the\nstopped-sha file. To create meaningful output make_patch() and 'git\nrebase --show-current-patch' would also need to be modified to diff the\nmerge parent and merge base in this case.\n\nSigned-off-by: Phillip Wood \n---\n sequencer.c | 24 +++++++++++++++++++-----\n t/t3430-rebase-merges.sh | 15 ++++++++++++++-\n 2 files changed, 33 insertions(+), 6 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 4034c0461b..df49199175 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2590,8 +2590,12 @@ static int error_with_patch(struct commit *commit,\n \tconst char *subject, int subject_len,\n \tstruct replay_opts *opts, int exit_code, int to_amend)\n {\n-\tif (make_patch(commit, opts))\n-\t\treturn -1;\n+\tif (commit) {\n+\t\tif (make_patch(commit, opts))\n+\t\t\treturn -1;\n+\t} else if (copy_file(rebase_path_message(), git_path_merge_msg(), 0666))\n+\t\treturn error(_(\"unable to copy '%s' to '%s'\"),\n+\t\t\t git_path_merge_msg(), rebase_path_message());\n \n \tif (to_amend) {\n \t\tif (intend_to_amend())\n@@ -2604,9 +2608,19 @@ static int error_with_patch(struct commit *commit,\n \t\t\t\"Once you are satisfied with your changes, run\\n\"\n \t\t\t\"\\n\"\n \t\t\t\" git rebase --continue\\n\", gpg_sign_opt_quoted(opts));\n-\t} else if (exit_code)\n-\t\tfprintf(stderr, \"Could not apply %s... %.*s\\n\",\n-\t\t\tshort_commit_name(commit), subject_len, subject);\n+\t} else if (exit_code) {\n+\t\tif (commit)\n+\t\t\tfprintf(stderr, \"Could not apply %s... %.*s\\n\",\n+\t\t\t\tshort_commit_name(commit),\n+\t\t\t\tsubject_len, subject);\n+\t\telse\n+\t\t\t/*\n+\t\t\t * We don't have the hash of the parent so\n+\t\t\t * just print the line from the todo file.\n+\t\t\t */\n+\t\t\tfprintf(stderr, \"Could not merge %.*s\\n\",\n+\t\t\t\tsubject_len, subject);\n+\t}\n \n \treturn exit_code;\n }\ndiff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh\nindex 31fe4268d5..2e767d4f1e 100755\n--- a/t/t3430-rebase-merges.sh\n+++ b/t/t3430-rebase-merges.sh\n@@ -129,7 +129,7 @@ test_expect_success '`reset` refuses to overwrite untracked files' '\n \tgit rebase --abort\n '\n \n-test_expect_success 'failed `merge` writes patch (may be rescheduled, too)' '\n+test_expect_success 'failed `merge -C` writes patch (may be rescheduled, too)' '\n \ttest_when_finished \"test_might_fail git rebase --abort\" &&\n \tgit checkout -b conflicting-merge A &&\n \n@@ -151,6 +151,19 @@ test_expect_success 'failed `merge` writes patch (may be rescheduled, too)' '\n \ttest_path_is_file .git/rebase-merge/patch\n '\n \n+SQ=\"'\"\n+test_expect_success 'failed `merge ` does not crash' '\n+\ttest_when_finished \"test_might_fail git rebase --abort\" &&\n+\tgit checkout conflicting-G &&\n+\n+\techo \"merge G\" >script-from-scratch &&\n+\ttest_config sequence.editor \\\"\"$PWD\"/replace-editor.sh\\\" &&\n+\ttest_tick &&\n+\ttest_must_fail git rebase -ir HEAD &&\n+\t! grep \"^merge G$\" .git/rebase-merge/git-rebase-todo &&\n+\tgrep \"^Merge branch ${SQ}G${SQ}$\" .git/rebase-merge/message\n+'\n+\n test_expect_success 'with a branch tip that was cherry-picked already' '\n \tgit checkout -b already-upstream master &&\n \tbase=\"$(git rev-parse --verify HEAD)\" &&\n-- \n2.18.0\n\n"},{"id":"355682","messageId":"20180815093935.10222-2-phillip.wood@talktalk.net","threadId":"49129","inReplyTo":"20180815093935.10222-1-phillip.wood@talktalk.net","subject":"[PATCH 1/2] t3430: add conflicting commit","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-08-15T09:39:34Z","receivedAt":"2018-08-15T09:39:54Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nMove the creation of conflicting-G from a test to the setup so that it\ncan be used in subsequent tests without creating the kind of implicit\ndependencies that plague t3404. While we're at it simplify the\narguments to the test_commit() call the creates the conflicting commit.\n\nSigned-off-by: Phillip Wood \n---\n t/t3430-rebase-merges.sh | 15 +++++++++------\n 1 file changed, 9 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh\nindex 78f7c99580..31fe4268d5 100755\n--- a/t/t3430-rebase-merges.sh\n+++ b/t/t3430-rebase-merges.sh\n@@ -13,8 +13,10 @@ Initial setup:\n -- B -- (first)\n / \\\n A - C - D - E - H (master)\n- \\ /\n- F - G (second)\n+ \\ \\ /\n+ \\ F - G (second)\n+ \\\n+ Conflicting-G\n '\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-rebase.sh\n@@ -49,7 +51,9 @@ test_expect_success 'setup' '\n \tgit merge --no-commit G &&\n \ttest_tick &&\n \tgit commit -m H &&\n-\tgit tag -m H H\n+\tgit tag -m H H &&\n+\tgit checkout A &&\n+\ttest_commit conflicting-G G.t\n '\n \n test_expect_success 'create completely different structure' '\n@@ -72,7 +76,7 @@ test_expect_success 'create completely different structure' '\n \tEOF\n \ttest_config sequence.editor \\\"\"$PWD\"/replace-editor.sh\\\" &&\n \ttest_tick &&\n-\tgit rebase -i -r A &&\n+\tgit rebase -i -r A master &&\n \ttest_cmp_graph <<-\\EOF\n \t* Merge the topic branch '\\''onebranch'\\''\n \t|\\\n@@ -141,8 +145,7 @@ test_expect_success 'failed `merge` writes patch (may be rescheduled, too)' '\n \n \t: fail because of merge conflict &&\n \trm G.t .git/rebase-merge/patch &&\n-\tgit reset --hard &&\n-\ttest_commit conflicting-G G.t not-G conflicting-G &&\n+\tgit reset --hard conflicting-G &&\n \ttest_must_fail git rebase --continue &&\n \t! grep \"^merge -C .* G$\" .git/rebase-merge/git-rebase-todo &&\n \ttest_path_is_file .git/rebase-merge/patch\n-- \n2.18.0\n\n"},{"id":"355822","messageId":"nycvar.QRO.7.76.6.1808161036200.71@tvgsbejvaqbjf.bet","threadId":"49129","inReplyTo":"20180815093935.10222-3-phillip.wood@talktalk.net","subject":"Re: [PATCH 2/2] rebase -i: fix SIGSEGV when 'merge ' fails","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-08-16T08:36:34Z","receivedAt":"2018-08-16T08:36:45Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Phillip,\n\nOn Wed, 15 Aug 2018, Phillip Wood wrote:\n\n> From: Phillip Wood \n> \n> If a merge command in the todo list specifies just a branch to merge\n> with no -C/-c argument then item->commit is NULL. This means that if\n> there are merge conflicts error_with_patch() is passed a NULL commit\n> which causes a segmentation fault when make_patch() tries to look it up.\n> \n> This commit implements a minimal fix which fixes the crash and allows\n> the user to successfully commit a conflict resolution with 'git rebase\n> --continue'. It does not write .git/rebase-merge/patch,\n> .git/rebase-merge/stopped-sha or update REBASE_HEAD. To sensibly get the\n> hashes of the merge parents would require refactoring do_merge() to\n> extract the code that parses the merge parents into a separate function\n> which error_with_patch() could then use to write the parents into the\n> stopped-sha file. To create meaningful output make_patch() and 'git\n> rebase --show-current-patch' would also need to be modified to diff the\n> merge parent and merge base in this case.\n> \n> Signed-off-by: Phillip Wood \n\nACK!\n\nThanks,\nDscho\n"},{"id":"355846","messageId":"xmqqzhxmfgbb.fsf@gitster-ct.c.googlers.com","threadId":"49129","inReplyTo":"20180815093935.10222-3-phillip.wood@talktalk.net","subject":"Re: [PATCH 2/2] rebase -i: fix SIGSEGV when 'merge ' fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-16T16:04:56Z","receivedAt":"2018-08-16T16:05:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> This commit implements a minimal fix which fixes the crash and allows\n> the user to successfully commit a conflict resolution with 'git rebase\n> --continue'. It does not write .git/rebase-merge/patch,\n> .git/rebase-merge/stopped-sha or update REBASE_HEAD.\n\nI think that should be OK. When merging, a patch that shows the\ndiff from the merge base to the tip indeed is an interesting and\nuseful reference material to help the conflict resolution, but it is\nnot even clear what the latter two should mean while merging.\n\n> diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh\n> index 31fe4268d5..2e767d4f1e 100755\n> --- a/t/t3430-rebase-merges.sh\n> +++ b/t/t3430-rebase-merges.sh\n> @@ -129,7 +129,7 @@ test_expect_success '`reset` refuses to overwrite untracked files' '\n> \tgit rebase --abort\n> '\n> \n> -test_expect_success 'failed `merge` writes patch (may be rescheduled, too)' '\n> +test_expect_success 'failed `merge -C` writes patch (may be rescheduled, too)' '\n> \ttest_when_finished \"test_might_fail git rebase --abort\" &&\n> \tgit checkout -b conflicting-merge A &&\n> \n> @@ -151,6 +151,19 @@ test_expect_success 'failed `merge` writes patch (may be rescheduled, too)' '\n> \ttest_path_is_file .git/rebase-merge/patch\n> '\n> \n> +SQ=\"'\"\n\nA low-hanging fruit tangent, but somebody may want to go through the\noutput from\n\n $ git grep '[Ss][Qq]_*=' t\n\nand come up with a shared \"convenience\" definition of this, which\nperhaps sits next to the definition of $_x40 etc.\n\n> +test_expect_success 'failed `merge ` does not crash' '\n> +\ttest_when_finished \"test_might_fail git rebase --abort\" &&\n> +\tgit checkout conflicting-G &&\n> +\n> +\techo \"merge G\" >script-from-scratch &&\n> +\ttest_config sequence.editor \\\"\"$PWD\"/replace-editor.sh\\\" &&\n> +\ttest_tick &&\n> +\ttest_must_fail git rebase -ir HEAD &&\n> +\t! grep \"^merge G$\" .git/rebase-merge/git-rebase-todo &&\n> +\tgrep \"^Merge branch ${SQ}G${SQ}$\" .git/rebase-merge/message\n> +'\n> +\n> test_expect_success 'with a branch tip that was cherry-picked already' '\n> \tgit checkout -b already-upstream master &&\n> \tbase=\"$(git rev-parse --verify HEAD)\" &&\n\n"},{"id":"356577","messageId":"nycvar.QRO.7.76.6.1808271442010.73@tvgsbejvaqbjf.bet","threadId":"49129","inReplyTo":"xmqqzhxmfgbb.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/2] rebase -i: fix SIGSEGV when 'merge ' fails","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-08-27T12:49:25Z","receivedAt":"2018-08-27T12:49:34Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 16 Aug 2018, Junio C Hamano wrote:\n\n> Phillip Wood writes:\n> \n> > This commit implements a minimal fix which fixes the crash and allows\n> > the user to successfully commit a conflict resolution with 'git rebase\n> > --continue'. It does not write .git/rebase-merge/patch,\n> > .git/rebase-merge/stopped-sha or update REBASE_HEAD.\n> \n> I think that should be OK. When merging, a patch that shows the\n> diff from the merge base to the tip indeed is an interesting and\n> useful reference material to help the conflict resolution, but it is\n> not even clear what the latter two should mean while merging.\n\nLate reply, I know.\n\nIt is indeed ambiguous what the REBASE_HEAD should be... While a natural\nchoice would be the commit to be merged, that would be inconsistent with\nthe `-c`/`-C` version of `merge`.\n\nBut yes, the `patch` should not be written, and the `stopped-sha` does not\nmake sense with a `merge` that has neither `-c ` nor `-C `\n(although, please note, that this breaks a subsequent `fixup` if there is\none directly after the `merge`).\n\n> > diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh\n> > index 31fe4268d5..2e767d4f1e 100755\n> > --- a/t/t3430-rebase-merges.sh\n> > +++ b/t/t3430-rebase-merges.sh\n> > @@ -129,7 +129,7 @@ test_expect_success '`reset` refuses to overwrite untracked files' '\n> > \tgit rebase --abort\n> > '\n> > \n> > -test_expect_success 'failed `merge` writes patch (may be rescheduled, too)' '\n> > +test_expect_success 'failed `merge -C` writes patch (may be rescheduled, too)' '\n> > \ttest_when_finished \"test_might_fail git rebase --abort\" &&\n> > \tgit checkout -b conflicting-merge A &&\n> > \n> > @@ -151,6 +151,19 @@ test_expect_success 'failed `merge` writes patch (may be rescheduled, too)' '\n> > \ttest_path_is_file .git/rebase-merge/patch\n> > '\n> > \n> > +SQ=\"'\"\n> \n> A low-hanging fruit tangent, but somebody may want to go through the\n> output from\n> \n> $ git grep '[Ss][Qq]_*=' t\n> \n> and come up with a shared \"convenience\" definition of this, which\n> perhaps sits next to the definition of $_x40 etc.\n\nIf only we had a nice bug tracker with labels. I guess\nhttps://github.com/git/git would be a good place, but it *is* discouraged\nby us, which is a pity.\n\nCiao,\nDscho\n"}]}