{"thread":{"id":"38236","subject":"git: regression with mergetool and answering \"n\" (backport fix / add tests)","startedAt":"2014-12-23T19:08:34Z","lastAt":"2015-01-23T13:19:07Z","messageCount":4,"participants":["Daniel Hahler","David Aguilar"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"254041","messageId":"5499BDB2.4070301@thequod.de","threadId":"38236","inReplyTo":null,"subject":"git: regression with mergetool and answering \"n\" (backport fix / add tests)","fromName":"Daniel Hahler","fromEmail":"genml+git-2014@thequod.de","sentAt":"2014-12-23T19:08:34Z","receivedAt":"2014-12-23T19:08:34Z","isPatch":false,"sender":{"key":"genml+git-2014@thequod.de","avatar":null},"body":"Hi,\n\nthis is in reply to the commits from David:\n\n commit 0ddedd4d6b9b3e8eb3557d8ed28e1a0b354a25f8\n Refs: v2.2.0-60-g0ddedd4\n Merge: e886efd 1e86d5b\n Author: Junio C Hamano \n AuthorDate: Fri Dec 12 14:31:39 2014 -0800\n Commit: Junio C Hamano \n CommitDate: Fri Dec 12 14:31:39 2014 -0800\n\n Merge branch 'da/difftool-mergetool-simplify-reporting-status'\n\n Code simplification.\n\n * da/difftool-mergetool-simplify-reporting-status:\n mergetools: stop setting $status in merge_cmd()\n mergetool: simplify conditionals\n difftool--helper: add explicit exit statement\n mergetool--lib: remove use of $status global\n mergetool--lib: remove no-op assignment to $status from setup_user_tool\n\nI've ran into a problem, where \"git mergetool\" (using vimdiff) would add\nthe changes to the index, although you'd answered \"n\" after not changing/saving\nthe merged file.\n\nThis regression has been introduced in:\n\n commit 99474b6340dbcbe58f6c256fdee231cbadb060f4\n Author: David Aguilar \n Date: Fri Nov 14 13:33:55 2014 -0800\n\n difftool: honor --trust-exit-code for builtin tools\n \n run_merge_tool() was not setting $status, which prevented the\n exit code for builtin tools from being forwarded to the caller.\n \n Capture the exit status and add a test to guarantee the behavior.\n \n Reported-by: Adria Farres <14farresa@gmail.com>\n Signed-off-by: David Aguilar \n Signed-off-by: Junio C Hamano \n\n diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\n index c45a020..cce4f8c 100644\n --- a/git-mergetool--lib.sh\n +++ b/git-mergetool--lib.sh\n @@ -221,6 +221,7 @@ run_merge_tool () {\n else\n run_diff_cmd \"$1\"\n fi\n + status=$?\n return $status\n }\n\n\nMy fix has been the following, but I agree that the changes from David\nare much better in general.\n\n diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\n index cce4f8c..fa9acb1 100644\n --- a/git-mergetool--lib.sh\n +++ b/git-mergetool--lib.sh\n @@ -105,6 +105,7 @@ check_unchanged () {\n esac\n done\n fi\n + return $status\n }\n \n valid_tool () {\n\nI haven't verified if it really fixes the regression, but if it does it\nshould get backported into the branches where the regression is present.\n\n\nAlso, there should be some tests for this. I've came up with the\nfollowing, which is not finished and left in a state of debugging why it\ndid not trigger the bug.\n\nIn t/t7610-mergetool.sh:\n\n test_expect_success 'return code with untrusted mergetool' '\n # git config merge.tool untrusted &&\n # git config mergetool.untrusted.cmd \"true\" &&\n # git config mergetool.untrusted.trustExitCode false &&\n test_config mergetool.vimdiff.cmd \"cat \\\"\\$REMOTE\\\"\" &&\n git config merge.tool vimdiff &&\n git config --get-regexp mergetool.\n git config --get-regexp merge\\.tool.\n\n git checkout -b test_untrusted branch1 &&\n test_must_fail git merge master >/dev/null 2>&1 &&\n git status -s &&\n test \"$(git status --short both)\" = \"AA both\" &&\n ( echo \"n\" | test_must_fail git mergetool both ) &&\n git status -s &&\n\n test \"$(git status --short both)\" = \"AA both\" &&\n ( echo \"y\" | git mergetool both ) &&\n git status -s &&\n test \"$(git status --short both)\" = \"M both\" &&\n git reset --hard &&\n git config merge.tool mytool\n\n\nCheers,\nDaniel.\n\n-- \nhttp://daniel.hahler.de/\n\n\n"},{"id":"254099","messageId":"20141226010023.GC14150@gmail.com","threadId":"38236","inReplyTo":"5499BDB2.4070301@thequod.de","subject":"Re: git: regression with mergetool and answering \"n\" (backport fix / add tests)","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2014-12-26T01:00:25Z","receivedAt":"2014-12-26T01:00:25Z","isPatch":false,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Tue, Dec 23, 2014 at 08:08:34PM +0100, Daniel Hahler wrote:\n> Hi,\n> \n> this is in reply to the commits from David:\n> \n> commit 0ddedd4d6b9b3e8eb3557d8ed28e1a0b354a25f8\n> Refs: v2.2.0-60-g0ddedd4\n> Merge: e886efd 1e86d5b\n> Author: Junio C Hamano \n> AuthorDate: Fri Dec 12 14:31:39 2014 -0800\n> Commit: Junio C Hamano \n> CommitDate: Fri Dec 12 14:31:39 2014 -0800\n> \n> Merge branch 'da/difftool-mergetool-simplify-reporting-status'\n> \n> Code simplification.\n> \n> * da/difftool-mergetool-simplify-reporting-status:\n> mergetools: stop setting $status in merge_cmd()\n> mergetool: simplify conditionals\n> difftool--helper: add explicit exit statement\n> mergetool--lib: remove use of $status global\n> mergetool--lib: remove no-op assignment to $status from setup_user_tool\n> \n> I've ran into a problem, where \"git mergetool\" (using vimdiff) would add\n> the changes to the index, although you'd answered \"n\" after not changing/saving\n> the merged file.\n\nThanks for the heads-up.\n\nDo you perhaps have mergetool.vimdiff.trustExitCode defined, or\na similar setting?\n\nIf you saw the prompt then it should have aborted right after\nyou answered \"n\".\n\nThe very last thing merge_cmd() for vimdiff does is call\ncheck_unchanged(). We'll come back to check_unchanged() later.\n\nI tried to reproduce this issue. Here's a transcript:\n\n....\n$ git status -s\nUU file.txt\n\n$ git mergetool -t vimdiff file.txt\nMerging:\nfile.txt\n\nNormal merge conflict for 'file.txt':\n {local}: modified file\n {remote}: modified file\n4 files to edit\n#### Enter :qall inside vim\nfile.txt seems unchanged.\nWas the merge successful? [y/n] n\nmerge of file.txt failed\nContinue merging other unresolved paths (y/n) ? n\n\n$ git status -s\nUU file.txt\n....\n\nThat seemed to work fine. Any clues?\nMore notes below...\n\n> This regression has been introduced in:\n> \n> commit 99474b6340dbcbe58f6c256fdee231cbadb060f4\n> Author: David Aguilar \n> Date: Fri Nov 14 13:33:55 2014 -0800\n> \n> difftool: honor --trust-exit-code for builtin tools\n> \n> run_merge_tool() was not setting $status, which prevented the\n> exit code for builtin tools from being forwarded to the caller.\n> \n> Capture the exit status and add a test to guarantee the behavior.\n> \n> Reported-by: Adria Farres <14farresa@gmail.com>\n> Signed-off-by: David Aguilar \n> Signed-off-by: Junio C Hamano \n> \n> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\n> index c45a020..cce4f8c 100644\n> --- a/git-mergetool--lib.sh\n> +++ b/git-mergetool--lib.sh\n> @@ -221,6 +221,7 @@ run_merge_tool () {\n> else\n> run_diff_cmd \"$1\"\n> fi\n> + status=$?\n> return $status\n> }\n> \n> \n> My fix has been the following, but I agree that the changes from David\n> are much better in general.\n> \n> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\n> index cce4f8c..fa9acb1 100644\n> --- a/git-mergetool--lib.sh\n> +++ b/git-mergetool--lib.sh\n> @@ -105,6 +105,7 @@ check_unchanged () {\n> esac\n> done\n> fi\n> + return $status\n> }\n\nI don't think this fix does anything.\nHere is all of check_unchanged() for context:\n\ncheck_unchanged () {\n\tif test \"$MERGED\" -nt \"$BACKUP\"\n\tthen\n\t\treturn 0\n\telse\n\t\twhile true\n\t\tdo\n\t\t\techo \"$MERGED seems unchanged.\"\n\t\t\tprintf \"Was the merge successful? [y/n] \"\n\t\t\tread answer || return 1\n\t\t\tcase \"$answer\" in\n\t\t\ty*|Y*) return 0 ;;\n\t\t\tn*|N*) return 1 ;;\n\t\t\tesac\n\t\tdone\n\tfi\n}\n\nThe addition of \"return $status\" after the \"fi\" in the above fix\nwon't do anything because that code is unreachable.\nWe either return 0 or 1.\n\n> I haven't verified if it really fixes the regression, but if it does it\n> should get backported into the branches where the regression is present.\n\nAlso, the $status variable doesn't even exist anymore, so the\nfix is suspect.\n\nWhat platform are you on?\n\n> Also, there should be some tests for this.\n\nI don't disagree with that ;-)\n\nLet me know if you have any clues. I don't see anything obvious.\n\ncheers,\n-- \nDavid\n"},{"id":"254100","messageId":"549CB5ED.5040804@thequod.de","threadId":"38236","inReplyTo":"20141226010023.GC14150@gmail.com","subject":"Re: git: regression with mergetool and answering \"n\" (backport fix / add tests)","fromName":"Daniel Hahler","fromEmail":"genml+git-2014@thequod.de","sentAt":"2014-12-26T01:12:13Z","receivedAt":"2014-12-26T01:12:13Z","isPatch":false,"sender":{"key":"genml+git-2014@thequod.de","avatar":null},"body":"Hi David,\n\nsorry for the confusion - the patch / fix I've mentioned was meant to be\napplied on the commit that caused the regression and not current master.\n\n\nCheers,\nDaniel.\n\nOn 26.12.2014 02:00, David Aguilar wrote:\n> On Tue, Dec 23, 2014 at 08:08:34PM +0100, Daniel Hahler wrote:\n>> Hi,\n>>\n>> this is in reply to the commits from David:\n>>\n>> commit 0ddedd4d6b9b3e8eb3557d8ed28e1a0b354a25f8\n>> Refs: v2.2.0-60-g0ddedd4\n>> Merge: e886efd 1e86d5b\n>> Author: Junio C Hamano \n>> AuthorDate: Fri Dec 12 14:31:39 2014 -0800\n>> Commit: Junio C Hamano \n>> CommitDate: Fri Dec 12 14:31:39 2014 -0800\n>>\n>> Merge branch 'da/difftool-mergetool-simplify-reporting-status'\n>>\n>> Code simplification.\n>>\n>> * da/difftool-mergetool-simplify-reporting-status:\n>> mergetools: stop setting $status in merge_cmd()\n>> mergetool: simplify conditionals\n>> difftool--helper: add explicit exit statement\n>> mergetool--lib: remove use of $status global\n>> mergetool--lib: remove no-op assignment to $status from setup_user_tool\n>>\n>> I've ran into a problem, where \"git mergetool\" (using vimdiff) would add\n>> the changes to the index, although you'd answered \"n\" after not changing/saving\n>> the merged file.\n> \n> Thanks for the heads-up.\n> \n> Do you perhaps have mergetool.vimdiff.trustExitCode defined, or\n> a similar setting?\n> \n> If you saw the prompt then it should have aborted right after\n> you answered \"n\".\n> \n> The very last thing merge_cmd() for vimdiff does is call\n> check_unchanged(). We'll come back to check_unchanged() later.\n> \n> I tried to reproduce this issue. Here's a transcript:\n> \n> ....\n> $ git status -s\n> UU file.txt\n> \n> $ git mergetool -t vimdiff file.txt\n> Merging:\n> file.txt\n> \n> Normal merge conflict for 'file.txt':\n> {local}: modified file\n> {remote}: modified file\n> 4 files to edit\n> #### Enter :qall inside vim\n> file.txt seems unchanged.\n> Was the merge successful? [y/n] n\n> merge of file.txt failed\n> Continue merging other unresolved paths (y/n) ? n\n> \n> $ git status -s\n> UU file.txt\n> ....\n> \n> That seemed to work fine. Any clues?\n> More notes below...\n> \n>> This regression has been introduced in:\n>>\n>> commit 99474b6340dbcbe58f6c256fdee231cbadb060f4\n>> Author: David Aguilar \n>> Date: Fri Nov 14 13:33:55 2014 -0800\n>>\n>> difftool: honor --trust-exit-code for builtin tools\n>> \n>> run_merge_tool() was not setting $status, which prevented the\n>> exit code for builtin tools from being forwarded to the caller.\n>> \n>> Capture the exit status and add a test to guarantee the behavior.\n>> \n>> Reported-by: Adria Farres <14farresa@gmail.com>\n>> Signed-off-by: David Aguilar \n>> Signed-off-by: Junio C Hamano \n>>\n>> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\n>> index c45a020..cce4f8c 100644\n>> --- a/git-mergetool--lib.sh\n>> +++ b/git-mergetool--lib.sh\n>> @@ -221,6 +221,7 @@ run_merge_tool () {\n>> else\n>> run_diff_cmd \"$1\"\n>> fi\n>> + status=$?\n>> return $status\n>> }\n>>\n>>\n>> My fix has been the following, but I agree that the changes from David\n>> are much better in general.\n>>\n>> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\n>> index cce4f8c..fa9acb1 100644\n>> --- a/git-mergetool--lib.sh\n>> +++ b/git-mergetool--lib.sh\n>> @@ -105,6 +105,7 @@ check_unchanged () {\n>> esac\n>> done\n>> fi\n>> + return $status\n>> }\n> \n> I don't think this fix does anything.\n> Here is all of check_unchanged() for context:\n> \n> check_unchanged () {\n> \tif test \"$MERGED\" -nt \"$BACKUP\"\n> \tthen\n> \t\treturn 0\n> \telse\n> \t\twhile true\n> \t\tdo\n> \t\t\techo \"$MERGED seems unchanged.\"\n> \t\t\tprintf \"Was the merge successful? [y/n] \"\n> \t\t\tread answer || return 1\n> \t\t\tcase \"$answer\" in\n> \t\t\ty*|Y*) return 0 ;;\n> \t\t\tn*|N*) return 1 ;;\n> \t\t\tesac\n> \t\tdone\n> \tfi\n> }\n> \n> The addition of \"return $status\" after the \"fi\" in the above fix\n> won't do anything because that code is unreachable.\n> We either return 0 or 1.\n> \n>> I haven't verified if it really fixes the regression, but if it does it\n>> should get backported into the branches where the regression is present.\n> \n> Also, the $status variable doesn't even exist anymore, so the\n> fix is suspect.\n> \n> What platform are you on?\n> \n>> Also, there should be some tests for this.\n> \n> I don't disagree with that ;-)\n> \n> Let me know if you have any clues. I don't see anything obvious.\n> \n> cheers,\n> \n\n-- \nhttp://daniel.hahler.de/\n\n"},{"id":"255181","messageId":"54C24A4B.3000909@thequod.de","threadId":"38236","inReplyTo":"549CB5ED.5040804@thequod.de","subject":"Re: git: regression with mergetool and answering \"n\" (backport fix / add tests)","fromName":"Daniel Hahler","fromEmail":"genml+git-2014@thequod.de","sentAt":"2015-01-23T13:19:07Z","receivedAt":"2015-01-23T13:19:07Z","isPatch":false,"sender":{"key":"genml+git-2014@thequod.de","avatar":null},"body":"Hi,\n\nI am a bit surprised that this bug still exists in \"maint\" / v2.2.2.\n\nCherry-picking/merging 0ddedd4 fixes it.\n\n\nRegards,\nDaniel.\n\nOn 26.12.2014 02:12, Daniel Hahler wrote:\n> Hi David,\n> \n> sorry for the confusion - the patch / fix I've mentioned was meant to be\n> applied on the commit that caused the regression and not current master.\n> \n> \n> Cheers,\n> Daniel.\n> \n> On 26.12.2014 02:00, David Aguilar wrote:\n>> On Tue, Dec 23, 2014 at 08:08:34PM +0100, Daniel Hahler wrote:\n>>> Hi,\n>>>\n>>> this is in reply to the commits from David:\n>>>\n>>> commit 0ddedd4d6b9b3e8eb3557d8ed28e1a0b354a25f8\n>>> Refs: v2.2.0-60-g0ddedd4\n>>> Merge: e886efd 1e86d5b\n>>> Author: Junio C Hamano \n>>> AuthorDate: Fri Dec 12 14:31:39 2014 -0800\n>>> Commit: Junio C Hamano \n>>> CommitDate: Fri Dec 12 14:31:39 2014 -0800\n>>>\n>>> Merge branch 'da/difftool-mergetool-simplify-reporting-status'\n>>>\n>>> Code simplification.\n>>>\n>>> * da/difftool-mergetool-simplify-reporting-status:\n>>> mergetools: stop setting $status in merge_cmd()\n>>> mergetool: simplify conditionals\n>>> difftool--helper: add explicit exit statement\n>>> mergetool--lib: remove use of $status global\n>>> mergetool--lib: remove no-op assignment to $status from setup_user_tool\n>>>\n>>> I've ran into a problem, where \"git mergetool\" (using vimdiff) would add\n>>> the changes to the index, although you'd answered \"n\" after not changing/saving\n>>> the merged file.\n>>\n>> Thanks for the heads-up.\n>>\n>> Do you perhaps have mergetool.vimdiff.trustExitCode defined, or\n>> a similar setting?\n>>\n>> If you saw the prompt then it should have aborted right after\n>> you answered \"n\".\n>>\n>> The very last thing merge_cmd() for vimdiff does is call\n>> check_unchanged(). We'll come back to check_unchanged() later.\n>>\n>> I tried to reproduce this issue. Here's a transcript:\n>>\n>> ....\n>> $ git status -s\n>> UU file.txt\n>>\n>> $ git mergetool -t vimdiff file.txt\n>> Merging:\n>> file.txt\n>>\n>> Normal merge conflict for 'file.txt':\n>> {local}: modified file\n>> {remote}: modified file\n>> 4 files to edit\n>> #### Enter :qall inside vim\n>> file.txt seems unchanged.\n>> Was the merge successful? [y/n] n\n>> merge of file.txt failed\n>> Continue merging other unresolved paths (y/n) ? n\n>>\n>> $ git status -s\n>> UU file.txt\n>> ....\n>>\n>> That seemed to work fine. Any clues?\n>> More notes below...\n>>\n>>> This regression has been introduced in:\n>>>\n>>> commit 99474b6340dbcbe58f6c256fdee231cbadb060f4\n>>> Author: David Aguilar \n>>> Date: Fri Nov 14 13:33:55 2014 -0800\n>>>\n>>> difftool: honor --trust-exit-code for builtin tools\n>>> \n>>> run_merge_tool() was not setting $status, which prevented the\n>>> exit code for builtin tools from being forwarded to the caller.\n>>> \n>>> Capture the exit status and add a test to guarantee the behavior.\n>>> \n>>> Reported-by: Adria Farres <14farresa@gmail.com>\n>>> Signed-off-by: David Aguilar \n>>> Signed-off-by: Junio C Hamano \n>>>\n>>> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\n>>> index c45a020..cce4f8c 100644\n>>> --- a/git-mergetool--lib.sh\n>>> +++ b/git-mergetool--lib.sh\n>>> @@ -221,6 +221,7 @@ run_merge_tool () {\n>>> else\n>>> run_diff_cmd \"$1\"\n>>> fi\n>>> + status=$?\n>>> return $status\n>>> }\n>>>\n>>>\n>>> My fix has been the following, but I agree that the changes from David\n>>> are much better in general.\n>>>\n>>> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\n>>> index cce4f8c..fa9acb1 100644\n>>> --- a/git-mergetool--lib.sh\n>>> +++ b/git-mergetool--lib.sh\n>>> @@ -105,6 +105,7 @@ check_unchanged () {\n>>> esac\n>>> done\n>>> fi\n>>> + return $status\n>>> }\n>>\n>> I don't think this fix does anything.\n>> Here is all of check_unchanged() for context:\n>>\n>> check_unchanged () {\n>> \tif test \"$MERGED\" -nt \"$BACKUP\"\n>> \tthen\n>> \t\treturn 0\n>> \telse\n>> \t\twhile true\n>> \t\tdo\n>> \t\t\techo \"$MERGED seems unchanged.\"\n>> \t\t\tprintf \"Was the merge successful? [y/n] \"\n>> \t\t\tread answer || return 1\n>> \t\t\tcase \"$answer\" in\n>> \t\t\ty*|Y*) return 0 ;;\n>> \t\t\tn*|N*) return 1 ;;\n>> \t\t\tesac\n>> \t\tdone\n>> \tfi\n>> }\n>>\n>> The addition of \"return $status\" after the \"fi\" in the above fix\n>> won't do anything because that code is unreachable.\n>> We either return 0 or 1.\n>>\n>>> I haven't verified if it really fixes the regression, but if it does it\n>>> should get backported into the branches where the regression is present.\n>>\n>> Also, the $status variable doesn't even exist anymore, so the\n>> fix is suspect.\n>>\n>> What platform are you on?\n>>\n>>> Also, there should be some tests for this.\n>>\n>> I don't disagree with that ;-)\n>>\n>> Let me know if you have any clues. I don't see anything obvious.\n>>\n>> cheers,\n>>\n> \n\n-- \nhttp://daniel.hahler.de/\n\n"}]}