Re: [PATCH 2/4] transport-helper: check if remote helper is alive
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:56:35
Felipe Contreras [off-list ref] writes:
Otherwise transport-helper will continue checking for refs and other things what will confuse the user more. ---
Sign-off?
quoted hunk ↗ jump to hunk
git-remote-testgit | 11 +++++++++++ t/t5801-remote-helpers.sh | 19 +++++++++++++++++++ transport-helper.c | 8 ++++++++ 3 files changed, 38 insertions(+)diff --git a/git-remote-testgit b/git-remote-testgit index b395c8d..ca0cf09 100755 --- a/git-remote-testgit +++ b/git-remote-testgit@@ -61,12 +61,23 @@ do echo "feature import-marks=$gitmarks" echo "feature export-marks=$gitmarks" fi + + if test -n "$GIT_REMOTE_TESTGIT_FAILURE" + then + exit -1 + fi + 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 + exit -1 + fi + before=$(git for-each-ref --format='%(refname) %(objectname)') git fast-import "${testgitmarks_args[@]}" --quietdiff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh index f387027..26e9a5b 100755 --- a/t/t5801-remote-helpers.sh +++ b/t/t5801-remote-helpers.sh@@ -166,4 +166,23 @@ test_expect_success 'push ref with existing object' ' 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>&1 | \ + grep "Error while running helper"
This will not care if "git fetch" succeeds or fails and returns the exit code from grep. Perhaps something like this instead? ( GIT_REMOTE_TESTGIT_FAILURE=1 && export GIT_REMOTE_TESTGIT_FAILURE && cd local && test_must_fail git fetch 2>error && grep "Error while running helper" error )
+# We sleep to give fast-export a chance to catch the SIGPIPE +test_expect_failure 'proper failure checks for pushing' ' + (GIT_REMOTE_TESTGIT_FAILURE=1 && + export GIT_REMOTE_TESTGIT_FAILURE && + cd local && + test_must_fail git push --all 2>&1 | \ + grep "Error while running helper"
Ditto.
quoted hunk ↗ jump to hunk
+ ) +' + test_donediff --git a/transport-helper.c b/transport-helper.c index cb3ef7d..dfdfa7a 100644 --- a/transport-helper.c +++ b/transport-helper.c@@ -460,6 +460,10 @@ static int fetch_with_import(struct transport *transport, if (finish_command(&fastimport)) die("Error while running fast-import"); + + if (!check_command(data->helper)) + die("Error while running helper"); + argv_array_free_detached(fastimport.argv); /*@@ -818,6 +822,10 @@ static int push_refs_with_export(struct transport *transport, if (finish_command(&exporter)) die("Error while running fast-export"); + + if (!check_command(data->helper)) + die("Error while running helper"); + push_update_refs_status(data, remote_refs); return 0; }
OK, so the idea is that fetch_with_import() does - get_helper(transport), which spawns a helper process; - get_importer(transport, &fastimport), which spawns a fast-import and make it read from the output of the helper process; - we did finish_command() to wait for the fast-import to finish, expecting that the fast-import would finish when the helper stops feeding it, which in turn would mean the helper would have died. The same for the pushing side. Shouldn't transport_disconnect() have called release_helper() which in turn calls disconnect_helper() to call finish_command() on the helper to wait for that procesanyway? Is somebody discarding return value from transport_disconnect() or the current calling site of transport_disconnect() is too late to notice the error? Puzzled...