On Sat, Jan 05, 2013 at 02:19:09PM -0800, Jonathan Nieder wrote:
> > 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 <jrnieder@xxxxxxxxx>
Thanks (and to J6t) for the review.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html