# [PATCH] Allow cloning an empty repository 23 messages from 2009-01-23 to 2009-01-25. Participants: Sverre Rabbelier, Junio C Hamano, Johannes Schindelin, Miklos Vajna, Mike Hommey, Jeff King. Thread: https://gitlist.dev/t/17315 ## Sverre Rabbelier, 2009-01-23 00:07 Subject: [PATCH] Allow cloning an empty repository Message-ID: <1232669252-21881-1-git-send-email-srabbelier@gmail.com> URL: https://gitlist.dev/e/1232669252-21881-1-git-send-email-srabbelier%40gmail.com ``` Cloning an empty repository manually (that is, doing 'git init' and then doing all configuration by hand) can be a lot of work. Save the user this work by allowing the cloning of empty repositories. Signed-off-by: Sverre Rabbelier --- Thanks to Dscho for giving some pointers on how to do this. builtin-clone.c | 17 +++++++++++++---- t/t5701-clone-local.sh | 16 ++++++++++++++++ 2 files changed, 29 insertions(+), 4 deletions(-) diff --git a/builtin-clone.c b/builtin-clone.c index f7e5a7b..d5bba0e 100644 --- a/builtin-clone.c +++ b/builtin-clone.c @@ -522,14 +522,23 @@ int cmd_clone(int argc, const char **argv, const char *prefix) option_upload_pack); refs = transport_get_remote_refs(transport); - transport_fetch_refs(transport, refs); + if(refs) + transport_fetch_refs(transport, refs); } - clear_extra_refs(); + if(refs) { + clear_extra_refs(); - mapped_refs = write_remote_refs(refs, &refspec, reflog_msg.buf); + mapped_refs = write_remote_refs(refs, &refspec, reflog_msg.buf); - head_points_at = locate_head(refs, mapped_refs, &remote_head); + head_points_at = locate_head(refs, mapped_refs, &remote_head); + } + else { + warning("You appear to have cloned an empty repository."); + head_points_at = NULL; + remote_head = NULL; + option_no_checkout = 1; + } if (head_points_at) { /* Local default branch link */ diff --git a/t/t5701-clone-local.sh b/t/t5701-clone-local.sh index 8dfaaa4..fbd9bfa 100755 --- a/t/t5701-clone-local.sh +++ b/t/t5701-clone-local.sh @@ -116,4 +116,20 @@ test_expect_success 'bundle clone with nonexistent HEAD' ' test ! -e .git/refs/heads/master ' +test_expect_success 'clone empty repository' ' + cd "$D" && + mkdir empty && + (cd empty && git init) && + git clone empty empty-clone && + test_tick && + (cd empty-clone + echo "content" >> foo && + git add foo && + git commit -m "Initial commit" && + git push origin master && + expected=$(git rev-parse master) && + actual=$(git --git-dir=../empty/.git rev-parse master) && + test $actual = $expected) +' + test_done -- 1.6.1.399.g0d272.dirty ``` ## Junio C Hamano, 2009-01-23 01:24 Subject: Re: [PATCH] Allow cloning an empty repository Message-ID: <7vwscm4xx0.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vwscm4xx0.fsf%40gitster.siamese.dyndns.org In-Reply-To: <1232669252-21881-1-git-send-email-srabbelier@gmail.com> ``` Sverre Rabbelier writes: > diff --git a/builtin-clone.c b/builtin-clone.c > index f7e5a7b..d5bba0e 100644 > --- a/builtin-clone.c > +++ b/builtin-clone.c > @@ -522,14 +522,23 @@ int cmd_clone(int argc, const char **argv, const char *prefix) > option_upload_pack); > > refs = transport_get_remote_refs(transport); > - transport_fetch_refs(transport, refs); > + if(refs) > + transport_fetch_refs(transport, refs); Thanks. I think the basic idea is Ok, but is it a reliable check at this point to see if (refs == NULL) to tell if the target repository is an empty one? I am mostly worried about a failure case (connected but couldn't get the refs, or perhaps connection failed to start). If you get a NULL in such a case you may end up saying "oh you cloned a void" when you should say "nah, such a remote repository does not exist". If transport_get_remote_refs() dies without returning NULL, that would be sufficient, but I didn't check. ``` ## Sverre Rabbelier, 2009-01-23 01:46 Subject: Re: [PATCH] Allow cloning an empty repository Message-ID: URL: https://gitlist.dev/e/bd6139dc0901221746h258f548etf857ab37399133da%40mail.gmail.com In-Reply-To: <7vwscm4xx0.fsf@gitster.siamese.dyndns.org> ``` On Fri, Jan 23, 2009 at 02:24, Junio C Hamano wrote: > I think the basic idea is Ok, but is it a reliable check at this point to > see if (refs == NULL) to tell if the target repository is an empty one? This is the question I asked Dscho, and he said/guessed that it was. > I am mostly worried about a failure case (connected but couldn't get the > refs, or perhaps connection failed to start). If you get a NULL in such a > case you may end up saying "oh you cloned a void" when you should say > "nah, such a remote repository does not exist". Yes, this was my concern as well. > If transport_get_remote_refs() dies without returning NULL, that would be > sufficient, but I didn't check. It does, transport_get_remote_refs() calls fetch_pack, which dies when 'refs' (the fourth argument) is NULL: static int fetch_refs_via_pack(...) { // setup code snipped if (!data->conn) { connect_setup(transport); get_remote_heads(data->fd[0], &refs_tmp, 0, NULL, 0, NULL); } refs = fetch_pack(&args, data->fd, data->conn, refs_tmp ? refs_tmp : transport->remote_refs, dest, nr_heads, heads, &transport->pack_lockfile); So unless get_remote_heads messes with it, it will remain NULL. Now I must admit that I'm not familiar enough with get_remote_heads to know if this could cause a false positive, Dscho? -- Cheers, Sverre Rabbelier ``` ## Johannes Schindelin, 2009-01-23 02:42 Subject: Re: [PATCH] Allow cloning an empty repository Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0901230333060.3586%40pacific.mpi-cbg.de In-Reply-To: ``` Hi, On Fri, 23 Jan 2009, Sverre Rabbelier wrote: > On Fri, Jan 23, 2009 at 02:24, Junio C Hamano wrote: > > I think the basic idea is Ok, but is it a reliable check at this point to > > see if (refs == NULL) to tell if the target repository is an empty one? > > This is the question I asked Dscho, and he said/guessed that it was. > > > I am mostly worried about a failure case (connected but couldn't get > > the refs, or perhaps connection failed to start). If you get a NULL > > in such a case you may end up saying "oh you cloned a void" when you > > should say "nah, such a remote repository does not exist". > > Yes, this was my concern as well. >From what I can see in get_remote_heads(), the native protocols would die(), as would rsync(). HTTP transport, however, would not die() on connection errors, from my cursory look. That might be skewed, though, as I am on top of Mike's patches (in the hopefully not so futile hope that Mike -- after letting me wait for over one year -- finishes his work. HTTP being 2nd class citizen anyway (and we can always fix it after Mike's cleanups), I'd say this patch is ready to roll. Ciao, Dscho ``` ## Sverre Rabbelier, 2009-01-23 02:47 Subject: Re: [PATCH] Allow cloning an empty repository Message-ID: URL: https://gitlist.dev/e/bd6139dc0901221847u6b8aeadcl580a091751de3d26%40mail.gmail.com In-Reply-To: ``` On Fri, Jan 23, 2009 at 03:42, Johannes Schindelin wrote: > HTTP transport, however, would not die() on connection errors, from my > cursory look. Heh, this is why I asked you, my cursory look told me "no clue" instead. > HTTP being 2nd class citizen anyway (and we can always fix it after Mike's > cleanups), I'd say this patch is ready to roll. Aside from that, am I correct in asserting that in the "worst case" scenario the repo is cloned instead of erroring out, and a simple "git fetch" would fix the issue? -- Cheers, Sverre Rabbelier ``` ## Johannes Schindelin, 2009-01-23 03:20 Subject: Re: [PATCH] Allow cloning an empty repository Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0901230419080.3586%40pacific.mpi-cbg.de In-Reply-To: ``` Hi, On Fri, 23 Jan 2009, Sverre Rabbelier wrote: > On Fri, Jan 23, 2009 at 03:42, Johannes Schindelin > wrote: > > > HTTP being 2nd class citizen anyway (and we can always fix it after > > Mike's cleanups), I'd say this patch is ready to roll. > > Aside from that, am I correct in asserting that in the "worst case" > scenario the repo is cloned instead of erroring out, and a simple "git > fetch" would fix the issue? Probably. Note, however, that scripts might rely on a fail if there were problems. But then, scripts have no business cloning repositories (fetching, yes. But cloning?) Ciao, Dscho ``` ## Miklos Vajna, 2009-01-23 16:55 Subject: Re: [PATCH] Allow cloning an empty repository Message-ID: <20090123165549.GI21473@genesis.frugalware.org> URL: https://gitlist.dev/e/20090123165549.GI21473%40genesis.frugalware.org In-Reply-To: ``` On Fri, Jan 23, 2009 at 04:20:34AM +0100, Johannes Schindelin wrote: > But then, scripts have no business cloning repositories (fetching, yes. > But cloning?) I think portals like repo.or.cz may do it. Isn't setting errno (or similar variable) in the HTTP code an option? Then we could see why the transport failed and make a difference between "network error" and "no refs found". ``` ## Johannes Schindelin, 2009-01-23 20:05 Subject: Re: [PATCH] Allow cloning an empty repository Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0901232100020.21467%40intel-tinevez-2-302 In-Reply-To: <20090123165549.GI21473@genesis.frugalware.org> ``` Hi, On Fri, 23 Jan 2009, Miklos Vajna wrote: > On Fri, Jan 23, 2009 at 04:20:34AM +0100, Johannes Schindelin > wrote: > > > But then, scripts have no business cloning repositories (fetching, > > yes. But cloning?) > > I think portals like repo.or.cz may do it. "git grep git.clone" in repo.git suggests that only clonecheck.sh uses it, which is not called anywhere. But more generally, the same should apply to such scripts as to human users: HTTP _is_ 2nd class, and the fetch _could_ succeed later. > Isn't setting errno (or similar variable) in the HTTP code an option? > Then we could see why the transport failed and make a difference between > "network error" and "no refs found". I'd rather see the HTTP code path taken by transport.c _fail_ (i.e. die()) when there is some sort of connection error. Ciao, Dscho ``` ## Mike Hommey, 2009-01-23 21:55 Subject: Re: [PATCH] Allow cloning an empty repository Message-ID: <20090123215559.GA5561@glandium.org> URL: https://gitlist.dev/e/20090123215559.GA5561%40glandium.org In-Reply-To: ``` On Fri, Jan 23, 2009 at 03:42:24AM +0100, Johannes Schindelin wrote: > Hi, > > On Fri, 23 Jan 2009, Sverre Rabbelier wrote: > > > On Fri, Jan 23, 2009 at 02:24, Junio C Hamano wrote: > > > I think the basic idea is Ok, but is it a reliable check at this point to > > > see if (refs == NULL) to tell if the target repository is an empty one? > > > > This is the question I asked Dscho, and he said/guessed that it was. > > > > > I am mostly worried about a failure case (connected but couldn't get > > > the refs, or perhaps connection failed to start). If you get a NULL > > > in such a case you may end up saying "oh you cloned a void" when you > > > should say "nah, such a remote repository does not exist". > > > > Yes, this was my concern as well. > > From what I can see in get_remote_heads(), the native protocols would > die(), as would rsync(). > > HTTP transport, however, would not die() on connection errors, from my > cursory look. > > That might be skewed, though, as I am on top of Mike's patches (in the > hopefully not so futile hope that Mike -- after letting me wait for over > one year -- finishes his work. As I said when posting my patch batch, I don't have much time nor motivation to work on this series. But let's make a deal: if someone writes a good enough http test suite, I'll polish the http code. Mike ``` ## Jeff King, 2009-01-23 22:32 Subject: Re: [PATCH] Allow cloning an empty repository Message-ID: <20090123223205.GA11491@coredump.intra.peff.net> URL: https://gitlist.dev/e/20090123223205.GA11491%40coredump.intra.peff.net In-Reply-To: ``` On Fri, Jan 23, 2009 at 04:20:34AM +0100, Johannes Schindelin wrote: > Probably. Note, however, that scripts might rely on a fail if there were > problems. > > But then, scripts have no business cloning repositories (fetching, yes. > But cloning?) Really? I have scripts that call clone (usually followed by building the result). Are you proposing that all scripts should "git init && git remote add && git fetch"? So I am strongly in favor of telling the difference between failure and emptiness. -Peff ``` ## Sverre Rabbelier, 2009-01-23 22:34 Subject: Re: [PATCH] Allow cloning an empty repository Message-ID: URL: https://gitlist.dev/e/bd6139dc0901231434n6d6af5a6ma8bb12a634d5b3ba%40mail.gmail.com In-Reply-To: <20090123223205.GA11491@coredump.intra.peff.net> ``` On Fri, Jan 23, 2009 at 23:32, Jeff King wrote: > Really? I have scripts that call clone (usually followed by building the > result). Are you proposing that all scripts should "git init && git > remote add && git fetch"? > > So I am strongly in favor of telling the difference between failure and > emptiness. A switch then, '--allow-empty'? -- Cheers, Sverre Rabbelier ``` ## Jeff King, 2009-01-23 22:37 Subject: Re: [PATCH] Allow cloning an empty repository Message-ID: <20090123223740.GA11527@coredump.intra.peff.net> URL: https://gitlist.dev/e/20090123223740.GA11527%40coredump.intra.peff.net In-Reply-To: ``` On Fri, Jan 23, 2009 at 11:34:04PM +0100, Sverre Rabbelier wrote: > On Fri, Jan 23, 2009 at 23:32, Jeff King wrote: > > Really? I have scripts that call clone (usually followed by building the > > result). Are you proposing that all scripts should "git init && git > > remote add && git fetch"? > > > > So I am strongly in favor of telling the difference between failure and > > emptiness. > > A switch then, '--allow-empty'? No, I don't mind success on cloning an empty repository. But I thought the issue at hand was that for some instances, we would report that we successfully created an empty repository, when in fact what happened was that we failed to clone a non-empty repository. And that that was fixable, but it was a problem with our code interfaces (which should be fixable) and not some fundamental limitation. Or am I misunderstanding the situation? -Peff ``` ## Sverre Rabbelier, 2009-01-23 22:39 Subject: Re: [PATCH] Allow cloning an empty repository Message-ID: URL: https://gitlist.dev/e/bd6139dc0901231439x1e20f06bka90a228a883681ff%40mail.gmail.com In-Reply-To: <20090123223740.GA11527@coredump.intra.peff.net> ``` On Fri, Jan 23, 2009 at 23:37, Jeff King wrote: > No, I don't mind success on cloning an empty repository. But I thought > the issue at hand was that for some instances, we would report that we > successfully created an empty repository, when in fact what happened was > that we failed to clone a non-empty repository. And that that was > fixable, but it was a problem with our code interfaces (which should be > fixable) and not some fundamental limitation. Ah, mhh, if that is the case then I'm afraid that's beyond me (at least for now), someone else would have to do said refactoring first :(. > Or am I misunderstanding the situation? I think you summarized it pretty well, if that is indeed what Junio meant. -- Cheers, Sverre Rabbelier ``` ## Miklos Vajna, 2009-01-23 23:05 Subject: Re: [PATCH] Allow cloning an empty repository Message-ID: <20090123230520.GL21473@genesis.frugalware.org> URL: https://gitlist.dev/e/20090123230520.GL21473%40genesis.frugalware.org In-Reply-To: ``` On Fri, Jan 23, 2009 at 03:42:24AM +0100, Johannes Schindelin wrote: > > > I am mostly worried about a failure case (connected but couldn't get > > > the refs, or perhaps connection failed to start). If you get a NULL > > > in such a case you may end up saying "oh you cloned a void" when you > > > should say "nah, such a remote repository does not exist". > > > > Yes, this was my concern as well. > > From what I can see in get_remote_heads(), the native protocols would > die(), as would rsync(). > > HTTP transport, however, would not die() on connection errors, from my > cursory look. I'm not familiar with the HTTP code, either, but here is the call stack I see: - builtin-clone calls transport_get_remote_refs() - that will call transport->get_refs_list() - that will call get_refs_via_curl() - that die()s on error, does not use return error() Have I missed something? ``` ## Johannes Schindelin, 2009-01-24 00:26 Subject: http fixes, was Re: [PATCH] Allow cloning an empty repository Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0901240125190.3586%40pacific.mpi-cbg.de In-Reply-To: <20090123215559.GA5561@glandium.org> ``` Hi, On Fri, 23 Jan 2009, Mike Hommey wrote: > As I said when posting my patch batch, I don't have much time nor > motivation to work on this series. But let's make a deal: if someone > writes a good enough http test suite, I'll polish the http code. I already said in my replied to your patch that I will add the http test suite if you fix your patches. Ciao, Dscho ``` ## Johannes Schindelin, 2009-01-24 00:28 Subject: Re: [PATCH] Allow cloning an empty repository Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0901240127110.3586%40pacific.mpi-cbg.de In-Reply-To: <20090123223740.GA11527@coredump.intra.peff.net> ``` Hi, On Fri, 23 Jan 2009, Jeff King wrote: > But I thought the issue at hand was that for some instances, we would > report that we successfully created an empty repository, when in fact > what happened was that we failed to clone a non-empty repository. And > that that was fixable, but it was a problem with our code interfaces > (which should be fixable) and not some fundamental limitation. It appears that the HTTP code (at least after the first round of Mike's patches) has issues there, yes, but I think it would not be fair to stop Sverre's patch because of that: the HTTP code needs fixing anyway, and this fix is orthogonal to the empty clones. Ciao, Dscho ``` ## Johannes Schindelin, 2009-01-24 00:33 Subject: Re: [PATCH] Allow cloning an empty repository Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0901240131570.3586%40pacific.mpi-cbg.de In-Reply-To: <20090123230520.GL21473@genesis.frugalware.org> ``` Hi, On Sat, 24 Jan 2009, Miklos Vajna wrote: > On Fri, Jan 23, 2009 at 03:42:24AM +0100, Johannes Schindelin > wrote: > > > > I am mostly worried about a failure case (connected but couldn't > > > > get the refs, or perhaps connection failed to start). If you get > > > > a NULL in such a case you may end up saying "oh you cloned a void" > > > > when you should say "nah, such a remote repository does not > > > > exist". > > > > > > Yes, this was my concern as well. > > > > From what I can see in get_remote_heads(), the native protocols would > > die(), as would rsync(). > > > > HTTP transport, however, would not die() on connection errors, from my > > cursory look. > > I'm not familiar with the HTTP code, either, but here is the call stack > I see: > > - builtin-clone calls transport_get_remote_refs() > - that will call transport->get_refs_list() > - that will call get_refs_via_curl() > - that die()s on error, does not use return error() > > Have I missed something? Not really. Only after writing my email (and just before sending) did I remember that I have Mike's HTTP cleanups applied in my Git checkout. So I was analyzing the wrong code. Thanks for analyzing the right code. Ciao, Dscho ``` ## Sverre Rabbelier, 2009-01-24 00:42 Subject: Re: [PATCH] Allow cloning an empty repository Message-ID: URL: https://gitlist.dev/e/bd6139dc0901231642v6bd593d3mfefaca3c419a9f0a%40mail.gmail.com In-Reply-To: ``` On Sat, Jan 24, 2009 at 01:33, Johannes Schindelin wrote: > On Sat, 24 Jan 2009, Miklos Vajna wrote: >> I'm not familiar with the HTTP code, either, but here is the call stack >> I see: >> >> - builtin-clone calls transport_get_remote_refs() >> - that will call transport->get_refs_list() >> - that will call get_refs_via_curl() >> - that die()s on error, does not use return error() Which means all protocols actually die? > Only after writing my email (and just before sending) did I remember that > I have Mike's HTTP cleanups applied in my Git checkout. So I was > analyzing the wrong code. Hmmm, can't blame you considering it was around 4am ;). -- Cheers, Sverre Rabbelier ``` ## Sverre Rabbelier, 2009-01-24 09:39 Subject: Re: http fixes, was Re: [PATCH] Allow cloning an empty repository Message-ID: URL: https://gitlist.dev/e/bd6139dc0901240139l76ea28bq2610bf0ac07087b3%40mail.gmail.com In-Reply-To: ``` On Sat, Jan 24, 2009 at 01:26, Johannes Schindelin wrote: > On Fri, 23 Jan 2009, Mike Hommey wrote: >> As I said when posting my patch batch, I don't have much time nor >> motivation to work on this series. But let's make a deal: if someone >> writes a good enough http test suite, I'll polish the http code. > > I already said in my replied to your patch that I will add the http test > suite if you fix your patches. I think Mike meant that he wants a working test suite before continuing work on it? As in, what's the point in working on something if you don't have a working test-suite to test your solution against ;). -- Cheers, Sverre Rabbelier ``` ## Miklos Vajna, 2009-01-25 00:49 Subject: Re: [PATCH] Allow cloning an empty repository Message-ID: <20090125004955.GY21473@genesis.frugalware.org> URL: https://gitlist.dev/e/20090125004955.GY21473%40genesis.frugalware.org In-Reply-To: ``` On Sat, Jan 24, 2009 at 01:42:30AM +0100, Sverre Rabbelier wrote: > Which means all protocols actually die? I think so. ``` ## Sverre Rabbelier, 2009-01-25 00:55 Subject: Re: [PATCH] Allow cloning an empty repository Message-ID: URL: https://gitlist.dev/e/bd6139dc0901241655n66f75a8fn8450b774809fa8e7%40mail.gmail.com In-Reply-To: <20090125004955.GY21473@genesis.frugalware.org> ``` Heya, On Sun, Jan 25, 2009 at 01:49, Miklos Vajna wrote: > On Sat, Jan 24, 2009 at 01:42:30AM +0100, Sverre Rabbelier wrote: >> Which means all protocols actually die? > > I think so. Junio, are your concerns address now, since Miklos confirmed that transport_get_remote_refs always dies? -- Cheers, Sverre Rabbelier ``` ## Junio C Hamano, 2009-01-25 05:05 Subject: Re: [PATCH] Allow cloning an empty repository Message-ID: <7vy6x0j7py.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vy6x0j7py.fsf%40gitster.siamese.dyndns.org In-Reply-To: ``` Sverre Rabbelier writes: > On Sun, Jan 25, 2009 at 01:49, Miklos Vajna wrote: >> On Sat, Jan 24, 2009 at 01:42:30AM +0100, Sverre Rabbelier wrote: >>> Which means all protocols actually die? >> >> I think so. > > Junio, are your concerns address now, since Miklos confirmed that > transport_get_remote_refs always dies? Yeah, it is on 'next' and I've pushed the results out last night, but I got sick and didn't manage to send out "What's cooking". ``` ## Sverre Rabbelier, 2009-01-25 18:33 Subject: Re: [PATCH] Allow cloning an empty repository Message-ID: URL: https://gitlist.dev/e/bd6139dc0901251033g166478d5y2050977cc7beaa5a%40mail.gmail.com In-Reply-To: <7vy6x0j7py.fsf@gitster.siamese.dyndns.org> ``` On Sun, Jan 25, 2009 at 06:05, Junio C Hamano wrote: > Yeah, it is on 'next' and I've pushed the results out last night, but I > got sick and didn't manage to send out "What's cooking". Ok, awesome! :). -- Cheers, Sverre Rabbelier ```