[PATCH] Allow cloning an empty repository

[PATCH] Allow cloning an empty repository

[Sverre Rabbelier]

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 <srabbelier@gmail.com>
---

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

Re: [PATCH] Allow cloning an empty repository

[Junio C Hamano]

Sverre Rabbelier <srabbelier@gmail.com> 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.

Re: [PATCH] Allow cloning an empty repository

[Sverre Rabbelier]

On Fri, Jan 23, 2009 at 02:24, Junio C Hamano <gitster@pobox.com> 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

Re: [PATCH] Allow cloning an empty repository

[Johannes Schindelin]

Hi,

On Fri, 23 Jan 2009, Sverre Rabbelier wrote:

> On Fri, Jan 23, 2009 at 02:24, Junio C Hamano <gitster@pobox.com> 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

Re: [PATCH] Allow cloning an empty repository

[Sverre Rabbelier]

On Fri, Jan 23, 2009 at 03:42, Johannes Schindelin
<Johannes.Schindelin@gmx.de> 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

Re: [PATCH] Allow cloning an empty repository

[Johannes Schindelin]

Hi,

On Fri, 23 Jan 2009, Sverre Rabbelier wrote:

> On Fri, Jan 23, 2009 at 03:42, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> 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

Re: [PATCH] Allow cloning an empty repository

[Miklos Vajna]

[-- Attachment #1: Type: text/plain, Size: 424 bytes --]

On Fri, Jan 23, 2009 at 04:20:34AM +0100, Johannes Schindelin <Johannes.Schindelin@gmx.de> 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".

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] Allow cloning an empty repository

[Johannes Schindelin]

Hi,

On Fri, 23 Jan 2009, Miklos Vajna wrote:

> On Fri, Jan 23, 2009 at 04:20:34AM +0100, Johannes Schindelin 
> <Johannes.Schindelin@gmx.de> 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

Re: [PATCH] Allow cloning an empty repository

[Mike Hommey]

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 <gitster@pobox.com> 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

Re: [PATCH] Allow cloning an empty repository

[Jeff King]

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

Re: [PATCH] Allow cloning an empty repository

[Sverre Rabbelier]

On Fri, Jan 23, 2009 at 23:32, Jeff King <peff@peff.net> 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

Re: [PATCH] Allow cloning an empty repository

[Jeff King]

On Fri, Jan 23, 2009 at 11:34:04PM +0100, Sverre Rabbelier wrote:

> On Fri, Jan 23, 2009 at 23:32, Jeff King <peff@peff.net> 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

Re: [PATCH] Allow cloning an empty repository

[Sverre Rabbelier]

On Fri, Jan 23, 2009 at 23:37, Jeff King <peff@peff.net> 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

Re: [PATCH] Allow cloning an empty repository

[Miklos Vajna]

[-- Attachment #1: Type: text/plain, Size: 943 bytes --]

On Fri, Jan 23, 2009 at 03:42:24AM +0100, Johannes Schindelin <Johannes.Schindelin@gmx.de> 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?

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

http fixes, was Re: [PATCH] Allow cloning an empty repository

[Johannes Schindelin]

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

Re: [PATCH] Allow cloning an empty repository

[Johannes Schindelin]

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

Re: [PATCH] Allow cloning an empty repository

[Johannes Schindelin]

Hi,

On Sat, 24 Jan 2009, Miklos Vajna wrote:

> On Fri, Jan 23, 2009 at 03:42:24AM +0100, Johannes Schindelin 
> <Johannes.Schindelin@gmx.de> 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

Re: [PATCH] Allow cloning an empty repository

[Sverre Rabbelier]

On Sat, Jan 24, 2009 at 01:33, Johannes Schindelin
<Johannes.Schindelin@gmx.de> 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

Re: http fixes, was Re: [PATCH] Allow cloning an empty repository

[Sverre Rabbelier]

On Sat, Jan 24, 2009 at 01:26, Johannes Schindelin
<Johannes.Schindelin@gmx.de> 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

Re: [PATCH] Allow cloning an empty repository

[Miklos Vajna]

[-- Attachment #1: Type: text/plain, Size: 144 bytes --]

On Sat, Jan 24, 2009 at 01:42:30AM +0100, Sverre Rabbelier <srabbelier@gmail.com> wrote:
> Which means all protocols actually die?

I think so.

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] Allow cloning an empty repository

[Sverre Rabbelier]

Heya,

On Sun, Jan 25, 2009 at 01:49, Miklos Vajna <vmiklos@frugalware.org> wrote:
> On Sat, Jan 24, 2009 at 01:42:30AM +0100, Sverre Rabbelier <srabbelier@gmail.com> 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

Re: [PATCH] Allow cloning an empty repository

[Junio C Hamano]

Sverre Rabbelier <srabbelier@gmail.com> writes:

> On Sun, Jan 25, 2009 at 01:49, Miklos Vajna <vmiklos@frugalware.org> wrote:
>> On Sat, Jan 24, 2009 at 01:42:30AM +0100, Sverre Rabbelier <srabbelier@gmail.com> 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".

Re: [PATCH] Allow cloning an empty repository

[Sverre Rabbelier]

On Sun, Jan 25, 2009 at 06:05, Junio C Hamano <gitster@pobox.com> 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