Re: bug & patch: exit codes from internal commands are handled incorrectly

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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?

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




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]