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

From: Jeff King <peff@peff.net>
Date: Jan 5, 2013, 23:12 UTC
Message-ID: <20130105231248.GA4140@sigill.intra.peff.net>
In-Reply-To: <20130105221909.GA3247@elie.Belkin>

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

Show 9 quoted lines

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

Show 11 quoted lines

> 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 <jrnieder@gmail.com>

Thanks (and to J6t) for the review.

-Peff