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 <keni@xxxxxxx> wrote:
>
> On Dec 18, 2014, at 2:18 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
>> 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?
>
> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html