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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 18, 2014, 19:18 UTC
Message-ID: <xmqqegrw7omv.fsf@gitster.dls.corp.google.com>
In-Reply-To: <885593BD-3024-4811-83A5-D3A0C4CE6AC1@his.com>

Kenneth Lorber <keni@his.com> writes:

Show 6 quoted lines

>> 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 ;-)