Felipe Contreras <felipe.contreras@xxxxxxxxx> writes:
> Otherwise transport-helper will continue checking for refs and other
> things what will confuse the user more.
> ---
Sign-off?
> 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[@]}" --quiet
> diff --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.
> + )
> +'
> +
> test_done
> diff --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...
--
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