git fetch origin hanging in 1.7.0

git fetch origin hanging in 1.7.0

[Kevin Menard]

Hi,

I've run into an issue that doing a "git fetch origin" is hanging for
me in git 1.7.0.  The setup here may be wrong, so I don't want to call
it a bug.  But basically I have an empty repo created on the server
that a client then clones.  There's a job that fetches new updates to
the server periodically.  If nothing has been pushed to the server
yet, the client fetch hangs.

This worked fine in 1.6.6, so I'll just roll back my version for now.
But any help on how to do this different would be much appreciated.

-- 
Thanks,
Kevin

Re: git fetch origin hanging in 1.7.0

[Jeff King]

On Mon, Feb 15, 2010 at 07:08:25PM -0500, Kevin Menard wrote:

> I've run into an issue that doing a "git fetch origin" is hanging for
> me in git 1.7.0.  The setup here may be wrong, so I don't want to call
> it a bug.  But basically I have an empty repo created on the server
> that a client then clones.  There's a job that fetches new updates to
> the server periodically.  If nothing has been pushed to the server
> yet, the client fetch hangs.
> 
> This worked fine in 1.6.6, so I'll just roll back my version for now.
> But any help on how to do this different would be much appreciated.

No, what you're doing should work OK (though it does actually produce
ugly "the remote end hung up unexpectedly" messages in v1.6.6).

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. Maybe we need this?

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;

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?).

-Peff

Re: git fetch origin hanging in 1.7.0

[Tay Ray Chuan]

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

Re: git fetch origin hanging in 1.7.0

[Jeff King]

On Tue, Feb 16, 2010 at 03:18:21PM +0800, Tay Ray Chuan wrote:

> > 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.

OK, I figured it was something like that. In that case, your fix looks
like the right thing to do (and it fixes my test case).

Thanks.

> -- >8 --
> Subject: [PATCH] transport: add got_remote_refs flag

Acked-by: Jeff King <peff@peff.net>

-Peff

Re: git fetch origin hanging in 1.7.0

[Tay Ray Chuan]

Hi,

On Tue, Feb 16, 2010 at 3:24 PM, Jeff King <peff@peff.net> wrote:
> OK, I figured it was something like that. In that case, your fix looks
> like the right thing to do (and it fixes my test case).
>
> Thanks.

No problem.

Kevin, I didn't add a Reported-by line, as I don't regard my patch as
actually fixing the issue at hand (hanging read) - it just side-steps
the issue. Hope you don't mind this.

-- 
Cheers,
Ray Chuan

Re: git fetch origin hanging in 1.7.0

[Kevin Menard]

On Tue, Feb 16, 2010 at 2:32 AM, Tay Ray Chuan <rctay89@gmail.com> wrote:

> Kevin, I didn't add a Reported-by line, as I don't regard my patch as
> actually fixing the issue at hand (hanging read) - it just side-steps
> the issue. Hope you don't mind this.


No worries.  I'm just grateful someone more knowledgeable than me
could take a look, confirm it was an issue, and supply a patch.
Thanks to others that chimed in as well.

-- 
Kevin

Re: git fetch origin hanging in 1.7.0

[Ilari Liusvaara]

On Tue, Feb 16, 2010 at 03:18:21PM +0800, Tay Ray Chuan wrote:
> NAK. This will only work if the given transport is git://. (My take
> below.)
> 
> 
> -- >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.

<snip patch>

Seems to work even in external transport case (not suprising, as at this
level, there's no difference between git://, ssh://, file:// and external
transports).

-Ilari