# bug & patch: exit codes from internal commands are handled incorrectly 8 messages from 2014-12-16 to 2015-04-01. Participants: Kenneth Lorber, Torsten Bögershausen, Junio C Hamano. Thread: https://gitlist.dev/t/38185 ## Kenneth Lorber, 2014-12-16 19:28 Subject: bug & patch: exit codes from internal commands are handled incorrectly Message-ID: <13474FB1-5310-42E5-82A9-4047FEFEFF4A@his.com> URL: https://gitlist.dev/e/13474FB1-5310-42E5-82A9-4047FEFEFF4A%40his.com ``` (Apologies if I’ve missed anything here - I’m still climbing the git learning curve.) Bug: exit codes from (at least) internal commands are handled incorrectly. E.g. git-merge-file, docs say: The exit value of this program is negative on error, and the number of conflicts otherwise. If the merge was clean, the exit value is 0. But only 8 bits get carried through exit, so 256 conflicts gives exit(0), which means the merge was clean. Issue shows up on: git version 1.8.5.2 (Apple Git-48) build from source (git version 2.2.0.68.g64396d6.dirty after patch below applied) Reproduce by: Put the following test program in an empty directory as buggen.pl TEST PROGRAM START open OUTB, ">basefile" or die; print OUTB "this is the base file\n"; close OUTB; open OUT1, ">bfile1" or die; open OUT2, ">bfile2" or die; $c = shift; while($c > 0){ print OUT1 "a\nb\nc\nd\nchange $c file 1\n"; print OUT2 "a\nb\nc\nd\nchange $c file 2\n"; $c--; } TEST PROGRAM END Do these tests: perl buggen.pl 0 git merge-file -p bfile1 basefile bfile2 echo $status 0 perl buggen.pl 1 git merge-file -p bfile1 basefile bfile2 echo $status 1 perl buggen.pl 256 git merge-file -p bfile1 basefile bfile2 echo $status 0 <===OOPS Proposed patches: diff --git a/git.c b/git.c index 82d7a1c..8228a3c 100644 --- a/git.c +++ b/git.c @@ -349,6 +349,8 @@ static int run_builtin(struct cmd_struct *p, int argc, const trace_argv_printf(argv, "trace: built-in: git"); status = p->fn(argc, argv, prefix); + if (status > 255) + status = 255; /* prevent exit() from truncating to 0 */ if (status) return status; diff --git a/Documentation/git-merge-file.txt b/Documentation/git-merge-file.txt index d2fc12e..76e6a11 100644 --- a/Documentation/git-merge-file.txt +++ b/Documentation/git-merge-file.txt @@ -41,7 +41,8 @@ lines from ``, or lines from both respectively. T conflict markers can be given with the `--marker-size` option. The exit value of this program is negative on error, and the number of -conflicts otherwise. If the merge was clean, the exit value is 0. +conflicts otherwise (but 255 will be reported even if more than 255 conflicts +exist). If the merge was clean, the exit value is 0. 'git merge-file' is designed to be a minimal clone of RCS 'merge'; that is, it implements all of RCS 'merge''s functionality which is needed by Open questions: 1) Is 255 safe for all operating systems? 2) Does this issue affect any other places? Thanks, Keni keni@his.com ``` ## Kenneth Lorber, 2014-12-18 02:15 Subject: Re: bug & patch: exit codes from internal commands are handled incorrectly Message-ID: <885593BD-3024-4811-83A5-D3A0C4CE6AC1@his.com> URL: https://gitlist.dev/e/885593BD-3024-4811-83A5-D3A0C4CE6AC1%40his.com In-Reply-To: <13474FB1-5310-42E5-82A9-4047FEFEFF4A@his.com> ``` The situation is actually slightly more complex than I stated previously. From the docs: The exit value of this program is negative on error, But there’s no such thing as a negative error code under Unix, so (at best) that will be exit(255). No patch, because this is getting painfully close to needing someone with a global view of the code to fix. Thanks, Keni On Dec 16, 2014, at 2:28 PM, Kenneth Lorber wrote: > (Apologies if I’ve missed anything here - I’m still climbing the git learning curve.) > > Bug: exit codes from (at least) internal commands are handled incorrectly. > E.g. git-merge-file, docs say: > The exit value of this program is negative on error, and the number of > conflicts otherwise. If the merge was clean, the exit value is 0. > But only 8 bits get carried through exit, so 256 conflicts gives exit(0), which means the merge was clean. > > Issue shows up on: > git version 1.8.5.2 (Apple Git-48) > build from source (git version 2.2.0.68.g64396d6.dirty after patch below applied) > > Reproduce by: > > Put the following test program in an empty directory as buggen.pl > > TEST PROGRAM START > open OUTB, ">basefile" or die; > print OUTB "this is the base file\n"; > close OUTB; > > open OUT1, ">bfile1" or die; > open OUT2, ">bfile2" or die; > > $c = shift; > > while($c > 0){ > print OUT1 "a\nb\nc\nd\nchange $c file 1\n"; > print OUT2 "a\nb\nc\nd\nchange $c file 2\n"; > $c--; > } > TEST PROGRAM END > > Do these tests: > > perl buggen.pl 0 > git merge-file -p bfile1 basefile bfile2 > echo $status > 0 > > perl buggen.pl 1 > git merge-file -p bfile1 basefile bfile2 > echo $status > 1 > > perl buggen.pl 256 > git merge-file -p bfile1 basefile bfile2 > echo $status > 0 <===OOPS > > Proposed patches: > diff --git a/git.c b/git.c > index 82d7a1c..8228a3c 100644 > --- a/git.c > +++ b/git.c > @@ -349,6 +349,8 @@ static int run_builtin(struct cmd_struct *p, int argc, const > trace_argv_printf(argv, "trace: built-in: git"); > > status = p->fn(argc, argv, prefix); > + if (status > 255) > + status = 255; /* prevent exit() from truncating to 0 */ > if (status) > return status; > diff --git a/Documentation/git-merge-file.txt b/Documentation/git-merge-file.txt > index d2fc12e..76e6a11 100644 > --- a/Documentation/git-merge-file.txt > +++ b/Documentation/git-merge-file.txt > @@ -41,7 +41,8 @@ lines from ``, or lines from both respectively. T > conflict markers can be given with the `--marker-size` option. > > The exit value of this program is negative on error, and the number of > -conflicts otherwise. If the merge was clean, the exit value is 0. > +conflicts otherwise (but 255 will be reported even if more than 255 conflicts > +exist). If the merge was clean, the exit value is 0. > > 'git merge-file' is designed to be a minimal clone of RCS 'merge'; that is, it > implements all of RCS 'merge''s functionality which is needed by > > Open questions: > 1) Is 255 safe for all operating systems? > 2) Does this issue affect any other places? > > Thanks, > Keni > keni@his.com > ``` ## Torsten Bögershausen, 2014-12-18 17:43 Subject: Re: bug & patch: exit codes from internal commands are handled incorrectly Message-ID: <54931233.6080900@web.de> URL: https://gitlist.dev/e/54931233.6080900%40web.de In-Reply-To: <885593BD-3024-4811-83A5-D3A0C4CE6AC1@his.com> ``` On 18.12.14 03:15, Kenneth Lorber wrote: > The situation is actually slightly more complex than I stated previously. From the docs: > The exit value of this program is negative on error, > But there’s no such thing as a negative error code under Unix, so (at best) that will be exit(255). > > No patch, because this is getting painfully close to needing someone with a global view of the code to fix. > > Thanks, > Keni > My spontanous question: Would it be save to clamp at 127 ? >> + if (status > 127) >> + status = 127; /* prevent exit() from truncating to 0 or becoming negative */ (And the rest looked good enough to become a patch, or at least to start a wider discussion) ``` ## Junio C Hamano, 2014-12-18 19:18 Subject: Re: bug & patch: exit codes from internal commands are handled incorrectly Message-ID: URL: https://gitlist.dev/e/xmqqegrw7omv.fsf%40gitster.dls.corp.google.com In-Reply-To: <885593BD-3024-4811-83A5-D3A0C4CE6AC1@his.com> ``` Kenneth Lorber writes: >> Bug: exit codes from (at least) internal commands are handled incorrectly. >> E.g. git-merge-file, docs say: >> The exit value of this program is negative on error, and the number of >> conflicts otherwise. If the merge was clean, the exit value is 0. >> But only 8 bits get carried through exit, so 256 conflicts gives >> exit(0), which means the merge was clean. Wouldn't any cmd_foo() that returns negative to main() be buggy? Your change sweeps such problems under the rug, which is not a healthy thing to do. Expecting that the exit code can signal small positive integers and other generic kinds of failures is a losing proposition. I think it is a better fix to update cmd_merge_file() to return 1 (when ret is positive), 0 (when ret is zero) or 128 (when ret is negative), or something simple like that, and update the documentation to match that, without touching git.c::main(). Among the in-tree users, I notice git-cvsserver.perl is already using the command incorrectly. It does this: my $return = system("git", "merge-file", $file_local, $file_old, $file_new); $return >>= 8; cleanupTmpDir(); if ( $return == 0 ) { $log->info("Merged successfully"); ... } elsif ( $return == 1 ) { $log->info("Merged with conflicts"); ... } else { $log->warn("Merge failed"); next; } which assumes $return == 1 is special "half-good", which is not consistent with the documented interface. It will incorrectly say "Merge failed" when there are two or more conflicts. And with such a "1 or 0 or -1" change, you will fix that caller as well ;-) ``` ## Kenneth Lorber, 2014-12-20 12:39 Subject: Re: bug & patch: exit codes from internal commands are handled incorrectly Message-ID: <0C0299AC-0180-460D-A486-1A96E322CC5E@his.com> URL: https://gitlist.dev/e/0C0299AC-0180-460D-A486-1A96E322CC5E%40his.com In-Reply-To: ``` On Dec 18, 2014, at 2:18 PM, Junio C Hamano wrote: > Kenneth Lorber writes: > >>> Bug: exit codes from (at least) internal commands are handled incorrectly. >>> E.g. git-merge-file, docs say: >>> The exit value of this program is negative on error, and the number of >>> conflicts otherwise. If the merge was clean, the exit value is 0. >>> But only 8 bits get carried through exit, so 256 conflicts gives >>> exit(0), which means the merge was clean. > > Wouldn't any cmd_foo() that returns negative to main() be buggy? Absolutely. > Your change sweeps such problems under the rug, which is not a > healthy thing to do. True. But as I don’t know the codebase it seemed the safe thing to do at least to prove the bug existed before filing a bug report. > Expecting that the exit code can signal small positive integers and > other generic kinds of failures is a losing proposition. I think it > is a better fix to update cmd_merge_file() to return 1 (when ret is > positive), 0 (when ret is zero) or 128 (when ret is negative), or > something simple like that, and update the documentation to match > that, without touching git.c::main(). I agree. It does leave open the question of whether this bit of information is of any use and should/could be reported out in another way. It’s not to me, so I won’t be proposing anything to address that. > Among the in-tree users, I notice git-cvsserver.perl is already > using the command incorrectly. It does this: > > my $return = system("git", "merge-file", $file_local, $file_old, $file_new); > $return >>= 8; > > cleanupTmpDir(); > > if ( $return == 0 ) > { > $log->info("Merged successfully"); > ... > } > elsif ( $return == 1 ) > { > $log->info("Merged with conflicts"); > ... > } > else > { > $log->warn("Merge failed"); > next; > } > > which assumes $return == 1 is special "half-good", which is not > consistent with the documented interface. It will incorrectly > say "Merge failed" when there are two or more conflicts. > > And with such a "1 or 0 or -1" change, you will fix that caller as > well ;-) Yay! :-) Thanks, Keni ``` ## Kenneth Lorber, 2015-02-01 22:32 Subject: Re: bug & patch: exit codes from internal commands are handled incorrectly Message-ID: <4B764581-0053-467C-93B5-C238EF5AE112@his.com> URL: https://gitlist.dev/e/4B764581-0053-467C-93B5-C238EF5AE112%40his.com In-Reply-To: ``` On Dec 18, 2014, at 2:18 PM, Junio C Hamano wrote: > Kenneth Lorber writes: > >>> Bug: exit codes from (at least) internal commands are handled incorrectly. >>> E.g. git-merge-file, docs say: >>> The exit value of this program is negative on error, and the number of >>> conflicts otherwise. If the merge was clean, the exit value is 0. >>> But only 8 bits get carried through exit, so 256 conflicts gives >>> exit(0), which means the merge was clean. > > Wouldn't any cmd_foo() that returns negative to main() be buggy? Yes. > > Your change sweeps such problems under the rug, which is not a > healthy thing to do. Agreed. (See below.) > > Expecting that the exit code can signal small positive integers and > other generic kinds of failures is a losing proposition. I think it > is a better fix to update cmd_merge_file() to return 1 (when ret is > positive), 0 (when ret is zero) or 128 (when ret is negative), or > something simple like that, and update the documentation to match > that, without touching git.c::main(). The new patch uses 0, 1, and 2 to match diff(1). > Among the in-tree users, I notice git-cvsserver.perl is already > using the command incorrectly. It does this: > > my $return = system("git", "merge-file", $file_local, $file_old, $file_new); > $return >>= 8; > > cleanupTmpDir(); > > if ( $return == 0 ) > { > $log->info("Merged successfully"); > ... > } > elsif ( $return == 1 ) > { > $log->info("Merged with conflicts"); > ... > } > else > { > $log->warn("Merge failed"); > next; > } > > which assumes $return == 1 is special "half-good", which is not > consistent with the documented interface. It will incorrectly > say "Merge failed" when there are two or more conflicts. > > And with such a "1 or 0 or -1" change, you will fix that caller as > well ;-) I'll call that a win. The attached patch covers the following: - fixes all places where "make test" attempts to call exit() with a value <0 or >255 - changes git-merge-file.txt to reflect the change in exit codes - fixes the exit codes for merge-file - 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 - fixes a typo in t7007-show.sh - replaces return(0) with exit(0) in test-parse-options.c The diff is cut against 15598cf41beed0d86cd2ac443e0f69c5a3b40321 (2.3.0-rc2) Thanks, Keni ``` ## Kenneth Lorber, 2015-04-01 00:10 Subject: Re: bug & patch: exit codes from internal commands are handled incorrectly Message-ID: URL: https://gitlist.dev/e/B30AC3FD-A046-40C4-8D37-70835205E446%40his.com In-Reply-To: <4B764581-0053-467C-93B5-C238EF5AE112@his.com> ``` Ping? The original version of this got some discussion, but this version - nothing. Thanks, Keni On Feb 1, 2015, at 5:32 PM, Kenneth Lorber wrote: > > On Dec 18, 2014, at 2:18 PM, Junio C Hamano wrote: > >> Kenneth Lorber writes: >> >>>> Bug: exit codes from (at least) internal commands are handled incorrectly. >>>> E.g. git-merge-file, docs say: >>>> The exit value of this program is negative on error, and the number of >>>> conflicts otherwise. If the merge was clean, the exit value is 0. >>>> But only 8 bits get carried through exit, so 256 conflicts gives >>>> exit(0), which means the merge was clean. >> >> Wouldn't any cmd_foo() that returns negative to main() be buggy? > > Yes. > >> >> Your change sweeps such problems under the rug, which is not a >> healthy thing to do. > > Agreed. (See below.) > >> >> Expecting that the exit code can signal small positive integers and >> other generic kinds of failures is a losing proposition. I think it >> is a better fix to update cmd_merge_file() to return 1 (when ret is >> positive), 0 (when ret is zero) or 128 (when ret is negative), or >> something simple like that, and update the documentation to match >> that, without touching git.c::main(). > > The new patch uses 0, 1, and 2 to match diff(1). > >> Among the in-tree users, I notice git-cvsserver.perl is already >> using the command incorrectly. It does this: >> >> my $return = system("git", "merge-file", $file_local, $file_old, $file_new); >> $return >>= 8; >> >> cleanupTmpDir(); >> >> if ( $return == 0 ) >> { >> $log->info("Merged successfully"); >> ... >> } >> elsif ( $return == 1 ) >> { >> $log->info("Merged with conflicts"); >> ... >> } >> else >> { >> $log->warn("Merge failed"); >> next; >> } >> >> which assumes $return == 1 is special "half-good", which is not >> consistent with the documented interface. It will incorrectly >> say "Merge failed" when there are two or more conflicts. >> >> And with such a "1 or 0 or -1" change, you will fix that caller as >> well ;-) > > I'll call that a win. > > The attached patch covers the following: > - fixes all places where "make test" attempts to call exit() with a value <0 or >255 > - changes git-merge-file.txt to reflect the change in exit codes > - fixes the exit codes for merge-file > - 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 > - fixes a typo in t7007-show.sh > - replaces return(0) with exit(0) in test-parse-options.c > > The diff is cut against 15598cf41beed0d86cd2ac443e0f69c5a3b40321 (2.3.0-rc2) > > Thanks, > Keni > > > > ``` ## Junio C Hamano, 2015-04-01 00:40 Subject: Re: bug & patch: exit codes from internal commands are handled incorrectly Message-ID: URL: https://gitlist.dev/e/CAPc5daVeyrwO59uC-C4FfOwWtqfBxeZhhmfH6KeW0%3D2BUX1WHg%40mail.gmail.com In-Reply-To: ``` On Tue, Mar 31, 2015 at 5:10 PM, Kenneth Lorber wrote: > Ping? The original version of this got some discussion, but this version - nothing. Pong? I do not know what you meant by "this version". Have you followed Documentaiton/SubmittingPatches? Otherwise the mailing list may have dropped the message. ```