Re: git fetch origin hanging in 1.7.0

[Tay Ray Chuan <rctay89@gmail.com>]

Hi,

On Tue, 16 Feb 2010 01:39:59 -0500
Jeff King <peff@peff.net> wrote:
> 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().

> 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