From: Junio C Hamano <hidden> Date: 2016-06-15 23:03:18
Kenneth Lorber [off-list ref] writes:
quoted
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 ;-)
On Dec 18, 2014, at 2:18 PM, Junio C Hamano [off-list ref] wrote:
Kenneth Lorber [off-list ref] writes:
quoted
quoted
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 ;-)
On Dec 18, 2014, at 2:18 PM, Junio C Hamano [off-list ref] wrote:
Kenneth Lorber [off-list ref] writes:
quoted
quoted
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
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 [off-list ref] wrote:
On Dec 18, 2014, at 2:18 PM, Junio C Hamano [off-list ref] wrote:
quoted
Kenneth Lorber [off-list ref] writes:
quoted
quoted
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.
quoted
Your change sweeps such problems under the rug, which is not a
healthy thing to do.
Agreed. (See below.)
quoted
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).
quoted
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>
From: Junio C Hamano <hidden> Date: 2016-06-15 23:04:20
On Tue, Mar 31, 2015 at 5:10 PM, Kenneth Lorber [off-list ref] 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.