# clong an empty repo over ssh causes (harmless) fatal 25 messages from 2009-08-31 to 2009-09-04. Participants: Sitaram Chamarty, Matthieu Moy, Jeff King, Sverre Rabbelier, Björn Steinbrink, Daniel Barkalow, Junio C Hamano. Thread: https://gitlist.dev/t/20798 ## Sitaram Chamarty, 2009-08-31 11:14 Subject: clong an empty repo over ssh causes (harmless) fatal Message-ID: URL: https://gitlist.dev/e/slrnh9nc4e.6bn.sitaramc%40sitaramc.homelinux.net ``` 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 ``` ## Matthieu Moy, 2009-08-31 11:22 Subject: Re: clong an empty repo over ssh causes (harmless) fatal Message-ID: URL: https://gitlist.dev/e/vpqskf8z0rj.fsf%40bauges.imag.fr In-Reply-To: ``` Sitaram Chamarty 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 ``` ## Sitaram Chamarty, 2009-08-31 14:30 Subject: Re: clong an empty repo over ssh causes (harmless) fatal Message-ID: <2e24e5b90908310730u53ee27d3nd2b66c1f58202e7@mail.gmail.com> URL: https://gitlist.dev/e/2e24e5b90908310730u53ee27d3nd2b66c1f58202e7%40mail.gmail.com In-Reply-To: ``` [apologies if you get this twice] On Mon, Aug 31, 2009 at 4:52 PM, Matthieu Moy wrote: > > Sitaram Chamarty 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 ``` ## Matthieu Moy, 2009-08-31 14:47 Subject: Re: clong an empty repo over ssh causes (harmless) fatal Message-ID: URL: https://gitlist.dev/e/vpq4oroqbvc.fsf%40bauges.imag.fr In-Reply-To: <2e24e5b90908310730u53ee27d3nd2b66c1f58202e7@mail.gmail.com> ``` Sitaram Chamarty 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 ``` ## Jeff King, 2009-08-31 16:41 Subject: Re: clong an empty repo over ssh causes (harmless) fatal Message-ID: <20090831164146.GA23245@coredump.intra.peff.net> URL: https://gitlist.dev/e/20090831164146.GA23245%40coredump.intra.peff.net In-Reply-To: <2e24e5b90908310730u53ee27d3nd2b66c1f58202e7@mail.gmail.com> ``` 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 ``` ## Sverre Rabbelier, 2009-08-31 17:12 Subject: Re: clong an empty repo over ssh causes (harmless) fatal Message-ID: URL: https://gitlist.dev/e/fabb9a1e0908311012q4cea2d51i2c2f0cbceac0cab%40mail.gmail.com In-Reply-To: <20090831164146.GA23245@coredump.intra.peff.net> ``` Heya, On Mon, Aug 31, 2009 at 18:41, Jeff King 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 ``` ## Matthieu Moy, 2009-08-31 17:25 Subject: Re: clong an empty repo over ssh causes (harmless) fatal Message-ID: URL: https://gitlist.dev/e/vpqocpvevzx.fsf%40bauges.imag.fr In-Reply-To: <20090831164146.GA23245@coredump.intra.peff.net> ``` Jeff King 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 ``` ## Jeff King, 2009-08-31 19:08 Subject: Re: clong an empty repo over ssh causes (harmless) fatal Message-ID: <20090831190827.GA4876@sigill.intra.peff.net> URL: https://gitlist.dev/e/20090831190827.GA4876%40sigill.intra.peff.net In-Reply-To: ``` On Mon, Aug 31, 2009 at 07:12:44PM +0200, Sverre Rabbelier wrote: > On Mon, Aug 31, 2009 at 18:41, Jeff King 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 ``` ## Sverre Rabbelier, 2009-08-31 19:09 Subject: Re: clong an empty repo over ssh causes (harmless) fatal Message-ID: URL: https://gitlist.dev/e/fabb9a1e0908311209y4c83b60eg75ced6d3db615dff%40mail.gmail.com In-Reply-To: <20090831190827.GA4876@sigill.intra.peff.net> ``` Heya, On Mon, Aug 31, 2009 at 21:08, Jeff King 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 ``` ## Jeff King, 2009-08-31 19:10 Subject: Re: clong an empty repo over ssh causes (harmless) fatal Message-ID: <20090831191032.GB4876@sigill.intra.peff.net> URL: https://gitlist.dev/e/20090831191032.GB4876%40sigill.intra.peff.net In-Reply-To: ``` 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 ``` ## Björn Steinbrink, 2009-08-31 20:19 Subject: Re: clong an empty repo over ssh causes (harmless) fatal Message-ID: <20090831201911.GA24989@atjola.homenet> URL: https://gitlist.dev/e/20090831201911.GA24989%40atjola.homenet In-Reply-To: <20090831191032.GB4876@sigill.intra.peff.net> ``` 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 ``` ## Jeff King, 2009-08-31 22:47 Subject: Re: clong an empty repo over ssh causes (harmless) fatal Message-ID: <20090831224749.GA24190@sigill.intra.peff.net> URL: https://gitlist.dev/e/20090831224749.GA24190%40sigill.intra.peff.net In-Reply-To: <20090831201911.GA24989@atjola.homenet> ``` 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 ``` ## Sverre Rabbelier, 2009-08-31 22:50 Subject: Re: clong an empty repo over ssh causes (harmless) fatal Message-ID: URL: https://gitlist.dev/e/fabb9a1e0908311550r1b549eb2k2df65c188a0ea6a0%40mail.gmail.com In-Reply-To: <20090831224749.GA24190@sigill.intra.peff.net> ``` Heya, 2009/9/1 Jeff King : > 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 ``` ## Jeff King, 2009-09-01 01:08 Subject: Re: clong an empty repo over ssh causes (harmless) fatal Message-ID: <20090901010833.GA4033@sigill.intra.peff.net> URL: https://gitlist.dev/e/20090901010833.GA4033%40sigill.intra.peff.net In-Reply-To: ``` On Tue, Sep 01, 2009 at 12:50:25AM +0200, Sverre Rabbelier wrote: > 2009/9/1 Jeff King : > > 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); } ``` ## Daniel Barkalow, 2009-09-02 04:33 Subject: Re: clong an empty repo over ssh causes (harmless) fatal Message-ID: URL: https://gitlist.dev/e/alpine.LNX.2.00.0909020024270.28290%40iabervon.org In-Reply-To: <20090901010833.GA4033@sigill.intra.peff.net> ``` 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 : > > > 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* ``` ## Jeff King, 2009-09-02 05:16 Subject: Re: clong an empty repo over ssh causes (harmless) fatal Message-ID: <20090902051615.GC12046@coredump.intra.peff.net> URL: https://gitlist.dev/e/20090902051615.GC12046%40coredump.intra.peff.net In-Reply-To: ``` 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 ``` ## Sitaram Chamarty, 2009-09-02 05:30 Subject: Re: clong an empty repo over ssh causes (harmless) fatal Message-ID: <2e24e5b90909012230s554b8776q515d21d88e6ed1d6@mail.gmail.com> URL: https://gitlist.dev/e/2e24e5b90909012230s554b8776q515d21d88e6ed1d6%40mail.gmail.com In-Reply-To: <20090831224749.GA24190@sigill.intra.peff.net> ``` 2009/9/1 Jeff King : > 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 ``` ## Daniel Barkalow, 2009-09-02 06:02 Subject: Re: clong an empty repo over ssh causes (harmless) fatal Message-ID: URL: https://gitlist.dev/e/alpine.LNX.2.00.0909020159080.28290%40iabervon.org In-Reply-To: <20090902051615.GC12046@coredump.intra.peff.net> ``` 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* ``` ## Jeff King, 2009-09-02 06:36 Subject: [PATCH] clone: disconnect transport after fetching Message-ID: <20090902063647.GA29559@coredump.intra.peff.net> URL: https://gitlist.dev/e/20090902063647.GA29559%40coredump.intra.peff.net In-Reply-To: ``` 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 --- This was suggested by Daniel, so theoretically Acked-by: Daniel Barkalow :) 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 ``` ## Sverre Rabbelier, 2009-09-02 07:09 Subject: Re: [PATCH] clone: disconnect transport after fetching Message-ID: URL: https://gitlist.dev/e/fabb9a1e0909020009r3ee28b1fo3cba095abafec9d4%40mail.gmail.com In-Reply-To: <20090902063647.GA29559@coredump.intra.peff.net> ``` Heya, On Wed, Sep 2, 2009 at 08:36, Jeff King 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 ``` ## Jeff King, 2009-09-02 07:26 Subject: Re: [PATCH] clone: disconnect transport after fetching Message-ID: <20090902072638.GC31528@coredump.intra.peff.net> URL: https://gitlist.dev/e/20090902072638.GC31528%40coredump.intra.peff.net In-Reply-To: ``` On Wed, Sep 02, 2009 at 09:09:19AM +0200, Sverre Rabbelier wrote: > On Wed, Sep 2, 2009 at 08:36, Jeff King 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 ``` ## Sverre Rabbelier, 2009-09-02 07:37 Subject: Re: [PATCH] clone: disconnect transport after fetching Message-ID: URL: https://gitlist.dev/e/fabb9a1e0909020037m4ce6237ana6dec41088bba596%40mail.gmail.com In-Reply-To: <20090902072638.GC31528@coredump.intra.peff.net> ``` Heya, On Wed, Sep 2, 2009 at 09:26, Jeff King 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 ``` ## Daniel Barkalow, 2009-09-02 16:38 Subject: Re: [PATCH] clone: disconnect transport after fetching Message-ID: URL: https://gitlist.dev/e/alpine.LNX.2.00.0909021228570.28290%40iabervon.org In-Reply-To: <20090902063647.GA29559@coredump.intra.peff.net> ``` 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 > --- > This was suggested by Daniel, so theoretically > > Acked-by: Daniel Barkalow > > :) This is what I intended, so: Acked-by: Daniel Barkalow > 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 > ``` ## Junio C Hamano, 2009-09-02 17:55 Subject: Re: [PATCH] clone: disconnect transport after fetching Message-ID: <7veiqp1b9v.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7veiqp1b9v.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` Daniel Barkalow writes: > On Wed, 2 Sep 2009, Jeff King wrote: > ... >> Signed-off-by: Jeff King >> --- >> This was suggested by Daniel, so theoretically >> >> Acked-by: Daniel Barkalow >> >> :) > > This is what I intended, so: > > Acked-by: Daniel Barkalow Thanks, both. ``` ## Jeff King, 2009-09-04 02:30 Subject: Re: [PATCH] clone: disconnect transport after fetching Message-ID: <20090904023013.GA14651@coredump.intra.peff.net> URL: https://gitlist.dev/e/20090904023013.GA14651%40coredump.intra.peff.net In-Reply-To: <20090902063647.GA29559@coredump.intra.peff.net> ``` 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 ```