{"thread":{"id":"56594","subject":"[PATCH] Don't ignore transport_disconnect error codes in fetch and clone","startedAt":"2021-09-28T00:45:37Z","lastAt":"2021-10-05T12:29:22Z","messageCount":4,"participants":["Mike Hommey","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"437218","messageId":"20210928001726.2592734-1-mh@glandium.org","threadId":"56594","inReplyTo":null,"subject":"[PATCH] Don't ignore transport_disconnect error codes in fetch and clone","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2021-09-28T00:17:26Z","receivedAt":"2021-09-28T00:45:37Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"When a remote-helper fails in a way that is not directly visible in the\nremote-helper protocol, the helper failure is ignored by git during\nfetch or clone.\n\nFor example, a helper cannot directly report an error during an `import`\ncommand (short of sending `feature done` to the fast-import file\ndescriptor and not sending a `done` later on).\n\nOr if the helper crashes at the wrong moment, git doesn't notice and\nthinks everything went well.\n\nSigned-off-by: Mike Hommey <mh@glandium.org>\n---\n builtin/clone.c | 5 +++--\n builtin/fetch.c | 6 +++---\n 2 files changed, 6 insertions(+), 5 deletions(-)\n\nWhat I'm not sure about is whether a message should be explicitly\nprinted by git itself in those cases.\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 66fe66679c..f26fa027c5 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -1398,7 +1398,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tsubmodule_progress = transport->progress;\n \n \ttransport_unlock_pack(transport);\n-\ttransport_disconnect(transport);\n+\terr = transport_disconnect(transport);\n \n \tif (option_dissociate) {\n \t\tclose_object_store(the_repository->objects);\n@@ -1406,7 +1406,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t}\n \n \tjunk_mode = JUNK_LEAVE_REPO;\n-\terr = checkout(submodule_progress);\n+\tif (!err)\n+\t\terr = checkout(submodule_progress);\n \n \tfree(remote_name);\n \tstrbuf_release(&reflog_msg);\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 25740c13df..66bccf6f50 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1886,7 +1886,7 @@ static int fetch_one(struct remote *remote, int argc, const char **argv,\n {\n \tstruct refspec rs = REFSPEC_INIT_FETCH;\n \tint i;\n-\tint exit_code;\n+\tint exit_code, disconnect_code;\n \tint maybe_prune_tags;\n \tint remote_via_config = remote_is_configured(remote, 0);\n \n@@ -1952,9 +1952,9 @@ static int fetch_one(struct remote *remote, int argc, const char **argv,\n \texit_code = do_fetch(gtransport, &rs);\n \tsigchain_pop(SIGPIPE);\n \trefspec_clear(&rs);\n-\ttransport_disconnect(gtransport);\n+\tdisconnect_code = transport_disconnect(gtransport);\n \tgtransport = NULL;\n-\treturn exit_code;\n+\treturn exit_code || disconnect_code;\n }\n \n int cmd_fetch(int argc, const char **argv, const char *prefix)\n-- \n2.33.0\n\n"},{"id":"437244","messageId":"874ka5z9pz.fsf@evledraar.gmail.com","threadId":"56594","inReplyTo":"20210928001726.2592734-1-mh@glandium.org","subject":"Re: [PATCH] Don't ignore transport_disconnect error codes in fetch and clone","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-28T02:56:37Z","receivedAt":"2021-09-28T02:59:26Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Sep 28 2021, Mike Hommey wrote:\n\n> When a remote-helper fails in a way that is not directly visible in the\n> remote-helper protocol, the helper failure is ignored by git during\n> fetch or clone.\n>\n> For example, a helper cannot directly report an error during an `import`\n> command (short of sending `feature done` to the fast-import file\n> descriptor and not sending a `done` later on).\n>\n> Or if the helper crashes at the wrong moment, git doesn't notice and\n> thinks everything went well.\n>\n> Signed-off-by: Mike Hommey <mh@glandium.org>\n> ---\n>  builtin/clone.c | 5 +++--\n>  builtin/fetch.c | 6 +++---\n>  2 files changed, 6 insertions(+), 5 deletions(-)\n>\n> What I'm not sure about is whether a message should be explicitly\n> printed by git itself in those cases.\n>\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index 66fe66679c..f26fa027c5 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -1398,7 +1398,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n>  \tsubmodule_progress = transport->progress;\n>  \n>  \ttransport_unlock_pack(transport);\n> -\ttransport_disconnect(transport);\n> +\terr = transport_disconnect(transport);\n>  \n>  \tif (option_dissociate) {\n>  \t\tclose_object_store(the_repository->objects);\n> @@ -1406,7 +1406,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n>  \t}\n>  \n>  \tjunk_mode = JUNK_LEAVE_REPO;\n> -\terr = checkout(submodule_progress);\n> +\tif (!err)\n> +\t\terr = checkout(submodule_progress);\n>  \n>  \tfree(remote_name);\n>  \tstrbuf_release(&reflog_msg);\n\nThis seems buggy in some cases, e.g. just because we couldn't close()\nsome final socket we should just not run the checkout at all? Shouldn't\nwe note the disconnect status, but still see if we can do the checkout\netc?\n\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 25740c13df..66bccf6f50 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -1886,7 +1886,7 @@ static int fetch_one(struct remote *remote, int argc, const char **argv,\n>  {\n>  \tstruct refspec rs = REFSPEC_INIT_FETCH;\n>  \tint i;\n> -\tint exit_code;\n> +\tint exit_code, disconnect_code;\n>  \tint maybe_prune_tags;\n>  \tint remote_via_config = remote_is_configured(remote, 0);\n>  \n\n\n> @@ -1952,9 +1952,9 @@ static int fetch_one(struct remote *remote, int argc, const char **argv,\n>  \texit_code = do_fetch(gtransport, &rs);\n>  \tsigchain_pop(SIGPIPE);\n>  \trefspec_clear(&rs);\n> -\ttransport_disconnect(gtransport);\n> +\tdisconnect_code = transport_disconnect(gtransport);\n>  \tgtransport = NULL;\n> -\treturn exit_code;\n> +\treturn exit_code || disconnect_code;\n>  }\n>  \n>  int cmd_fetch(int argc, const char **argv, const char *prefix)\n\nThis seems like it really needs fixes in other areas,\ni.e. disconnect_git() returns 0 unconditionally, but should check at\nleast finish_connect(), no?\n\nAlso once that's done you'll have a logic error here where you're\nconflating exit and error codes, we should not return -1 from main() (we\ndo in a few cases, but it's nasty)>\n"},{"id":"437955","messageId":"20211005045412.pqwsid6gpprxajbw@glandium.org","threadId":"56594","inReplyTo":"874ka5z9pz.fsf@evledraar.gmail.com","subject":"Re: [PATCH] Don't ignore transport_disconnect error codes in fetch and clone","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2021-10-05T04:54:12Z","receivedAt":"2021-10-05T05:15:34Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"Hi,\n\nSorry for the delay, I managed to miss this reply.\n\nOn Tue, Sep 28, 2021 at 04:56:37AM +0200, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Tue, Sep 28 2021, Mike Hommey wrote:\n> \n> > When a remote-helper fails in a way that is not directly visible in the\n> > remote-helper protocol, the helper failure is ignored by git during\n> > fetch or clone.\n> >\n> > For example, a helper cannot directly report an error during an `import`\n> > command (short of sending `feature done` to the fast-import file\n> > descriptor and not sending a `done` later on).\n> >\n> > Or if the helper crashes at the wrong moment, git doesn't notice and\n> > thinks everything went well.\n> >\n> > Signed-off-by: Mike Hommey <mh@glandium.org>\n> > ---\n> >  builtin/clone.c | 5 +++--\n> >  builtin/fetch.c | 6 +++---\n> >  2 files changed, 6 insertions(+), 5 deletions(-)\n> >\n> > What I'm not sure about is whether a message should be explicitly\n> > printed by git itself in those cases.\n> >\n> > diff --git a/builtin/clone.c b/builtin/clone.c\n> > index 66fe66679c..f26fa027c5 100644\n> > --- a/builtin/clone.c\n> > +++ b/builtin/clone.c\n> > @@ -1398,7 +1398,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n> >  \tsubmodule_progress = transport->progress;\n> >  \n> >  \ttransport_unlock_pack(transport);\n> > -\ttransport_disconnect(transport);\n> > +\terr = transport_disconnect(transport);\n> >  \n> >  \tif (option_dissociate) {\n> >  \t\tclose_object_store(the_repository->objects);\n> > @@ -1406,7 +1406,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n> >  \t}\n> >  \n> >  \tjunk_mode = JUNK_LEAVE_REPO;\n> > -\terr = checkout(submodule_progress);\n> > +\tif (!err)\n> > +\t\terr = checkout(submodule_progress);\n> >  \n> >  \tfree(remote_name);\n> >  \tstrbuf_release(&reflog_msg);\n> \n> This seems buggy in some cases, e.g. just because we couldn't close()\n> some final socket we should just not run the checkout at all? Shouldn't\n> we note the disconnect status, but still see if we can do the checkout\n> etc?\n\nI guess it could be seen both ways. In my particular use case, I do want\nautomated testing to be able to catch Leak Sanitizer failures from a\nremote-helper.\n\n> > diff --git a/builtin/fetch.c b/builtin/fetch.c\n> > index 25740c13df..66bccf6f50 100644\n> > --- a/builtin/fetch.c\n> > +++ b/builtin/fetch.c\n> > @@ -1886,7 +1886,7 @@ static int fetch_one(struct remote *remote, int argc, const char **argv,\n> >  {\n> >  \tstruct refspec rs = REFSPEC_INIT_FETCH;\n> >  \tint i;\n> > -\tint exit_code;\n> > +\tint exit_code, disconnect_code;\n> >  \tint maybe_prune_tags;\n> >  \tint remote_via_config = remote_is_configured(remote, 0);\n> >  \n> \n> \n> > @@ -1952,9 +1952,9 @@ static int fetch_one(struct remote *remote, int argc, const char **argv,\n> >  \texit_code = do_fetch(gtransport, &rs);\n> >  \tsigchain_pop(SIGPIPE);\n> >  \trefspec_clear(&rs);\n> > -\ttransport_disconnect(gtransport);\n> > +\tdisconnect_code = transport_disconnect(gtransport);\n> >  \tgtransport = NULL;\n> > -\treturn exit_code;\n> > +\treturn exit_code || disconnect_code;\n> >  }\n> >  \n> >  int cmd_fetch(int argc, const char **argv, const char *prefix)\n> \n> This seems like it really needs fixes in other areas,\n> i.e. disconnect_git() returns 0 unconditionally, but should check at\n> least finish_connect(), no?\n> \n> Also once that's done you'll have a logic error here where you're\n> conflating exit and error codes, we should not return -1 from main() (we\n> do in a few cases, but it's nasty)>\n\ntransport_disconnect returns the remote-helper exit code for external\nhelpers, though, but I guess we don't necessarily need to return that\nparticular exit code.\n\nMike\n"},{"id":"437968","messageId":"87h7dvlko2.fsf@evledraar.gmail.com","threadId":"56594","inReplyTo":"20211005045412.pqwsid6gpprxajbw@glandium.org","subject":"Re: [PATCH] Don't ignore transport_disconnect error codes in fetch and clone","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-05T12:21:49Z","receivedAt":"2021-10-05T12:29:22Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Oct 05 2021, Mike Hommey wrote:\n\n> Hi,\n>\n> Sorry for the delay, I managed to miss this reply.\n>\n> On Tue, Sep 28, 2021 at 04:56:37AM +0200, Ævar Arnfjörð Bjarmason wrote:\n>> \n>> On Tue, Sep 28 2021, Mike Hommey wrote:\n>> \n>> > When a remote-helper fails in a way that is not directly visible in the\n>> > remote-helper protocol, the helper failure is ignored by git during\n>> > fetch or clone.\n>> >\n>> > For example, a helper cannot directly report an error during an `import`\n>> > command (short of sending `feature done` to the fast-import file\n>> > descriptor and not sending a `done` later on).\n>> >\n>> > Or if the helper crashes at the wrong moment, git doesn't notice and\n>> > thinks everything went well.\n>> >\n>> > Signed-off-by: Mike Hommey <mh@glandium.org>\n>> > ---\n>> >  builtin/clone.c | 5 +++--\n>> >  builtin/fetch.c | 6 +++---\n>> >  2 files changed, 6 insertions(+), 5 deletions(-)\n>> >\n>> > What I'm not sure about is whether a message should be explicitly\n>> > printed by git itself in those cases.\n>> >\n>> > diff --git a/builtin/clone.c b/builtin/clone.c\n>> > index 66fe66679c..f26fa027c5 100644\n>> > --- a/builtin/clone.c\n>> > +++ b/builtin/clone.c\n>> > @@ -1398,7 +1398,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n>> >  \tsubmodule_progress = transport->progress;\n>> >  \n>> >  \ttransport_unlock_pack(transport);\n>> > -\ttransport_disconnect(transport);\n>> > +\terr = transport_disconnect(transport);\n>> >  \n>> >  \tif (option_dissociate) {\n>> >  \t\tclose_object_store(the_repository->objects);\n>> > @@ -1406,7 +1406,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n>> >  \t}\n>> >  \n>> >  \tjunk_mode = JUNK_LEAVE_REPO;\n>> > -\terr = checkout(submodule_progress);\n>> > +\tif (!err)\n>> > +\t\terr = checkout(submodule_progress);\n>> >  \n>> >  \tfree(remote_name);\n>> >  \tstrbuf_release(&reflog_msg);\n>> \n>> This seems buggy in some cases, e.g. just because we couldn't close()\n>> some final socket we should just not run the checkout at all? Shouldn't\n>> we note the disconnect status, but still see if we can do the checkout\n>> etc?\n>\n> I guess it could be seen both ways. In my particular use case, I do want\n> automated testing to be able to catch Leak Sanitizer failures from a\n> remote-helper.\n\nI'd like that too, but I'm pointing out something unrelated (or at least\nI think I am), which is that this is changing the equivalent of this code:\n\n    int fd = open(...);\n    close(fd);\n    do_stuff();\n\nTo be:\n\n    int err;\n    int fd = open(...); /* check err here too, but whatever, pseuodocode() */\n    if (!close(fd))\n        do_stuff();\n\nBut just with sockets, i.e. just because transport_disconnect() had\n*some* error are we really confident that checkout() should not be run?\n\n*Maybe*, but at the very least shouldn't we distinguish some of those\nerror states? Isn't any serious protocol error going to be caught\nearlier transport_disconnect() is just going to give us the equivalent\nof not being able to cleanly close the socket?\n\nExcept as noted in my second comment it doesn't, since we ignore\ndisconnect_git(). At this point I can't recall what we do check, and I\nintentionally haven't re-looked, but that's also a suggestion for your\ncommit message :) I.e. what error states are going to be different now,\nwhen don't we proceed because of a disconnect_git() failure?\n\n>> > diff --git a/builtin/fetch.c b/builtin/fetch.c\n>> > index 25740c13df..66bccf6f50 100644\n>> > --- a/builtin/fetch.c\n>> > +++ b/builtin/fetch.c\n>> > @@ -1886,7 +1886,7 @@ static int fetch_one(struct remote *remote, int argc, const char **argv,\n>> >  {\n>> >  \tstruct refspec rs = REFSPEC_INIT_FETCH;\n>> >  \tint i;\n>> > -\tint exit_code;\n>> > +\tint exit_code, disconnect_code;\n>> >  \tint maybe_prune_tags;\n>> >  \tint remote_via_config = remote_is_configured(remote, 0);\n>> >  \n>> \n>> \n>> > @@ -1952,9 +1952,9 @@ static int fetch_one(struct remote *remote, int argc, const char **argv,\n>> >  \texit_code = do_fetch(gtransport, &rs);\n>> >  \tsigchain_pop(SIGPIPE);\n>> >  \trefspec_clear(&rs);\n>> > -\ttransport_disconnect(gtransport);\n>> > +\tdisconnect_code = transport_disconnect(gtransport);\n>> >  \tgtransport = NULL;\n>> > -\treturn exit_code;\n>> > +\treturn exit_code || disconnect_code;\n>> >  }\n>> >  \n>> >  int cmd_fetch(int argc, const char **argv, const char *prefix)\n>> \n>> This seems like it really needs fixes in other areas,\n>> i.e. disconnect_git() returns 0 unconditionally, but should check at\n>> least finish_connect(), no?\n>> \n>> Also once that's done you'll have a logic error here where you're\n>> conflating exit and error codes, we should not return -1 from main() (we\n>> do in a few cases, but it's nasty)>\n>\n> transport_disconnect returns the remote-helper exit code for external\n> helpers, though, but I guess we don't necessarily need to return that\n> particular exit code.\n\nRight, some of these functions are quite naughty about that, and as\nnoted we get it wrong in a lot of existing cases.\n\nBut it is unportable (non-standard C, but POSIX plasters over it),\ni.e. a return of -1 from main() is interpreted as a wait() status in\nPOSIX, and results in a return of -1 & 0xff == 255.\n\nBut for new code let's try to get it right, which in this case is going\nto be transport_disconnect() being being a syscall error code, and the\n\"exit code\" being a return code, which should not be mixed, no?\n"}]}