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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 21, 2012, 20:33 UTC
Message-ID: <7v7gqjftuj.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1350847158-14154-1-git-send-email-felipe.contreras@gmail.com>

Felipe Contreras <felipe.contreras@gmail.com> writes:

Show 23 quoted lines

> Otherwise transport-helper will continue checking for refs and other
> things what will confuse the user more.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  git-remote-testgit.py     |  3 +++
>  run-command.c             | 17 +++++++++++++++++
>  run-command.h             |  1 +
>  t/t5800-remote-helpers.sh |  6 ++++++
>  transport-helper.c        |  8 ++++++++
>  5 files changed, 35 insertions(+)
>
> diff --git a/git-remote-testgit.py b/git-remote-testgit.py
> index 5f3ebd2..355e3f5 100644
> --- a/git-remote-testgit.py
> +++ b/git-remote-testgit.py
> @@ -159,6 +159,9 @@ def do_import(repo, args):
>          ref = line[7:].strip()
>          refs.append(ref)
>  
> +    if os.environ.get("GIT_REMOTE_TESTGIT_FAILURE"):
> +        die('Told to fail')
> +

Fun.

Show 24 quoted lines

> diff --git a/run-command.c b/run-command.c
> index 1101ef7..2852e9d 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -559,6 +559,23 @@ int run_command(struct child_process *cmd)
>  	return finish_command(cmd);
>  }
>  
> +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?

   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?

Show 15 quoted lines

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

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.