Re: clong an empty repo over ssh causes (harmless) fatal

Jeff King [off-list ref] writes:

On Mon, Aug 31, 2009 at 08:00:41PM +0530, Sitaram Chamarty wrote:

quoted

quoted

Maybe you have an older version of Git?

Had 1.6.4, just tried with 1.6.4.2 -- the error is still there, exactly so.

Anything I can do to provide more info?

IIRC, the message you are seeing comes when the _server_ is an older
version of git. It is harmless, though.

Since the client and server are the same machine:

    $ git clone ssh://sitaram@localhost/home/sitaram/t/a b

I'd bet Sitaram has two installations of git, and plain ssh to the
machine points to the old one (like a $PATH set in ~/.login and not
~/.profile or something like that).

-- 
Matthieu

On Mon, Aug 31, 2009 at 07:25:22PM +0200, Matthieu Moy wrote:

Since the client and server are the same machine:

    $ git clone ssh://sitaram@localhost/home/sitaram/t/a b

I'd bet Sitaram has two installations of git, and plain ssh to the
machine points to the old one (like a $PATH set in ~/.login and not
~/.profile or something like that).

Oh, indeed. I didn't notice that his host was @localhost. :)

But yes, that would be my guess, as well. Trying "ssh sitaram@localhost
git version" would be a good clue.

-Peff

On 2009.08.31 15:10:32 -0400, Jeff King wrote:

On Mon, Aug 31, 2009 at 07:25:22PM +0200, Matthieu Moy wrote:

quoted

Since the client and server are the same machine:

    $ git clone ssh://sitaram@localhost/home/sitaram/t/a b

I'd bet Sitaram has two installations of git, and plain ssh to the
machine points to the old one (like a $PATH set in ~/.login and not
~/.profile or something like that).

Oh, indeed. I didn't notice that his host was @localhost. :)

But yes, that would be my guess, as well. Trying "ssh sitaram@localhost
git version" would be a good clue.

I see the problem here, too.

doener@atjola:~ $ (mkdir a; cd a; git init)
Initialized empty Git repository in /home/doener/a/.git/

doener@atjola:~ $ git clone localhost:a b
Initialized empty Git repository in /home/doener/b/.git/
warning: You appear to have cloned an empty repository.
fatal: The remote end hung up unexpectedly

doener@atjola:~ $ ssh localhost git --version
git version 1.6.4.2.236.gf324c

Björn

On Mon, Aug 31, 2009 at 10:19:11PM +0200, Björn Steinbrink wrote:

I see the problem here, too.

doener@atjola:~ $ (mkdir a; cd a; git init)
Initialized empty Git repository in /home/doener/a/.git/

doener@atjola:~ $ git clone localhost:a b
Initialized empty Git repository in /home/doener/b/.git/
warning: You appear to have cloned an empty repository.
fatal: The remote end hung up unexpectedly

doener@atjola:~ $ ssh localhost git --version
git version 1.6.4.2.236.gf324c

OK, it is definitely not about mixed versions, and it is definitely
reproducible, even without ssh. The local clone optimization manages to
avoid it, but you can see it with:

  git clone file://$PWD/a b

It also happens with git://, except that it is the _remote_ side
producing the message, so git-daemon gets "the remote end hung up
unexpectedly" on its stderr channel.

AFAICT, this problem goes back to v1.6.2, the first version which
handled empty clones. So I blame Sverre. ;)

-Peff

Heya,

2009/9/1 Jeff King [off-list ref]:

AFAICT, this problem goes back to v1.6.2, the first version which
handled empty clones. So I blame Sverre. ;)

Eep :(. Any idea what is going on?

-- 
Cheers,

Sverre Rabbelier

On Tue, Sep 01, 2009 at 12:50:25AM +0200, Sverre Rabbelier wrote:

2009/9/1 Jeff King [off-list ref]:

quoted

AFAICT, this problem goes back to v1.6.2, the first version which
handled empty clones. So I blame Sverre. ;)

Eep :(. Any idea what is going on?

Yeah. We call upload-pack on the remote side, realize there are no refs,
and then we just stop talking. Meanwhile upload-pack is waiting for a
packet to say "these are the refs that I want". So the client really
needs to send an extra packet saying "list of refs is finished".

The patch below seems to work for me, but I'm a little concerned how it
might impact other transports. It actually calls the transport's
fetch method when we have no refs that we want. So each transport must
recognize that we want zero refs and do the appropriate thing. In this
case, for the git protocol, we want to:

  - do a packet_flush to signal "no more refs" to the remote side

  - be aware that we might have zero refs and avoid establishing a new
    connection in that case

Other transports might need to be tweaked similarly, but I don't have
time to test at the moment.

diff --git a/builtin-clone.c b/builtin-clone.c
index 0d2b4a8..f198c01 100644
--- a/builtin-clone.c
+++ b/builtin-clone.c

@@ -515,8 +515,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 					     option_upload_pack);
 
 		refs = transport_get_remote_refs(transport);
-		if(refs)
-			transport_fetch_refs(transport, refs);
+		transport_fetch_refs(transport, refs);
 	}
 
 	if (refs) {

diff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c
index 629735f..04a3776 100644
--- a/builtin-fetch-pack.c
+++ b/builtin-fetch-pack.c

@@ -803,6 +803,8 @@ struct ref *fetch_pack(struct fetch_pack_args *my_args,
 		nr_heads = remove_duplicates(nr_heads, heads);
 	if (!ref) {
 		packet_flush(fd[1]);
+		if (!nr_heads)
+			return NULL;
 		die("no matching remote head");
 	}
 	ref_cpy = do_fetch_pack(fd, ref, nr_heads, heads, pack_lockfile);

diff --git a/transport.c b/transport.c
index f2bd998..25e8946 100644
--- a/transport.c
+++ b/transport.c

@@ -512,6 +512,8 @@ static int fetch_refs_via_pack(struct transport *transport,
 		origh[i] = heads[i] = xstrdup(to_fetch[i]->name);
 
 	if (!data->conn) {
+		if (!nr_heads)
+			return 0;
 		connect_setup(transport, 0, 0);
 		get_remote_heads(data->fd[0], &refs_tmp, 0, NULL, 0, NULL);
 	}

On Mon, 31 Aug 2009, Jeff King wrote:

On Tue, Sep 01, 2009 at 12:50:25AM +0200, Sverre Rabbelier wrote:

quoted

2009/9/1 Jeff King [off-list ref]:

quoted

AFAICT, this problem goes back to v1.6.2, the first version which
handled empty clones. So I blame Sverre. ;)

Eep :(. Any idea what is going on?

Yeah. We call upload-pack on the remote side, realize there are no refs,
and then we just stop talking. Meanwhile upload-pack is waiting for a
packet to say "these are the refs that I want". So the client really
needs to send an extra packet saying "list of refs is finished".

The patch below seems to work for me, but I'm a little concerned how it
might impact other transports.

Does putting a "transport_disconnect(transport);" after the 
"transport_unlock_pack(transport);" in builtin-clone.c also work for you? 
I think that's a cleaner solution, and should future-proof it in case we 
have a future transport that both doesn't disconnect itself after a fetch 
and gives an error message if the connection is dropped suddenly.

It's kind of just an accident that the only transport that cares about 
disconnect very much doesn't care if you've fetched after getting the 
refs.

	-Daniel
*This .sig left intentionally blank*

On Wed, Sep 02, 2009 at 12:33:52AM -0400, Daniel Barkalow wrote:

quoted

The patch below seems to work for me, but I'm a little concerned how it
might impact other transports.

Does putting a "transport_disconnect(transport);" after the 
"transport_unlock_pack(transport);" in builtin-clone.c also work for you? 
I think that's a cleaner solution, and should future-proof it in case we 
have a future transport that both doesn't disconnect itself after a fetch 
and gives an error message if the connection is dropped suddenly.

It's kind of just an accident that the only transport that cares about 
disconnect very much doesn't care if you've fetched after getting the 
refs.

It does work, and I think that is a much saner solution for the reasons
you mention. Thanks. Do you want to write it up and submit it, or should
I?

-Peff

2009/9/1 Jeff King [off-list ref]:

OK, it is definitely not about mixed versions, and it is definitely
reproducible, even without ssh. The local clone optimization manages to
avoid it, but you can see it with:

 git clone file://$PWD/a b

ok I hadn't noticed that -- good spot!

Anyway this whole thread went way over my head very quickly so just
wanted to say I appreciate you guys looking at it and fixing it.

Don't know if enough people say that :)

Sitaram

On Wed, 2 Sep 2009, Jeff King wrote:

On Wed, Sep 02, 2009 at 12:33:52AM -0400, Daniel Barkalow wrote:

quoted

quoted

The patch below seems to work for me, but I'm a little concerned how it
might impact other transports.

Does putting a "transport_disconnect(transport);" after the 
"transport_unlock_pack(transport);" in builtin-clone.c also work for you? 
I think that's a cleaner solution, and should future-proof it in case we 
have a future transport that both doesn't disconnect itself after a fetch 
and gives an error message if the connection is dropped suddenly.

It's kind of just an accident that the only transport that cares about 
disconnect very much doesn't care if you've fetched after getting the 
refs.

It does work, and I think that is a much saner solution for the reasons
you mention. Thanks. Do you want to write it up and submit it, or should
I?

You probably should; I'm not sure when I'd get to putting together a 
patch, and you did the hard part (figuring out what was going on) anyway.

	-Daniel
*This .sig left intentionally blank*

The current code just leaves the transport in whatever state
it was in after performing the fetch.  For a non-empty clone
over the git protocol, the transport code already
disconnects at the end of the fetch.

But for an empty clone, we leave the connection hanging, and
eventually close the socket when clone exits. This causes
the remote upload-pack to complain "the remote end hung up
unexpectedly". While this message is harmless to the clone
itself, it is unnecessarily scary for a user to see and may
pollute git-daemon logs.

This patch just explicitly calls disconnect after we are
done with the remote end, which sends a flush packet to
upload-pack and cleanly disconnects, avoiding the error
message.

Other transports are unaffected or slightly improved:

 - for a non-empty repo over the git protocol, the second
   disconnect is a no-op (since we are no longer connected)

 - for "walker" transports (like HTTP or FTP), we actually
   free some used memory (which previously just sat until
   the clone process exits)

 - for "rsync", disconnect is always a no-op anyway

Signed-off-by: Jeff King <redacted>
---
This was suggested by Daniel, so theoretically

  Acked-by: Daniel Barkalow [off-list ref]

:)

As you can see from the commit message, I did a little extra hunting to
make sure we are not going to impact any other code paths, and I am
pretty sure we are fine.

 builtin-clone.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/builtin-clone.c b/builtin-clone.c
index 991a7ae..0f231d8 100644
--- a/builtin-clone.c
+++ b/builtin-clone.c

@@ -580,8 +580,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		option_no_checkout = 1;
 	}
 
-	if (transport)
+	if (transport) {
 		transport_unlock_pack(transport);
+		transport_disconnect(transport);
+	}
 
 	if (!option_no_checkout) {
 		struct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));

-- 
1.6.4.2.401.ga275f.dirty

Heya,

On Wed, Sep 2, 2009 at 08:36, Jeff King[off-list ref] wrote:

As you can see from the commit message, I did a little extra hunting to
make sure we are not going to impact any other code paths, and I am
pretty sure we are fine.

Thank you for fixing my mistake :).

-- 
Cheers,

Sverre Rabbelier

On Wed, Sep 02, 2009 at 09:09:19AM +0200, Sverre Rabbelier wrote:

On Wed, Sep 2, 2009 at 08:36, Jeff King[off-list ref] wrote:

quoted

As you can see from the commit message, I did a little extra hunting to
make sure we are not going to impact any other code paths, and I am
pretty sure we are fine.

Thank you for fixing my mistake :).

You're welcome, though I am not sure it is your mistake. Arguably this
is something we should have been doing all along. The point of
abstracting transports was that we didn't need to know their details at
the outer layer, but in this case we were relying on the fact that no
transports (until empty-clone-over-git) needed an explicit
transport_disconnect to cleanly hang up on the other end.

So think of it as you exposing a long-standing bug. ;)

-Peff

Heya,

On Wed, Sep 2, 2009 at 09:26, Jeff King[off-list ref] wrote:

So think of it as you exposing a long-standing bug. ;)

Ah, well in that case, you're all welcome :P.


-- 
Cheers,

Sverre Rabbelier

On Wed, 2 Sep 2009, Jeff King wrote:

The current code just leaves the transport in whatever state
it was in after performing the fetch.  For a non-empty clone
over the git protocol, the transport code already
disconnects at the end of the fetch.

But for an empty clone, we leave the connection hanging, and
eventually close the socket when clone exits. This causes
the remote upload-pack to complain "the remote end hung up
unexpectedly". While this message is harmless to the clone
itself, it is unnecessarily scary for a user to see and may
pollute git-daemon logs.

This patch just explicitly calls disconnect after we are
done with the remote end, which sends a flush packet to
upload-pack and cleanly disconnects, avoiding the error
message.

Other transports are unaffected or slightly improved:

 - for a non-empty repo over the git protocol, the second
   disconnect is a no-op (since we are no longer connected)

 - for "walker" transports (like HTTP or FTP), we actually
   free some used memory (which previously just sat until
   the clone process exits)

 - for "rsync", disconnect is always a no-op anyway

Signed-off-by: Jeff King <redacted>
---
This was suggested by Daniel, so theoretically

  Acked-by: Daniel Barkalow [off-list ref]

:)

This is what I intended, so:

Acked-by: Daniel Barkalow <redacted>

As you can see from the commit message, I did a little extra hunting to
make sure we are not going to impact any other code paths, and I am
pretty sure we are fine.

Also, builtin-fetch already does the explicit disconnect, and commonly 
exercises both the "we want something" and "we don't want anything" cases, 
so any problems would have to be surprisingly clone-specific.

On Wed, Sep 02, 2009 at 02:36:47AM -0400, Jeff King wrote:

This patch just explicitly calls disconnect after we are
done with the remote end, which sends a flush packet to
upload-pack and cleanly disconnects, avoiding the error
message.

I see you applied this with some extra tests. I should have mentioned in
the original cover letter that I considered tests but intentionally did
not include them.

The problem is that clone forks upload-pack, and then hangs up on it by
exiting, and then upload-pack spews the unwanted message. But control
has returned to the shell after clone exits, meaning that the message
from upload-pack may or may not have gotten there by the time we grep
stderr.

So I don't think your test will ever incorrectly show a failure, but I
believe that it would pass randomly even without the related fix to the
code.

-Peff