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 <jrnie...@gmail.com> 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html