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

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

[Kenneth Lorber]

(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

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

[Kenneth Lorber]

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:

> (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
> 

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

[Torsten Bögershausen]

On 18.12.14 03:15, Kenneth Lorber wrote:
> 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
> 
My spontanous question:
Would it be save to clamp at 127 ?

>> +       if (status > 127)
>> +               status = 127;   /* prevent exit() from truncating to 0 or becoming negative */


(And the rest looked good enough to become a patch, 
or at least to start a wider discussion)

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

[Junio C Hamano]

Kenneth Lorber <keni@his.com> 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 ;-)

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

[Kenneth Lorber]

On Dec 18, 2014, at 2:18 PM, Junio C Hamano <gitster@pobox.com> wrote:

> Kenneth Lorber <keni@his.com> 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

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

[Kenneth Lorber]

[-- Attachment #1: Type: text/plain, Size: 2727 bytes --]


On Dec 18, 2014, at 2:18 PM, Junio C Hamano <gitster@pobox.com> wrote:

> Kenneth Lorber <keni@his.com> 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



[-- Attachment #2: d2.ship --]
[-- Type: application/octet-stream, Size: 8475 bytes --]

diff --git a/Documentation/git-merge-file.txt b/Documentation/git-merge-file.txt
index d2fc12e..425dba3 100644
--- a/Documentation/git-merge-file.txt
+++ b/Documentation/git-merge-file.txt
@@ -40,8 +40,8 @@ however, these conflicts are resolved favouring lines from `<current-file>`,
 lines from `<other-file>`, or lines from both respectively.  The length of the
 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.
+The exit value of this program is 0 if the merge was clean, 1 if there were
+conflicts, and 2 if an error occured.
 
 '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
diff --git a/builtin/branch.c b/builtin/branch.c
index dc6f0b2..e586846 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -938,10 +938,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 			strbuf_release(&branch_ref);
 
 			if (!argc)
-				return error(_("No commit on branch '%s' yet."),
+				return -error(_("No commit on branch '%s' yet."),
 					     branch_name);
 			else
-				return error(_("No branch named '%s'."),
+				return -error(_("No branch named '%s'."),
 					     branch_name);
 		}
 		strbuf_release(&branch_ref);
diff --git a/builtin/check-attr.c b/builtin/check-attr.c
index 5600ec3..62cacf3 100644
--- a/builtin/check-attr.c
+++ b/builtin/check-attr.c
@@ -169,7 +169,7 @@ int cmd_check_attr(int argc, const char **argv, const char *prefix)
 			name = argv[i];
 			a = git_attr(name);
 			if (!a)
-				return error("%s: not a valid attribute name",
+				return -error("%s: not a valid attribute name",
 					name);
 			check[i].attr = a;
 		}
diff --git a/builtin/config.c b/builtin/config.c
index 15a7bea..23aab25 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -667,7 +667,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		ret = git_config_rename_section_in_file(given_config_source.file,
 							argv[0], argv[1]);
 		if (ret < 0)
-			return ret;
+			return -ret;
 		if (ret == 0)
 			die("No such section!");
 	}
@@ -678,7 +678,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		ret = git_config_rename_section_in_file(given_config_source.file,
 							argv[0], NULL);
 		if (ret < 0)
-			return ret;
+			return -ret;
 		if (ret == 0)
 			die("No such section!");
 	}
diff --git a/builtin/log.c b/builtin/log.c
index 923ffe7..cb6914b 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -561,7 +561,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)
 				break;
 			o = parse_object(t->tagged->sha1);
 			if (!o)
-				ret = error(_("Could not read object %s"),
+				ret = -error(_("Could not read object %s"),
 					    sha1_to_hex(t->tagged->sha1));
 			objects[i].item = o;
 			i--;
@@ -585,7 +585,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)
 			ret = cmd_log_walk(&rev);
 			break;
 		default:
-			ret = error(_("Unknown type: %d"), o->type);
+			ret = -error(_("Unknown type: %d"), o->type);
 		}
 	}
 	free(objects);
diff --git a/builtin/merge-file.c b/builtin/merge-file.c
index 844f84f..8dfb42c 100644
--- a/builtin/merge-file.c
+++ b/builtin/merge-file.c
@@ -61,9 +61,11 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
 	if (argc != 3)
 		usage_with_options(merge_file_usage, options);
 	if (quiet) {
-		if (!freopen("/dev/null", "w", stderr))
-			return error("failed to redirect stderr to /dev/null: "
+		if (!freopen("/dev/null", "w", stderr)) {
+			error("failed to redirect stderr to /dev/null: "
 				     "%s", strerror(errno));
+			return 2;
+		}
 	}
 
 	if (prefix)
@@ -74,10 +76,12 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
 		if (!names[i])
 			names[i] = argv[i];
 		if (read_mmfile(mmfs + i, fname))
-			return -1;
-		if (buffer_is_binary(mmfs[i].ptr, mmfs[i].size))
-			return error("Cannot merge binary files: %s",
+			return 2;
+		if (buffer_is_binary(mmfs[i].ptr, mmfs[i].size)) {
+			error("Cannot merge binary files: %s",
 					argv[i]);
+			return 2;
+		}
 	}
 
 	xmp.ancestor = names[1];
@@ -100,7 +104,11 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
 		else if (fclose(f))
 			ret = error("Could not close %s", filename);
 		free(result.ptr);
+
+		if (ret == -1) return 2;	/* error */
+		if (ret == 0) return 0;		/* no conflicts */
+		return 1;			/* conflicts */
 	}
 
-	return ret;
+	return 2;	/* error */
 }
diff --git a/builtin/replace.c b/builtin/replace.c
index 85d39b5..5ff7ee1 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -299,7 +299,7 @@ static int edit_and_replace(const char *object_ref, int force, int raw)
 	free(tmpfile);
 
 	if (!hashcmp(old, new))
-		return error("new object is the same as the old one: '%s'", sha1_to_hex(old));
+		return -error("new object is the same as the old one: '%s'", sha1_to_hex(old));
 
 	return replace_object_sha1(object_ref, old, "replacement", new, force);
 }
@@ -410,7 +410,7 @@ static int create_graft(int argc, const char **argv, int force)
 	strbuf_release(&buf);
 
 	if (!hashcmp(old, new))
-		return error("new commit is the same as the old one: '%s'", sha1_to_hex(old));
+		return -error("new commit is the same as the old one: '%s'", sha1_to_hex(old));
 
 	return replace_object_sha1(old_ref, old, "replacement", new, force);
 }
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index b564a77..7b7f6cd 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -277,7 +277,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 
 	/* match them up */
 	if (match_push_refs(local_refs, &remote_refs, nr_refspecs, refspecs, flags))
-		return -1;
+		return 1;
 
 	if (!is_empty_cas(&cas))
 		apply_push_cas(&cas, remote, remote_refs);
@@ -307,5 +307,5 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 	if (!ret && !transport_refs_pushed(remote_refs))
 		fprintf(stderr, "Everything up-to-date\n");
 
-	return ret;
+	return (ret<0) ? -ret : ret;
 }
diff --git a/git.c b/git.c
index 6b5ae6a..b6a75e9 100644
--- a/git.c
+++ b/git.c
@@ -346,6 +346,8 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
 	trace_argv_printf(argv, "trace: built-in: git");
 
 	status = p->fn(argc, argv, prefix);
+	if(status < 0 || status >255)
+		warning("internal bad status (no effect on the git command)");
 	if (status)
 		return status;
 
diff --git a/run-command.c b/run-command.c
index 0b432cc..353acc8 100644
--- a/run-command.c
+++ b/run-command.c
@@ -567,6 +567,14 @@ int run_command(struct child_process *cmd)
 	return finish_command(cmd);
 }
 
+int run_command2(struct child_process *cmd)
+{
+	int code = start_command(cmd);
+	if (code)
+		return code<0 ? -code : code;
+	return finish_command(cmd);
+}
+
 int run_command_v_opt(const char **argv, int opt)
 {
 	return run_command_v_opt_cd_env(argv, opt, NULL, NULL);
diff --git a/run-command.h b/run-command.h
index d6868dc..afa0212 100644
--- a/run-command.h
+++ b/run-command.h
@@ -51,6 +51,7 @@ void child_process_init(struct child_process *);
 int start_command(struct child_process *);
 int finish_command(struct child_process *);
 int run_command(struct child_process *);
+int run_command2(struct child_process *);
 
 extern char *find_hook(const char *name);
 LAST_ARG_MUST_BE_NULL
diff --git a/t/t7007-show.sh b/t/t7007-show.sh
index 1b824fe..e7272bb 100755
--- a/t/t7007-show.sh
+++ b/t/t7007-show.sh
@@ -13,7 +13,7 @@ test_expect_success setup '
 	rm -f .git/objects/$HH/$H38
 '
 
-test_expect_success 'showing a tag that point at a missing object' '
+test_expect_success 'showing a tag that points at a missing object' '
 	test_must_fail git --no-pager show foo-tag
 '
 
diff --git a/test-parse-options.c b/test-parse-options.c
index 5dabce6..8dba84f 100644
--- a/test-parse-options.c
+++ b/test-parse-options.c
@@ -97,5 +97,5 @@ int main(int argc, char **argv)
 	for (i = 0; i < argc; i++)
 		printf("arg %02d: %s\n", i, argv[i]);
 
-	return 0;
+	exit(0);
 }
diff --git a/test-run-command.c b/test-run-command.c
index 89c7de2..4d8c230 100644
--- a/test-run-command.c
+++ b/test-run-command.c
@@ -28,7 +28,7 @@ int main(int argc, char **argv)
 		return 1;
 	}
 	if (!strcmp(argv[1], "run-command"))
-		exit(run_command(&proc));
+		exit(run_command2(&proc));
 
 	fprintf(stderr, "check usage\n");
 	return 1;

[-- Attachment #3: Type: text/plain, Size: 2 bytes --]

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

[Kenneth Lorber]

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@his.com> wrote:

> 
> On Dec 18, 2014, at 2:18 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 
>> Kenneth Lorber <keni@his.com> 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>
> 

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

[Junio C Hamano]

On Tue, Mar 31, 2015 at 5:10 PM, Kenneth Lorber <keni@his.com> wrote:
> Ping?  The original version of this got some discussion, but this version - nothing.

Pong? I do not know what you meant by "this version".

Have you followed Documentaiton/SubmittingPatches? Otherwise the
mailing list may have dropped the message.