clong an empty repo over ssh causes (harmless) fatal

clong an empty repo over ssh causes (harmless) fatal

[Sitaram Chamarty]

Hello,

Cloning an empty repo seems to produce a spurious error.
The repo still seems fine though.

Any thoughts?

    $ git clone ssh://sitaram@localhost/home/sitaram/t/a b
    Initialized empty Git repository in /home/sitaram/t/b/.git/
    sitaram@localhost's password:
    warning: You appear to have cloned an empty repository.
    fatal: The remote end hung up unexpectedly

Thanks,

Sitaram

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

[Matthieu Moy]

Sitaram Chamarty <sitaramc@gmail.com> writes:

> Hello,
>
> Cloning an empty repo seems to produce a spurious error.
> The repo still seems fine though.

Can't reproduce here:

$ git clone ssh://.../tmp/empty cloned
Initialized empty Git repository in /tmp/cloned/.git/
warning: You appear to have cloned an empty repository.
$ git --version
git version 1.6.4.187.gd399.dirty

Maybe you have an older version of Git?

-- 
Matthieu

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

[Sitaram Chamarty]

[apologies if you get this twice]

On Mon, Aug 31, 2009 at 4:52 PM, Matthieu Moy <Matthieu.Moy@imag.fr> wrote:
>
> Sitaram Chamarty <sitaramc@gmail.com> writes:
>
> > Hello,
> >
> > Cloning an empty repo seems to produce a spurious error.
> > The repo still seems fine though.
>
> Can't reproduce here:
>
> $ git clone ssh://.../tmp/empty cloned
> Initialized empty Git repository in /tmp/cloned/.git/
> warning: You appear to have cloned an empty repository.
> $ git --version
> git version 1.6.4.187.gd399.dirty
>
> 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?

To start with, this is Mandriva 2009.1, kernel 2.6.29, git built from
scratch locally (not an RPM or something).  Should I try some other
things like strace?  It's too minor to bother, if that's all it is,
assuming it's not a symptom of something larger :)


>
> --
> Matthieu

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

[Matthieu Moy]

Sitaram Chamarty <sitaramc@gmail.com> writes:

> To start with, this is Mandriva 2009.1, kernel 2.6.29, git built from
> scratch locally (not an RPM or something).  Should I try some other
> things like strace?

Setting $GIT_TRACE to 2 can help, too. The problem is that the git
instance you want to debug is the remote one, I don't know if there's
a better way to do that than putting a wrapper script for
git-upload-pack ...

-- 
Matthieu

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

[Jeff King]

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

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

-Peff

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

[Sverre Rabbelier]

Heya,

On Mon, Aug 31, 2009 at 18:41, Jeff King<peff@peff.net> wrote:
> IIRC, the message you are seeing comes when the _server_ is an older
> version of git. It is harmless, though.

Mhhhh, is it some weird interaction between 'empty repository' patch
and old server versions, or did this happen too before my patch was
applied?

-- 
Cheers,

Sverre Rabbelier

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

[Matthieu Moy]

Jeff King <peff@peff.net> writes:

> On Mon, Aug 31, 2009 at 08:00:41PM +0530, Sitaram Chamarty wrote:
>
>> > 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

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

[Jeff King]

On Mon, Aug 31, 2009 at 07:12:44PM +0200, Sverre Rabbelier wrote:

> On Mon, Aug 31, 2009 at 18:41, Jeff King<peff@peff.net> wrote:
> > IIRC, the message you are seeing comes when the _server_ is an older
> > version of git. It is harmless, though.
> 
> Mhhhh, is it some weird interaction between 'empty repository' patch
> and old server versions, or did this happen too before my patch was
> applied?

I think the former. I thought it was discussed before, but the only
reference I can find is this (see the end of the email):

  http://article.gmane.org/gmane.comp.version-control.git/107626

and I don't see any followup for that specific part of the mail.

-Peff

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

[Sverre Rabbelier]

Heya,

On Mon, Aug 31, 2009 at 21:08, Jeff King<peff@peff.net> wrote:
> I think the former. I thought it was discussed before, but the only
> reference I can find is this (see the end of the email):
>
>  http://article.gmane.org/gmane.comp.version-control.git/107626

Ah, yeup, I see.

> and I don't see any followup for that specific part of the mail.

I don't remember any follow up to that either, shame on me :(.

-- 
Cheers,

Sverre Rabbelier

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

[Jeff King]

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

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

[Björn Steinbrink]

On 2009.08.31 15:10:32 -0400, Jeff King wrote:
> 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.

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

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

[Jeff King]

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

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

[Sverre Rabbelier]

Heya,

2009/9/1 Jeff King <peff@peff.net>:
> 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

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

[Jeff King]

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

> 2009/9/1 Jeff King <peff@peff.net>:
> > 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);
 	}

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

[Daniel Barkalow]

On Mon, 31 Aug 2009, Jeff King wrote:

> On Tue, Sep 01, 2009 at 12:50:25AM +0200, Sverre Rabbelier wrote:
> 
> > 2009/9/1 Jeff King <peff@peff.net>:
> > > 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*

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

[Jeff King]

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

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

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

[Sitaram Chamarty]

2009/9/1 Jeff King <peff@peff.net>:

> 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

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

[Daniel Barkalow]

On Wed, 2 Sep 2009, Jeff King wrote:

> On Wed, Sep 02, 2009 at 12:33:52AM -0400, Daniel Barkalow wrote:
> 
> > > 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*

[PATCH] clone: disconnect transport after fetching

[Jeff King]

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 <peff@peff.net>
---
This was suggested by Daniel, so theoretically

  Acked-by: Daniel Barkalow <barkalow@iabervon.org>

:)

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

Re: [PATCH] clone: disconnect transport after fetching

[Sverre Rabbelier]

Heya,

On Wed, Sep 2, 2009 at 08:36, Jeff King<peff@peff.net> 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

Re: [PATCH] clone: disconnect transport after fetching

[Jeff King]

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

> On Wed, Sep 2, 2009 at 08:36, Jeff King<peff@peff.net> 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 :).

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

Re: [PATCH] clone: disconnect transport after fetching

[Sverre Rabbelier]

Heya,

On Wed, Sep 2, 2009 at 09:26, Jeff King<peff@peff.net> 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

Re: [PATCH] clone: disconnect transport after fetching

[Daniel Barkalow]

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 <peff@peff.net>
> ---
> This was suggested by Daniel, so theoretically
> 
>   Acked-by: Daniel Barkalow <barkalow@iabervon.org>
> 
> :)

This is what I intended, so:

Acked-by: Daniel Barkalow <barkalow@iabervon.org>

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

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

Re: [PATCH] clone: disconnect transport after fetching

[Junio C Hamano]

Daniel Barkalow <barkalow@iabervon.org> writes:

> On Wed, 2 Sep 2009, Jeff King wrote:
> ...
>> Signed-off-by: Jeff King <peff@peff.net>
>> ---
>> This was suggested by Daniel, so theoretically
>> 
>>   Acked-by: Daniel Barkalow <barkalow@iabervon.org>
>> 
>> :)
>
> This is what I intended, so:
>
> Acked-by: Daniel Barkalow <barkalow@iabervon.org>

Thanks, both.

Re: [PATCH] clone: disconnect transport after fetching

[Jeff King]

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