Felipe Contreras <felipe.contreras@xxxxxxxxx> writes:
> Otherwise transport-helper will continue checking for refs and other
> things what will confuse the user more.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@xxxxxxxxx>
> ---
> 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.
> 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?
> 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.
--
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