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

[Felipe Contreras <felipe.contreras@gmail.com>]

On Sun, Oct 21, 2012 at 10:33 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:

>> +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.

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