Re: [PATCH v4] transport-helper: report errors properly
On Wed, Apr 10, 2013 at 04:13:20PM -0700, rh wrote:
> > +++ b/transport-helper.c
> > @@ -54,7 +54,7 @@ static int recvline_fh(FILE *helper, struct strbuf
> > *buffer) if (strbuf_getline(buffer, helper, '\n') == EOF) {
> > if (debug)
> > fprintf(stderr, "Debug: Remote helper quit.
> > \n");
> > - exit(128);
> > + die("Reading from remote helper failed");
>
> Do I read this correctly? If I'm in debug mode the remote helper quit
> but if not in debug mode it failed? Debuggers never fail they only quit!
In debug mode, it prints both messages. The debug version is superfluous
at this point, though, and we can probably just drop it.
-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
Re: [PATCH v4] transport-helper: report errors properly
On Tue, Apr 09, 2013 at 11:38:05PM +0200, Thomas Rast wrote: > Two out of six of these loops quit within 1 and 2 iterations, > respectively, both with an error along the lines of: > > expecting success: > (GIT_REMOTE_TESTGIT_FAILURE=1 && > export GIT_REMOTE_TESTGIT_FAILURE && > cd local && > test_must_fail git push --all 2> error && > cat error && > grep -q "Reading from remote helper failed" error > ) > > error: fast-export died of signal 13 > fatal: Error while running fast-export > not ok 21 - proper failure checks for pushing > > I haven't been able to reproduce outside of valgrind tests. Is this an > expected issue, caused by overrunning the sleep somehow? If so, can you > increase the sleep delay under valgrind so as to not cause intermittent > failures in the test suite? Yeah, I am not too surprised. The failing helper sleeps before exiting so that fast-export puts all of its data into the pipe buffer before the helper dies, and does not get SIGPIPE. But obviously the sleep is just delaying the problem if your fast-export runs really slowly (which, if you are running under valgrind, is a possibility). The helper should instead just consume all of fast-export's input before exiting, which accomplishes the same thing, finishes sooner in the normal case, and doesn't race. And I think it also simulates a reasonable real-world setup (a helper reads and converts the data, but then dies while writing the output to disk, the network, or whatever). I posted review comments, including that, and I'm assuming that Felipe is going to re-roll at some point. -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
Re: [PATCH v4] transport-helper: report errors properly
Felipe Contreras writes:
> If a push fails because the remote-helper died (with fast-export), the
> user won't see any error message. So let's add one.
>
> At the same time lets add tests to ensure this error is reported, and
> while we are at it, check the error from fast-import
[...]
> +# We sleep to give fast-export a chance to catch the SIGPIPE
> +test_expect_success 'proper failure checks for pushing' '
> + (GIT_REMOTE_TESTGIT_FAILURE=1 &&
> + export GIT_REMOTE_TESTGIT_FAILURE &&
> + cd local &&
> + test_must_fail git push --all 2> error &&
> + cat error &&
> + grep -q "Reading from remote helper failed" error
> + )
> +'
There appears to be a race in the version that is in today's pu
(5eb25f737b). I reproduced with this:
cd git/t
i=1
while ./t5801-remote-helpers.sh --root=/dev/shm --valgrind
do
i=$(($i+1))
done
Two out of six of these loops quit within 1 and 2 iterations,
respectively, both with an error along the lines of:
expecting success:
(GIT_REMOTE_TESTGIT_FAILURE=1 &&
export GIT_REMOTE_TESTGIT_FAILURE &&
cd local &&
test_must_fail git push --all 2> error &&
cat error &&
grep -q "Reading from remote helper failed" error
)
error: fast-export died of signal 13
fatal: Error while running fast-export
not ok 21 - proper failure checks for pushing
I haven't been able to reproduce outside of valgrind tests. Is this an
expected issue, caused by overrunning the sleep somehow? If so, can you
increase the sleep delay under valgrind so as to not cause intermittent
failures in the test suite?
--
Thomas Rast
trast@{inf,student}.ethz.ch
--
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
Re: [PATCH v4] transport-helper: report errors properly
On Mon, Apr 08, 2013 at 11:20:15AM -0700, Sverre Rabbelier wrote:
> On Mon, Apr 8, 2013 at 7:40 AM, Felipe Contreras
> wrote:
> > + die("Reading from remote helper failed");
>
> Does the user know what a remote helper is? Could we point them at
> some helpful docs in case they don't?
That's a good point. I wonder if it would be enough to say:
fatal: Reading from helper git-remote-X failed
That might make it more clear what the helper's role is, and showing the
command name gives the user a starting point for running "man".
-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
Re: [PATCH v4] transport-helper: report errors properly
On Mon, Apr 08, 2013 at 09:40:04AM -0500, Felipe Contreras wrote: > If a push fails because the remote-helper died (with fast-export), the > user won't see any error message. So let's add one. > > At the same time lets add tests to ensure this error is reported, and > while we are at it, check the error from fast-import Thanks, I think this patch is definitely the right direction. It seems like there is a lot of back-story that had to be clarified during the review/discussion. Is there a reason not to summarize it here so later readers of this commit are enlightened? I'm thinking something like: If a push fails because the remote-helper died (with fast-export), the user does not see any error message. We do correctly die with a failed exit code, as we notice that the helper has died while reading back the ref status from the helper. However, we don't print any message. This is OK if the helper itself printed a useful error message, but we cannot count on that; let's let the user know that the helper failed. In the long run, it may make more sense to propagate the error back up to push, so that it can present the usual status table and give a nicer message. But this is a much simpler fix that can help immediately. While we're adding tests, let's also confirm that the remote-helper dying is also detect when importing refs. We currently do so robustly when the helper uses the "done" feature (and that is what we test). We cannot do so reliably when the helper does not use the "done" feature, but it is not even worth testing; the right solution is for the helper to start using "done". > export) > + if test -n "$GIT_REMOTE_TESTGIT_FAILURE" > + then > + sleep 1 # don't let fast-export get SIGPIPE > + exit 1 > + fi We can do away with this sleep with: while read line; do test "$line" = "done" && break done The version I posted yesterday had both the read and the sleep, but the sleep was only necessary there to demonstrate the race with check_command. > +# We sleep to give fast-export a chance to catch the SIGPIPE > +test_expect_success 'proper failure checks for pushing' ' I think we can drop this comment now, right? -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
Re: [PATCH v4] transport-helper: report errors properly
On Mon, Apr 8, 2013 at 7:40 AM, Felipe Contreras
wrote:
> + die("Reading from remote helper failed");
Does the user know what a remote helper is? Could we point them at
some helpful docs in case they don't?
--
Cheers,
Sverre Rabbelier
--
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
