git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jeff King <peff@peff.net>
To: Greg Pflaum <greg.pflaum@pnp-hcl.com>
Cc: Taylor Blau <me@ttaylorr.com>, git@vger.kernel.org
Subject: Re: [BUG] clone of large repo fails when server closes idle SSH session during "Resolving deltas"
Date: Wed, 19 May 2021 05:57:06 -0400	[thread overview]
Message-ID: <YKTg8nYjSGpKbq8W@coredump.intra.peff.net> (raw)
In-Reply-To: <YKTbsByUaPEuDNtR@coredump.intra.peff.net>

On Wed, May 19, 2021 at 05:34:40AM -0400, Jeff King wrote:

> Another side issue is that once the protocol conversation has finished,
> I'm not sure if it's really useful for us to detect and complain about
> ssh's exit code. We know the other side completed the conversation
> successfully, and we have nothing left to ask it. So a fix for your
> immediate pain would be to stop noticing that. I think the root issue is
> still worth addressing, though; we are tying up network and local
> resources with a useless to-be-closed ssh connection.

By the way, there's an interesting subtlety / bug related to this. While
"git clone" does return a failed exit code in this case, it leaves the
repository directory in place! And because no real error occurred with
the clone, you can use it as usual (though I think if it's a non-bare
clone, you'd need to run "git checkout" fill in the working tree).

Propagating the error code comes from aab179d937 (builtin/clone.c: don't
ignore transport_fetch_refs() errors, 2020-12-03). So prior to Git
v2.30.0, your case would kind-of work.

But:

  - I think that is just "clone"; a "git fetch" would always have
    propagated the error from transport_fetch_refs()

  - that commit was right to start propagating the error code from
    transport_fetch_refs(). While in this specific case, we happened to
    produce a useful repository directory, most other errors would not.

  - there _is_ a bug in aab179d937, though. When it sees the error it
    should clean up the repo directory. And that even happens
    automatically via an atexit() handler. But because rather than
    calling die() it jumps to cleanup code, it mistakenly sets the flag
    for "leave the directory in place".

  - any logic to ignore errors would have to be inside the transport
    code (i.e., it realizes that ssh exiting non-zero isn't a big deal
    anymore, and then transport_fetch_refs() still returns success).

-Peff

  reply	other threads:[~2021-05-19  9:57 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-19  3:46 Greg Pflaum
2021-05-19  9:34 ` Jeff King
2021-05-19  9:57   ` Jeff King [this message]
2021-05-19 16:11   ` [PATCH] fetch-pack: signal v2 server that we are done making requests Jeff King

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

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YKTg8nYjSGpKbq8W@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=greg.pflaum@pnp-hcl.com \
    --cc=me@ttaylorr.com \
    --subject='Re: [BUG] clone of large repo fails when server closes idle SSH session during "Resolving deltas"' \
    /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

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 NNTP newsgroup(s).