From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Mike Hommey <mh@glandium.org>
Cc: git@vger.kernel.org, gitster@pobox.com
Subject: Re: [PATCH] Don't ignore transport_disconnect error codes in fetch and clone
Date: Tue, 05 Oct 2021 14:21:49 +0200 [thread overview]
Message-ID: <87h7dvlko2.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <20211005045412.pqwsid6gpprxajbw@glandium.org>
On Tue, Oct 05 2021, Mike Hommey wrote:
> 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@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?
>
> 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.
I'd like that too, but I'm pointing out something unrelated (or at least
I think I am), which is that this is changing the equivalent of this code:
int fd = open(...);
close(fd);
do_stuff();
To be:
int err;
int fd = open(...); /* check err here too, but whatever, pseuodocode() */
if (!close(fd))
do_stuff();
But just with sockets, i.e. just because transport_disconnect() had
*some* error are we really confident that checkout() should not be run?
*Maybe*, but at the very least shouldn't we distinguish some of those
error states? Isn't any serious protocol error going to be caught
earlier transport_disconnect() is just going to give us the equivalent
of not being able to cleanly close the socket?
Except as noted in my second comment it doesn't, since we ignore
disconnect_git(). At this point I can't recall what we do check, and I
intentionally haven't re-looked, but that's also a suggestion for your
commit message :) I.e. what error states are going to be different now,
when don't we proceed because of a disconnect_git() failure?
>> > 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.
Right, some of these functions are quite naughty about that, and as
noted we get it wrong in a lot of existing cases.
But it is unportable (non-standard C, but POSIX plasters over it),
i.e. a return of -1 from main() is interpreted as a wait() status in
POSIX, and results in a return of -1 & 0xff == 255.
But for new code let's try to get it right, which in this case is going
to be transport_disconnect() being being a syscall error code, and the
"exit code" being a return code, which should not be mixed, no?
prev parent reply other threads:[~2021-10-05 12:29 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-28 0:17 [PATCH] Don't ignore transport_disconnect error codes in fetch and clone Mike Hommey
2021-09-28 2:56 ` Ævar Arnfjörð Bjarmason
2021-10-05 4:54 ` Mike Hommey
2021-10-05 12:21 ` Ævar Arnfjörð Bjarmason [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87h7dvlko2.fsf@evledraar.gmail.com \
--to=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=mh@glandium.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).