{"thread":{"id":"38185","subject":"bug & patch: exit codes from internal commands are handled incorrectly","startedAt":"2014-12-16T19:28:15Z","lastAt":"2015-04-01T00:40:06Z","messageCount":8,"participants":["Kenneth Lorber","Torsten Bögershausen","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"253740","messageId":"13474FB1-5310-42E5-82A9-4047FEFEFF4A@his.com","threadId":"38185","inReplyTo":null,"subject":"bug & patch: exit codes from internal commands are handled incorrectly","fromName":"Kenneth Lorber","fromEmail":"keni@his.com","sentAt":"2014-12-16T19:28:15Z","receivedAt":"2014-12-16T19:28:15Z","isPatch":false,"sender":{"key":"keni@his.com","avatar":"https://avatars.githubusercontent.com/u/16437442?v=4"},"body":"(Apologies if I’ve missed anything here - I’m still climbing the git learning curve.)\n\nBug: exit codes from (at least) internal commands are handled incorrectly.\nE.g. git-merge-file, docs say:\n        The exit value of this program is negative on error, and the number of\n        conflicts otherwise. If the merge was clean, the exit value is 0.\nBut only 8 bits get carried through exit, so 256 conflicts gives exit(0), which means the merge was clean.\n\nIssue shows up on:\n git version 1.8.5.2 (Apple Git-48)\n build from source (git version 2.2.0.68.g64396d6.dirty after patch below applied)\n\nReproduce by:\n\nPut the following test program in an empty directory as buggen.pl\n\nTEST PROGRAM START\nopen OUTB, \">basefile\" or die;\nprint OUTB \"this is the base file\\n\";\nclose OUTB;\n\nopen OUT1, \">bfile1\" or die;\nopen OUT2, \">bfile2\" or die;\n\n$c = shift;\n\nwhile($c > 0){\n        print OUT1 \"a\\nb\\nc\\nd\\nchange $c file 1\\n\";\n        print OUT2 \"a\\nb\\nc\\nd\\nchange $c file 2\\n\";\n        $c--;\n}\nTEST PROGRAM END\n\nDo these tests:\n\nperl buggen.pl 0\ngit merge-file -p bfile1 basefile bfile2\necho $status\n0\n\nperl buggen.pl 1\ngit merge-file -p bfile1 basefile bfile2\necho $status\n1\n\nperl buggen.pl 256\ngit merge-file -p bfile1 basefile bfile2\necho $status\n0       <===OOPS\n\nProposed patches:\ndiff --git a/git.c b/git.c\nindex 82d7a1c..8228a3c 100644\n--- a/git.c\n+++ b/git.c\n@@ -349,6 +349,8 @@ static int run_builtin(struct cmd_struct *p, int argc, const\n        trace_argv_printf(argv, \"trace: built-in: git\");\n\n        status = p->fn(argc, argv, prefix);\n+       if (status > 255)\n+               status = 255;   /* prevent exit() from truncating to 0 */\n        if (status)\n                return status;\ndiff --git a/Documentation/git-merge-file.txt b/Documentation/git-merge-file.txt\nindex d2fc12e..76e6a11 100644\n--- a/Documentation/git-merge-file.txt\n+++ b/Documentation/git-merge-file.txt\n@@ -41,7 +41,8 @@ lines from `<other-file>`, or lines from both respectively.  T\n conflict markers can be given with the `--marker-size` option.\n\n The exit value of this program is negative on error, and the number of\n-conflicts otherwise. If the merge was clean, the exit value is 0.\n+conflicts otherwise (but 255 will be reported even if more than 255 conflicts\n+exist). If the merge was clean, the exit value is 0.\n\n 'git merge-file' is designed to be a minimal clone of RCS 'merge'; that is, it\n implements all of RCS 'merge''s functionality which is needed by\n\nOpen questions:\n1) Is 255 safe for all operating systems?\n2) Does this issue affect any other places?\n\nThanks,\nKeni\nkeni@his.com\n"},{"id":"253797","messageId":"885593BD-3024-4811-83A5-D3A0C4CE6AC1@his.com","threadId":"38185","inReplyTo":"13474FB1-5310-42E5-82A9-4047FEFEFF4A@his.com","subject":"Re: bug & patch: exit codes from internal commands are handled incorrectly","fromName":"Kenneth Lorber","fromEmail":"keni@his.com","sentAt":"2014-12-18T02:15:47Z","receivedAt":"2014-12-18T02:15:47Z","isPatch":false,"sender":{"key":"keni@his.com","avatar":"https://avatars.githubusercontent.com/u/16437442?v=4"},"body":"The situation is actually slightly more complex than I stated previously.  From the docs:\n  The exit value of this program is negative on error,\nBut there’s no such thing as a negative error code under Unix, so (at best) that will be exit(255).\n\nNo patch, because this is getting painfully close to needing someone with a global view of the code to fix.\n\nThanks,\nKeni\n\nOn Dec 16, 2014, at 2:28 PM, Kenneth Lorber <keni@his.com> wrote:\n\n> (Apologies if I’ve missed anything here - I’m still climbing the git learning curve.)\n> \n> Bug: exit codes from (at least) internal commands are handled incorrectly.\n> E.g. git-merge-file, docs say:\n>        The exit value of this program is negative on error, and the number of\n>        conflicts otherwise. If the merge was clean, the exit value is 0.\n> But only 8 bits get carried through exit, so 256 conflicts gives exit(0), which means the merge was clean.\n> \n> Issue shows up on:\n> git version 1.8.5.2 (Apple Git-48)\n> build from source (git version 2.2.0.68.g64396d6.dirty after patch below applied)\n> \n> Reproduce by:\n> \n> Put the following test program in an empty directory as buggen.pl\n> \n> TEST PROGRAM START\n> open OUTB, \">basefile\" or die;\n> print OUTB \"this is the base file\\n\";\n> close OUTB;\n> \n> open OUT1, \">bfile1\" or die;\n> open OUT2, \">bfile2\" or die;\n> \n> $c = shift;\n> \n> while($c > 0){\n>        print OUT1 \"a\\nb\\nc\\nd\\nchange $c file 1\\n\";\n>        print OUT2 \"a\\nb\\nc\\nd\\nchange $c file 2\\n\";\n>        $c--;\n> }\n> TEST PROGRAM END\n> \n> Do these tests:\n> \n> perl buggen.pl 0\n> git merge-file -p bfile1 basefile bfile2\n> echo $status\n> 0\n> \n> perl buggen.pl 1\n> git merge-file -p bfile1 basefile bfile2\n> echo $status\n> 1\n> \n> perl buggen.pl 256\n> git merge-file -p bfile1 basefile bfile2\n> echo $status\n> 0       <===OOPS\n> \n> Proposed patches:\n> diff --git a/git.c b/git.c\n> index 82d7a1c..8228a3c 100644\n> --- a/git.c\n> +++ b/git.c\n> @@ -349,6 +349,8 @@ static int run_builtin(struct cmd_struct *p, int argc, const\n>        trace_argv_printf(argv, \"trace: built-in: git\");\n> \n>        status = p->fn(argc, argv, prefix);\n> +       if (status > 255)\n> +               status = 255;   /* prevent exit() from truncating to 0 */\n>        if (status)\n>                return status;\n> diff --git a/Documentation/git-merge-file.txt b/Documentation/git-merge-file.txt\n> index d2fc12e..76e6a11 100644\n> --- a/Documentation/git-merge-file.txt\n> +++ b/Documentation/git-merge-file.txt\n> @@ -41,7 +41,8 @@ lines from `<other-file>`, or lines from both respectively.  T\n> conflict markers can be given with the `--marker-size` option.\n> \n> The exit value of this program is negative on error, and the number of\n> -conflicts otherwise. If the merge was clean, the exit value is 0.\n> +conflicts otherwise (but 255 will be reported even if more than 255 conflicts\n> +exist). If the merge was clean, the exit value is 0.\n> \n> 'git merge-file' is designed to be a minimal clone of RCS 'merge'; that is, it\n> implements all of RCS 'merge''s functionality which is needed by\n> \n> Open questions:\n> 1) Is 255 safe for all operating systems?\n> 2) Does this issue affect any other places?\n> \n> Thanks,\n> Keni\n> keni@his.com\n> \n"},{"id":"253810","messageId":"54931233.6080900@web.de","threadId":"38185","inReplyTo":"885593BD-3024-4811-83A5-D3A0C4CE6AC1@his.com","subject":"Re: bug & patch: exit codes from internal commands are handled incorrectly","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2014-12-18T17:43:15Z","receivedAt":"2014-12-18T17:43:15Z","isPatch":false,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 18.12.14 03:15, Kenneth Lorber wrote:\n> The situation is actually slightly more complex than I stated previously.  From the docs:\n>   The exit value of this program is negative on error,\n> But there’s no such thing as a negative error code under Unix, so (at best) that will be exit(255).\n> \n> No patch, because this is getting painfully close to needing someone with a global view of the code to fix.\n> \n> Thanks,\n> Keni\n> \nMy spontanous question:\nWould it be save to clamp at 127 ?\n\n>> +       if (status > 127)\n>> +               status = 127;   /* prevent exit() from truncating to 0 or becoming negative */\n\n\n(And the rest looked good enough to become a patch, \nor at least to start a wider discussion)\n"},{"id":"253812","messageId":"xmqqegrw7omv.fsf@gitster.dls.corp.google.com","threadId":"38185","inReplyTo":"885593BD-3024-4811-83A5-D3A0C4CE6AC1@his.com","subject":"Re: bug & patch: exit codes from internal commands are handled incorrectly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-18T19:18:32Z","receivedAt":"2014-12-18T19:18:32Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kenneth Lorber <keni@his.com> writes:\n\n>> Bug: exit codes from (at least) internal commands are handled incorrectly.\n>> E.g. git-merge-file, docs say:\n>>        The exit value of this program is negative on error, and the number of\n>>        conflicts otherwise. If the merge was clean, the exit value is 0.\n>> But only 8 bits get carried through exit, so 256 conflicts gives\n>> exit(0), which means the merge was clean.\n\nWouldn't any cmd_foo() that returns negative to main() be buggy?\n\nYour change sweeps such problems under the rug, which is not a\nhealthy thing to do.\n\nExpecting that the exit code can signal small positive integers and\nother generic kinds of failures is a losing proposition.  I think it\nis a better fix to update cmd_merge_file() to return 1 (when ret is\npositive), 0 (when ret is zero) or 128 (when ret is negative), or\nsomething simple like that, and update the documentation to match\nthat, without touching git.c::main().\n\nAmong the in-tree users, I notice git-cvsserver.perl is already\nusing the command incorrectly.  It does this:\n\n            my $return = system(\"git\", \"merge-file\", $file_local, $file_old, $file_new);\n            $return >>= 8;\n\n            cleanupTmpDir();\n\n            if ( $return == 0 )\n            {\n                $log->info(\"Merged successfully\");\n                ...\n            }\n            elsif ( $return == 1 )\n            {\n                $log->info(\"Merged with conflicts\");\n                ...\n            }\n            else\n            {\n                $log->warn(\"Merge failed\");\n                next;\n            }\n\nwhich assumes $return == 1 is special \"half-good\", which is not\nconsistent with the documented interface.  It will incorrectly\nsay \"Merge failed\" when there are two or more conflicts.\n\nAnd with such a \"1 or 0 or -1\" change, you will fix that caller as\nwell ;-)\n"},{"id":"253878","messageId":"0C0299AC-0180-460D-A486-1A96E322CC5E@his.com","threadId":"38185","inReplyTo":"xmqqegrw7omv.fsf@gitster.dls.corp.google.com","subject":"Re: bug & patch: exit codes from internal commands are handled incorrectly","fromName":"Kenneth Lorber","fromEmail":"keni@his.com","sentAt":"2014-12-20T12:39:16Z","receivedAt":"2014-12-20T12:39:16Z","isPatch":false,"sender":{"key":"keni@his.com","avatar":"https://avatars.githubusercontent.com/u/16437442?v=4"},"body":"\nOn Dec 18, 2014, at 2:18 PM, Junio C Hamano <gitster@pobox.com> wrote:\n\n> Kenneth Lorber <keni@his.com> writes:\n> \n>>> Bug: exit codes from (at least) internal commands are handled incorrectly.\n>>> E.g. git-merge-file, docs say:\n>>>       The exit value of this program is negative on error, and the number of\n>>>       conflicts otherwise. If the merge was clean, the exit value is 0.\n>>> But only 8 bits get carried through exit, so 256 conflicts gives\n>>> exit(0), which means the merge was clean.\n> \n> Wouldn't any cmd_foo() that returns negative to main() be buggy?\n\nAbsolutely.\n\n> Your change sweeps such problems under the rug, which is not a\n> healthy thing to do.\n\nTrue.  But as I don’t know the codebase it seemed the safe thing to do\nat least to prove the bug existed before filing a bug report.\n\n> Expecting that the exit code can signal small positive integers and\n> other generic kinds of failures is a losing proposition.  I think it\n> is a better fix to update cmd_merge_file() to return 1 (when ret is\n> positive), 0 (when ret is zero) or 128 (when ret is negative), or\n> something simple like that, and update the documentation to match\n> that, without touching git.c::main().\n\nI agree.\n\nIt does leave open the question of whether this bit of information\nis of any use and should/could be reported out in another way.  It’s not\nto me, so I won’t be proposing anything to address that.\n\n> Among the in-tree users, I notice git-cvsserver.perl is already\n> using the command incorrectly.  It does this:\n> \n>            my $return = system(\"git\", \"merge-file\", $file_local, $file_old, $file_new);\n>            $return >>= 8;\n> \n>            cleanupTmpDir();\n> \n>            if ( $return == 0 )\n>            {\n>                $log->info(\"Merged successfully\");\n>                ...\n>            }\n>            elsif ( $return == 1 )\n>            {\n>                $log->info(\"Merged with conflicts\");\n>                ...\n>            }\n>            else\n>            {\n>                $log->warn(\"Merge failed\");\n>                next;\n>            }\n> \n> which assumes $return == 1 is special \"half-good\", which is not\n> consistent with the documented interface.  It will incorrectly\n> say \"Merge failed\" when there are two or more conflicts.\n> \n> And with such a \"1 or 0 or -1\" change, you will fix that caller as\n> well ;-)\n\nYay! :-)\n\nThanks,\nKeni\n"},{"id":"255496","messageId":"4B764581-0053-467C-93B5-C238EF5AE112@his.com","threadId":"38185","inReplyTo":"xmqqegrw7omv.fsf@gitster.dls.corp.google.com","subject":"Re: bug & patch: exit codes from internal commands are handled incorrectly","fromName":"Kenneth Lorber","fromEmail":"keni@his.com","sentAt":"2015-02-01T22:32:52Z","receivedAt":"2015-02-01T22:32:52Z","isPatch":false,"sender":{"key":"keni@his.com","avatar":"https://avatars.githubusercontent.com/u/16437442?v=4"},"body":"\nOn Dec 18, 2014, at 2:18 PM, Junio C Hamano <gitster@pobox.com> wrote:\n\n> Kenneth Lorber <keni@his.com> writes:\n> \n>>> Bug: exit codes from (at least) internal commands are handled incorrectly.\n>>> E.g. git-merge-file, docs say:\n>>>       The exit value of this program is negative on error, and the number of\n>>>       conflicts otherwise. If the merge was clean, the exit value is 0.\n>>> But only 8 bits get carried through exit, so 256 conflicts gives\n>>> exit(0), which means the merge was clean.\n> \n> Wouldn't any cmd_foo() that returns negative to main() be buggy?\n\nYes.\n\n> \n> Your change sweeps such problems under the rug, which is not a\n> healthy thing to do.\n\nAgreed.  (See below.)\n\n> \n> Expecting that the exit code can signal small positive integers and\n> other generic kinds of failures is a losing proposition.  I think it\n> is a better fix to update cmd_merge_file() to return 1 (when ret is\n> positive), 0 (when ret is zero) or 128 (when ret is negative), or\n> something simple like that, and update the documentation to match\n> that, without touching git.c::main().\n\nThe new patch uses 0, 1, and 2 to match diff(1).\n\n> Among the in-tree users, I notice git-cvsserver.perl is already\n> using the command incorrectly.  It does this:\n> \n>            my $return = system(\"git\", \"merge-file\", $file_local, $file_old, $file_new);\n>            $return >>= 8;\n> \n>            cleanupTmpDir();\n> \n>            if ( $return == 0 )\n>            {\n>                $log->info(\"Merged successfully\");\n>                ...\n>            }\n>            elsif ( $return == 1 )\n>            {\n>                $log->info(\"Merged with conflicts\");\n>                ...\n>            }\n>            else\n>            {\n>                $log->warn(\"Merge failed\");\n>                next;\n>            }\n> \n> which assumes $return == 1 is special \"half-good\", which is not\n> consistent with the documented interface.  It will incorrectly\n> say \"Merge failed\" when there are two or more conflicts.\n> \n> And with such a \"1 or 0 or -1\" change, you will fix that caller as\n> well ;-)\n\nI'll call that a win.\n\nThe attached patch covers the following:\n- fixes all places where \"make test\" attempts to call exit() with a value <0 or >255\n- changes git-merge-file.txt to reflect the change in exit codes\n- fixes the exit codes for merge-file\n- adds a warning (which should probably be changed to something clearer) if git.c:run_builtin() is going to cause an out-of-bounds exit\n- fixes a typo in t7007-show.sh\n- replaces return(0) with exit(0) in test-parse-options.c\n\nThe diff is cut against 15598cf41beed0d86cd2ac443e0f69c5a3b40321 (2.3.0-rc2)\n\nThanks,\nKeni\n\n\n\n\n\n\n"},{"id":"258804","messageId":"B30AC3FD-A046-40C4-8D37-70835205E446@his.com","threadId":"38185","inReplyTo":"4B764581-0053-467C-93B5-C238EF5AE112@his.com","subject":"Re: bug & patch: exit codes from internal commands are handled incorrectly","fromName":"Kenneth Lorber","fromEmail":"keni@his.com","sentAt":"2015-04-01T00:10:05Z","receivedAt":"2015-04-01T00:10:05Z","isPatch":false,"sender":{"key":"keni@his.com","avatar":"https://avatars.githubusercontent.com/u/16437442?v=4"},"body":"Ping?  The original version of this got some discussion, but this version - nothing.\n\nThanks,\nKeni\n\nOn Feb 1, 2015, at 5:32 PM, Kenneth Lorber <keni@his.com> wrote:\n\n> \n> On Dec 18, 2014, at 2:18 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> \n>> Kenneth Lorber <keni@his.com> writes:\n>> \n>>>> Bug: exit codes from (at least) internal commands are handled incorrectly.\n>>>> E.g. git-merge-file, docs say:\n>>>>      The exit value of this program is negative on error, and the number of\n>>>>      conflicts otherwise. If the merge was clean, the exit value is 0.\n>>>> But only 8 bits get carried through exit, so 256 conflicts gives\n>>>> exit(0), which means the merge was clean.\n>> \n>> Wouldn't any cmd_foo() that returns negative to main() be buggy?\n> \n> Yes.\n> \n>> \n>> Your change sweeps such problems under the rug, which is not a\n>> healthy thing to do.\n> \n> Agreed.  (See below.)\n> \n>> \n>> Expecting that the exit code can signal small positive integers and\n>> other generic kinds of failures is a losing proposition.  I think it\n>> is a better fix to update cmd_merge_file() to return 1 (when ret is\n>> positive), 0 (when ret is zero) or 128 (when ret is negative), or\n>> something simple like that, and update the documentation to match\n>> that, without touching git.c::main().\n> \n> The new patch uses 0, 1, and 2 to match diff(1).\n> \n>> Among the in-tree users, I notice git-cvsserver.perl is already\n>> using the command incorrectly.  It does this:\n>> \n>>           my $return = system(\"git\", \"merge-file\", $file_local, $file_old, $file_new);\n>>           $return >>= 8;\n>> \n>>           cleanupTmpDir();\n>> \n>>           if ( $return == 0 )\n>>           {\n>>               $log->info(\"Merged successfully\");\n>>               ...\n>>           }\n>>           elsif ( $return == 1 )\n>>           {\n>>               $log->info(\"Merged with conflicts\");\n>>               ...\n>>           }\n>>           else\n>>           {\n>>               $log->warn(\"Merge failed\");\n>>               next;\n>>           }\n>> \n>> which assumes $return == 1 is special \"half-good\", which is not\n>> consistent with the documented interface.  It will incorrectly\n>> say \"Merge failed\" when there are two or more conflicts.\n>> \n>> And with such a \"1 or 0 or -1\" change, you will fix that caller as\n>> well ;-)\n> \n> I'll call that a win.\n> \n> The attached patch covers the following:\n> - fixes all places where \"make test\" attempts to call exit() with a value <0 or >255\n> - changes git-merge-file.txt to reflect the change in exit codes\n> - fixes the exit codes for merge-file\n> - adds a warning (which should probably be changed to something clearer) if git.c:run_builtin() is going to cause an out-of-bounds exit\n> - fixes a typo in t7007-show.sh\n> - replaces return(0) with exit(0) in test-parse-options.c\n> \n> The diff is cut against 15598cf41beed0d86cd2ac443e0f69c5a3b40321 (2.3.0-rc2)\n> \n> Thanks,\n> Keni\n> \n> \n> <d2.ship>\n> \n"},{"id":"258805","messageId":"CAPc5daVeyrwO59uC-C4FfOwWtqfBxeZhhmfH6KeW0=2BUX1WHg@mail.gmail.com","threadId":"38185","inReplyTo":"B30AC3FD-A046-40C4-8D37-70835205E446@his.com","subject":"Re: bug & patch: exit codes from internal commands are handled incorrectly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-01T00:40:06Z","receivedAt":"2015-04-01T00:40:06Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Tue, Mar 31, 2015 at 5:10 PM, Kenneth Lorber <keni@his.com> wrote:\n> Ping?  The original version of this got some discussion, but this version - nothing.\n\nPong? I do not know what you meant by \"this version\".\n\nHave you followed Documentaiton/SubmittingPatches? Otherwise the\nmailing list may have dropped the message.\n"}]}