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

> 
> 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)>