Re: git fetch origin hanging in 1.7.0
- From
Tay Ray Chuan <rctay89@gmail.com>
- Date
- Feb 16, 2010, 07:18 UTC
- Message-ID
- <20100216151821.994ace31.rctay89@gmail.com>
- In-Reply-To
- <20100216063959.GC2169@coredump.intra.peff.net>
Hi,
On Tue, 16 Feb 2010 01:39:59 -0500 Jeff King <peff@peff.net> wrote:
Show 12 quoted lines
> I can reproduce the bug with: > > $ mkdir foo && (cd foo && git init) > $ git clone foo bar > Initialized empty Git repository in /home/peff/bar/.git/ > warning: You appear to have cloned an empty repository. > $ cd bar && git fetch > > which hangs on a pipe read(). It bisects to 61b075b (Support taking over > transports, 2009-12-09) from Ilari (cc'd). It looks like we get the > empty ref list once in get_remote_heads, and then try to get it again > and hang.
I agree with the diagnosis. With gdb, I find that program execution hangs on the packet_read_line() call in connect.c::get_remote_heads().
Show 14 quoted lines
> diff --git a/transport.c b/transport.c
> index ad25b98..e6f9464 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -1010,7 +1010,8 @@ int transport_push(struct transport *transport,
>
> const struct ref *transport_get_remote_refs(struct transport *transport)
> {
> - if (!transport->remote_refs)
> + struct git_transport_data *data = transport->data;
> + if (!data->got_remote_heads)
> transport->remote_refs = transport->get_refs_list(transport, 0);
>
> return transport->remote_refs;NAK. This will only work if the given transport is git://. (My take below.)
> That fixes the problem for me, but I am totally clueless about this > code. Do all transports have a git_transport_data (if so, then why is it > a void pointer?).
It is void so that it can be any struct - for example, git_transport_data, bundle_transport_data. That way, transport->data can point to any transport-specific data, while keeping the compiler satisfied at compile-time.
-- >8 -- Subject: [PATCH] transport: add got_remote_refs flag
tranport.c::transport_get_remote_refs() used to check transport->remote_refs to determine whether transport->get_refs_list() should be invoked.
However, transport->remote_refs could evaluate to false (eg. if it is NULL), causingo transport->get_refs_list() to be invoked unnecessarily.
Introduce a flag, transport->got_remote_refs, and make tranport.c::transport_get_remote_refs() check this flag rather than evaluating transport->remote_refs.
Signed-off-by: Tay Ray Chuan <rctay89@gmail.com> --- transport.c | 5 ++++- transport.h | 6 ++++++ 2 files changed, 10 insertions(+), 1 deletions(-)
diff --git a/transport.c b/transport.c index 3846aac..08e4fa0 100644 --- a/transport.c +++ b/transport.c @@ -918,6 +918,7 @@ struct transport *transport_get(struct remote *remote, const char *url) if (!remote) die("No remote provided to transport_get()"); + ret->got_remote_refs = 0; ret->remote = remote; helper = remote->foreign_vcs; @@ -1079,8 +1080,10 @@ int transport_push(struct transport *transport, const struct ref *transport_get_remote_refs(struct transport *transport) { - if (!transport->remote_refs) + if (!transport->got_remote_refs) { transport->remote_refs = transport->get_refs_list(transport, 0); + transport->got_remote_refs = 1; + } return transport->remote_refs; } diff --git a/transport.h b/transport.h index 7cea5cc..1cca13b 100644 --- a/transport.h +++ b/transport.h @@ -20,6 +20,12 @@ struct transport { const struct ref *remote_refs; /** + * Indicates whether the remote_refs member has been set. Set by + * transport.c::transport_get_remote_refs(). + */ + unsigned got_remote_refs : 1; + + /** * Returns 0 if successful, positive if the option is not * recognized or is inapplicable, and negative if the option * is applicable but the value is invalid.
-- 1.7.0.1.gcd472.dirty -- Cheers, Ray Chuan