Re: [PATCH] transport-helper: check when helpers fail

Felipe Contreras [off-list ref] writes:

Fun.

It is nice to have a separate helper that would theoretically be
useful by anybody who runs a child process, but I have to wonder:

 - How other codepaths that run child process check their results?
   Do they ignore the result altogether?  Do they check the results
   in an ad-hoc way without a good reason?  Do they check the
   results differently because their error handling need to be
   different?

   With this, I am not requesting to port these other codepaths to
   use this helper in this patch series.  But designing the helper
   with potential others' use in mind is within the scope of this
   patch.

 - How does the caller use the return value from this helper?  I can
   see that "zero is success, non-zero is failure", unless there is
   a platform what defines a signum 0 for some signal that can kill
   the child process.  But I am not sure if the caller can tell more
   than that from the return value ("did it die with a signal, or
   did it exit with non-zero status"?)

   It looks to me that this helper loses information; does it belong
   to the public part of API in run-command.c?

Please be nicer to people who come *after* you are done with this
change.  Forcing failure will propagate to any new test added after
this piece with this patch.

Perhaps like this:

        test_expect_success 'proper failure checks' '
		(
	                GIT_REMOTE_TESTGIT_FAILURE=1 &&
	                export GIT_REMOTE_TESTGIT_FAILURE &&
	                test_must_fail git clone "testgit::$PWD/server" failure
		) 2>errors &&
                grep "Error while running helper" errors
        '

Points to note:

 - The environment does not propagate outside this test this way.

 - Avoid "export VAR=VAL" to help other people's shell; cf. 69ae92b
   (shell portability: no "export VAR=VAL", 2010-10-13).

 - Detect uncontrolled exit of "git clone" by using test_must_fail.

 - Avoid "grep -q"; cf. the latter half of aadbe44 (grep portability
   fix: don't use "-e" or "-q", 2008-03-12).

Thanks.

On Sun, Oct 21, 2012 at 10:33 PM, Junio C Hamano [off-list ref] wrote:

Felipe Contreras [off-list ref] writes:

quoted

+int check_command(struct child_process *cmd)
+{
+     int status;
+     pid_t pid;
+
+     pid = waitpid(cmd->pid, &status, WNOHANG);
+
+     if (pid < 0)
+             return -1;
+     if (WIFSIGNALED(status))
+             return WTERMSIG(status);
+     if (WIFEXITED(status))
+             return WEXITSTATUS(status);
+
+     return 0;
+}

It is nice to have a separate helper that would theoretically be
useful by anybody who runs a child process, but I have to wonder:

 - How other codepaths that run child process check their results?
   Do they ignore the result altogether?  Do they check the results
   in an ad-hoc way without a good reason?  Do they check the
   results differently because their error handling need to be
   different?

They probably check at the end, truly waiting for the process to
finish, that's not what we want, thus the WNOHANG that apparently
nobody else in git is using.

The reason is that the transport-helpers are special; they are
re-used, so we can't wait for the process to finish after running a
certain command.

   With this, I am not requesting to port these other codepaths to
   use this helper in this patch series.  But designing the helper
   with potential others' use in mind is within the scope of this
   patch.

That's what I did. If somebody else needs to check the status of the
command mid-stream, they can do that.

 - How does the caller use the return value from this helper?  I can
   see that "zero is success, non-zero is failure", unless there is
   a platform what defines a signum 0 for some signal that can kill
   the child process.  But I am not sure if the caller can tell more
   than that from the return value ("did it die with a signal, or
   did it exit with non-zero status"?)

I don't really care, I'm fine returning -1 on all errors.

quoted

diff --git a/t/t5800-remote-helpers.sh b/t/t5800-remote-helpers.sh
index e7dc668..d4b17ae 100755
--- a/t/t5800-remote-helpers.sh
+++ b/t/t5800-remote-helpers.sh

@@ -145,4 +145,10 @@ test_expect_failure 'push new branch with old:new refspec' '
      compare_refs clone HEAD server refs/heads/new-refspec
 '

+test_expect_success 'proper failure checks' '
+     export GIT_REMOTE_TESTGIT_FAILURE=1 &&
+     ! git clone "testgit::$PWD/server" failure 2> errors &&
+     grep -q "Error while running helper" errors
+'
+
 test_done

Please be nicer to people who come *after* you are done with this
change.  Forcing failure will propagate to any new test added after
this piece with this patch.

Perhaps like this:

        test_expect_success 'proper failure checks' '
                (
                        GIT_REMOTE_TESTGIT_FAILURE=1 &&
                        export GIT_REMOTE_TESTGIT_FAILURE &&
                        test_must_fail git clone "testgit::$PWD/server" failure
                ) 2>errors &&
                grep "Error while running helper" errors
        '

LGTM.

-- 
Felipe Contreras