Re: [PATCH] Don't ignore transport_disconnect error codes in fetch and clone

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Tue, Sep 28 2021, Mike Hommey wrote:

> When a remote-helper fails in a way that is not directly visible in the
> remote-helper protocol, the helper failure is ignored by git during
> fetch or clone.
>
> For example, a helper cannot directly report an error during an `import`
> command (short of sending `feature done` to the fast-import file
> descriptor and not sending a `done` later on).
>
> Or if the helper crashes at the wrong moment, git doesn't notice and
> thinks everything went well.
>
> Signed-off-by: Mike Hommey <mh@glandium.org>
> ---
>  builtin/clone.c | 5 +++--
>  builtin/fetch.c | 6 +++---
>  2 files changed, 6 insertions(+), 5 deletions(-)
>
> What I'm not sure about is whether a message should be explicitly
> printed by git itself in those cases.
>
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 66fe66679c..f26fa027c5 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -1398,7 +1398,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  	submodule_progress = transport->progress;
>  
>  	transport_unlock_pack(transport);
> -	transport_disconnect(transport);
> +	err = transport_disconnect(transport);
>  
>  	if (option_dissociate) {
>  		close_object_store(the_repository->objects);
> @@ -1406,7 +1406,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  	}
>  
>  	junk_mode = JUNK_LEAVE_REPO;
> -	err = checkout(submodule_progress);
> +	if (!err)
> +		err = checkout(submodule_progress);
>  
>  	free(remote_name);
>  	strbuf_release(&reflog_msg);

This seems buggy in some cases, e.g. just because we couldn't close()
some final socket we should just not run the checkout at all? Shouldn't
we note the disconnect status, but still see if we can do the checkout
etc?

> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 25740c13df..66bccf6f50 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -1886,7 +1886,7 @@ static int fetch_one(struct remote *remote, int argc, const char **argv,
>  {
>  	struct refspec rs = REFSPEC_INIT_FETCH;
>  	int i;
> -	int exit_code;
> +	int exit_code, disconnect_code;
>  	int maybe_prune_tags;
>  	int remote_via_config = remote_is_configured(remote, 0);
>  


> @@ -1952,9 +1952,9 @@ static int fetch_one(struct remote *remote, int argc, const char **argv,
>  	exit_code = do_fetch(gtransport, &rs);
>  	sigchain_pop(SIGPIPE);
>  	refspec_clear(&rs);
> -	transport_disconnect(gtransport);
> +	disconnect_code = transport_disconnect(gtransport);
>  	gtransport = NULL;
> -	return exit_code;
> +	return exit_code || disconnect_code;
>  }
>  
>  int cmd_fetch(int argc, const char **argv, const char *prefix)

This seems like it really needs fixes in other areas,
i.e. disconnect_git() returns 0 unconditionally, but should check at
least finish_connect(), no?

Also once that's done you'll have a logic error here where you're
conflating exit and error codes, we should not return -1 from main() (we
do in a few cases, but it's nasty)>