clong an empty repo over ssh causes (harmless) fatal

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

Sitaram Chamarty <sitaramc@gmail.com> writes:

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?

[apologies if you get this twice]

On Mon, Aug 31, 2009 at 4:52 PM, Matthieu Moy <Matthieu.Moy@imag.fr> wrote:

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

Sitaram Chamarty <sitaramc@gmail.com> writes:

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

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

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

-Peff

Heya,

On Mon, Aug 31, 2009 at 18:41, Jeff King<peff@peff.net> wrote:

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

On Mon, Aug 31, 2009 at 07:12:44PM +0200, Sverre Rabbelier 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

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

-Peff

Heya,

On Mon, Aug 31, 2009 at 21:08, Jeff King<peff@peff.net> wrote:

Ah, yeup, I see.

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

Jeff King <peff@peff.net> writes:

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

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

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:

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:

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 <peff@peff.net>:

Eep :(. Any idea what is going on?

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

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.

On Mon, 31 Aug 2009, Jeff King wrote:

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:

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

On Wed, 2 Sep 2009, Jeff King wrote:

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

Heya,

On Wed, Sep 2, 2009 at 08:36, Jeff King<peff@peff.net> wrote:

Thank you for fixing my mistake :).

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

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<peff@peff.net> wrote:

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

On Wed, 2 Sep 2009, Jeff King wrote:

This is what I intended, so:

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

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.

Daniel Barkalow <barkalow@iabervon.org> writes:

Thanks, both.

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

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

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

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