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

From: Kenneth Lorber <keni@his.com>
Date: Dec 18, 2014, 02:15 UTC
Message-ID: <885593BD-3024-4811-83A5-D3A0C4CE6AC1@his.com>
In-Reply-To: <13474FB1-5310-42E5-82A9-4047FEFEFF4A@his.com>

The situation is actually slightly more complex than I stated previously.  From the docs:
  The exit value of this program is negative on error,
But there’s no such thing as a negative error code under Unix, so (at best) that will be exit(255).

No patch, because this is getting painfully close to needing someone with a global view of the code to fix.

Thanks, Keni

On Dec 16, 2014, at 2:28 PM, Kenneth Lorber <keni@his.com> wrote:

Show 86 quoted lines

> (Apologies if I’ve missed anything here - I’m still climbing the git learning curve.)
> 
> 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.
> 
> Issue shows up on:
> git version 1.8.5.2 (Apple Git-48)
> build from source (git version 2.2.0.68.g64396d6.dirty after patch below applied)
> 
> Reproduce by:
> 
> Put the following test program in an empty directory as buggen.pl
> 
> TEST PROGRAM START
> open OUTB, ">basefile" or die;
> print OUTB "this is the base file\n";
> close OUTB;
> 
> open OUT1, ">bfile1" or die;
> open OUT2, ">bfile2" or die;
> 
> $c = shift;
> 
> while($c > 0){
>        print OUT1 "a\nb\nc\nd\nchange $c file 1\n";
>        print OUT2 "a\nb\nc\nd\nchange $c file 2\n";
>        $c--;
> }
> TEST PROGRAM END
> 
> Do these tests:
> 
> perl buggen.pl 0
> git merge-file -p bfile1 basefile bfile2
> echo $status
> 0
> 
> perl buggen.pl 1
> git merge-file -p bfile1 basefile bfile2
> echo $status
> 1
> 
> perl buggen.pl 256
> git merge-file -p bfile1 basefile bfile2
> echo $status
> 0       <===OOPS
> 
> Proposed patches:
> diff --git a/git.c b/git.c
> index 82d7a1c..8228a3c 100644
> --- a/git.c
> +++ b/git.c
> @@ -349,6 +349,8 @@ static int run_builtin(struct cmd_struct *p, int argc, const
>        trace_argv_printf(argv, "trace: built-in: git");
> 
>        status = p->fn(argc, argv, prefix);
> +       if (status > 255)
> +               status = 255;   /* prevent exit() from truncating to 0 */
>        if (status)
>                return status;
> diff --git a/Documentation/git-merge-file.txt b/Documentation/git-merge-file.txt
> index d2fc12e..76e6a11 100644
> --- a/Documentation/git-merge-file.txt
> +++ b/Documentation/git-merge-file.txt
> @@ -41,7 +41,8 @@ lines from `<other-file>`, or lines from both respectively.  T
> conflict markers can be given with the `--marker-size` option.
> 
> 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.
> +conflicts otherwise (but 255 will be reported even if more than 255 conflicts
> +exist). If the merge was clean, the exit value is 0.
> 
> 'git merge-file' is designed to be a minimal clone of RCS 'merge'; that is, it
> implements all of RCS 'merge''s functionality which is needed by
> 
> Open questions:
> 1) Is 255 safe for all operating systems?
> 2) Does this issue affect any other places?
> 
> Thanks,
> Keni
> keni@his.com
>