Re: [PATCH] run-command: encode signal death as a positive integer

On Sat, Jan 05, 2013 at 02:19:09PM -0800, Jonathan Nieder wrote:

quoted

 Documentation/technical/api-run-command.txt | 6 ++----
 editor.c                                    | 2 +-
 run-command.c                               | 2 +-
 3 files changed, 4 insertions(+), 6 deletions(-)

t/test-terminal.perl imitates the same logic.  It doesn't check for
anything other than whether the exit status is 0, but maybe it would
be worth squashing in the below as a futureproofing measure
nonetheless.

Yeah, I think so. As you say, it does not matter, but it makes sense to
keep our conventions consistent.

Aside from the launch_editor bugfix, the only observable effects of
the above patch I can find are some changed error messages:

	error: external filter cat failed -126
	-> error: external filter cat failed 130

	warning: svnrdump, returned -126
	-> warning: svnrdump, returned 130

Those messages are equally senseless before and after the patch, so
for what it's worth,

Thanks. I agree that change isn't a big deal (I would argue the positive
return is slightly more coherent as it matches what the shell would
report, but I really think user-facing errors should probably not even
mention the exact exit code, as it is just noise most of the time, and
we already complain about signals in wait_or_whine).

I did try auditing the callers of finish_command (and run_command) to
make sure I wasn't regressing anybody, but there are a lot of call
sites. In some cases we immediately say:

  if (finish_command(&child))
          die("failed...");

which is obviously unaffected. But in many cases we pass the exit code
up through several levels. It usually just ends up in exit() or being
collapsed to an error boolean, which is fine, but I may have missed a
spot where it matters. I'd expecting cooking this patch for a while
would flush out any I missed.

Reviewed-by: Jonathan Nieder <redacted>

Thanks (and to J6t) for the review.

-Peff