transport-helper: report errors properly

If a push fails because the remote-helper died (with fast-export),
the user may 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 detected 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".

Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
Acked-by: Sverre Rabbelier <srabbelier@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/git-remote-testgit b/git-remote-testgit
index b395c8de59c33768f1a957248447d162bda8ef94..5fd09f965a644a279ac79f5f1d0748650744e922 100755 (executable)
--- a/git-remote-testgit
+++ b/git-remote-testgit
@@ -61,12 +61,31 @@ do
                        echo "feature import-marks=$gitmarks"
                        echo "feature export-marks=$gitmarks"
                fi
                        echo "feature import-marks=$gitmarks"
                        echo "feature export-marks=$gitmarks"
                fi

+
+               if test -n "$GIT_REMOTE_TESTGIT_FAILURE"
+               then
+                       echo "feature done"
+                       exit 1
+               fi
+

                echo "feature done"
                git fast-export "${testgitmarks_args[@]}" $refs |
                sed -e "s#refs/heads/#${prefix}/heads/#g"
                echo "done"
                ;;
        export)
                echo "feature done"
                git fast-export "${testgitmarks_args[@]}" $refs |
                sed -e "s#refs/heads/#${prefix}/heads/#g"
                echo "done"
                ;;
        export)

+               if test -n "$GIT_REMOTE_TESTGIT_FAILURE"
+               then
+                       # consume input so fast-export doesn't get SIGPIPE;
+                       # git would also notice that case, but we want
+                       # to make sure we are exercising the later
+                       # error checks
+                       while read line; do
+                               test "done" = "$line" && break
+                       done
+                       exit 1
+               fi
+

                before=$(git for-each-ref --format='%(refname) %(objectname)')
 
                git fast-import "${testgitmarks_args[@]}" --quiet
                before=$(git for-each-ref --format='%(refname) %(objectname)')
 
                git fast-import "${testgitmarks_args[@]}" --quiet

diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
index f387027c05cf502c6892c7694598bef34376be92..aafc46ac94a565f6668fdc95824a12ec3167429e 100755 (executable)
--- a/t/t5801-remote-helpers.sh
+++ b/t/t5801-remote-helpers.sh
@@ -166,4 +166,24 @@ test_expect_success 'push ref with existing object' '
        compare_refs local dup server dup
 '
 
        compare_refs local dup server dup
 '
 

+test_expect_success 'proper failure checks for fetching' '
+       (GIT_REMOTE_TESTGIT_FAILURE=1 &&
+       export GIT_REMOTE_TESTGIT_FAILURE &&
+       cd local &&
+       test_must_fail git fetch 2> error &&
+       cat error &&
+       grep -q "Error while running fast-import" error
+       )
+'
+
+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
+       )
+'
+

 test_done
 test_done

diff --git a/transport-helper.c b/transport-helper.c
index cb3ef7d38e475480a5d81ce6077c4c87ea09a318..96081cc392efcf6656fd6eade959bd13961fe8b0 100644 (file)
--- a/transport-helper.c
+++ 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");
        if (strbuf_getline(buffer, helper, '\n') == EOF) {
                if (debug)
                        fprintf(stderr, "Debug: Remote helper quit.\n");

-               exit(128);
+               die("Reading from remote helper failed");

        }
 
        if (debug)
        }
 
        if (debug)