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 Tue, Mar 31, 2015 at 5:10 PM, Kenneth Lorber k...@his.com 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. -- 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
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) -- 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
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?
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 ;-)
--
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
