Hi,
Sorry for the delay, I managed to miss this reply.
On Tue, Sep 28, 2021 at 04:56:37AM +0200, Ævar Arnfjörð Bjarmason wrote:
>
> 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@xxxxxxxxxxxx>
> > ---
> > 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?
I guess it could be seen both ways. In my particular use case, I do want
automated testing to be able to catch Leak Sanitizer failures from a
remote-helper.
> > 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)>
transport_disconnect returns the remote-helper exit code for external
helpers, though, but I guess we don't necessarily need to return that
particular exit code.
Mike
