Re: bug patch: exit codes from internal commands are handled incorrectly
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 k...@his.com wrote: On Dec 18, 2014, at 2:18 PM, Junio C Hamano gits...@pobox.com wrote: Kenneth Lorber k...@his.com 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 d2.ship -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: bug patch: exit codes from internal commands are handled incorrectly
On Dec 18, 2014, at 2:18 PM, Junio C Hamano gits...@pobox.com wrote: Kenneth Lorber k...@his.com 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 d2.ship Description: Binary data
Re: bug patch: exit codes from internal commands are handled incorrectly
On Dec 18, 2014, at 2:18 PM, Junio C Hamano gits...@pobox.com wrote: Kenneth Lorber k...@his.com 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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: bug patch: exit codes from internal commands are handled incorrectly
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 k...@his.com 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 `other-file`, 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 k...@his.com -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
bug patch: exit codes from internal commands are handled incorrectly
(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 `other-file`, 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 k...@his.com -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html