Kenneth Lorber <keni@xxxxxxx> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html