[PATCH] receive-pack: Create a HEAD ref for ref namespace

[PATCH] receive-pack: Create a HEAD ref for ref namespace

[Johannes Löthberg]

Each ref namespace have their own separate branches, tags, and HEAD, so
when pushing to a namespace we need to make sure that there exists a
HEAD ref for the namespace, otherwise you will not be able to check out
the repo after cloning from a namespace
---

So, I have absolutely no clue where this should actually be put, so I 
just put it where it fit for now.

Any comments on where to put it, or comments on the patch in general?

 builtin/receive-pack.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 5292bb5..c189838 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -864,7 +864,9 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 {
 	const char *name = cmd->ref_name;
 	struct strbuf namespaced_name_buf = STRBUF_INIT;
-	const char *namespaced_name, *ret;
+	struct strbuf namespaced_head_buf = STRBUF_INIT;
+	const char *namespaced_name, *ret, *namespace;
+	const char *namespaced_head_path;
 	unsigned char *old_sha1 = cmd->old_sha1;
 	unsigned char *new_sha1 = cmd->new_sha1;
 
@@ -981,6 +983,14 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 		return NULL; /* good */
 	}
 	else {
+		namespace = get_git_namespace();
+		if (strcmp(namespace, "refs/namespaces/")) {
+			strbuf_addf(&namespaced_head_buf, "%s%s", namespace, "HEAD");
+			namespaced_head_path = strbuf_detach(&namespaced_head_buf, NULL);
+
+			create_symref(namespaced_head_path, namespaced_name, NULL);
+		}
+
 		struct strbuf err = STRBUF_INIT;
 		if (shallow_update && si->shallow_ref[cmd->index] &&
 		    update_shallow_ref(cmd, si))
-- 
2.4.2

Re: [PATCH] receive-pack: Create a HEAD ref for ref namespace

[Johannes Löthberg]

Ping.

--
Sincerely, 
Johannes Löthberg 
(Sent from my phone.)

Re: [PATCH] receive-pack: Create a HEAD ref for ref namespace

[Michael J Gruber]

Johannes Löthberg venit, vidit, dixit 05.06.2015 13:53:
> Ping.
> 
> --
> Sincerely, 
> Johannes Löthberg 
> (Sent from my phone.)
> 

It appears your patch proposes to fix a problem. It's a good idea to
expose the problem by writing a test so that one can check that the fix
actually fixes the problem.

(Also, your patch duplicates the line "struct strbuf namespaced_head_buf
= STRBUF_INIT;")

Michael

Re: [PATCH] receive-pack: Create a HEAD ref for ref namespace

[Johannes Löthberg]

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

On 05/06, Michael J Gruber wrote:
>It appears your patch proposes to fix a problem. It's a good idea to
>expose the problem by writing a test so that one can check that the fix
>actually fixes the problem.
>

Right, will look into writing a test for it.

>(Also, your patch duplicates the line "struct strbuf 
>namespaced_head_buf
>= STRBUF_INIT;")
>

Hmm, that's weird, no clue how that happened. Thanks.

-- 
Sincerely,
  Johannes Löthberg
  PGP Key ID: 0x50FB9B273A9D0BB5
  https://theos.kyriasis.com/~kyrias/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 1495 bytes --]

Re: [PATCH] receive-pack: Create a HEAD ref for ref namespace

[Johannes Löthberg]

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

On 05/06, Michael J Gruber wrote:
>(Also, your patch duplicates the line "struct strbuf namespaced_head_buf
>= STRBUF_INIT;")
>

I replied too soon, it doesn't duplicate it, it's a different variable 
named similarly.

-- 
Sincerely,
  Johannes Löthberg
  PGP Key ID: 0x50FB9B273A9D0BB5
  https://theos.kyriasis.com/~kyrias/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 1495 bytes --]

[PATCH v2] Fix cloning from ref namespace

[Johannes Löthberg]

Since v1:
  * Added a test case

Johannes Löthberg (2):
  receive-pack: Create a HEAD ref for ref namespace
  t: Add test for cloning from ref namespace

 builtin/receive-pack.c              | 12 +++++++++++-
 t/t9904-clone-from-ref-namespace.sh | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 1 deletion(-)
 create mode 100755 t/t9904-clone-from-ref-namespace.sh

-- 
2.4.2

[PATCH v2 1/2] receive-pack: Create a HEAD ref for ref namespace

[Johannes Löthberg]

Each ref namespace have their own separate branches, tags, and HEAD, so
when pushing to a namespace we need to make sure that there exists a
HEAD ref for the namespace, otherwise you will not be able to check out
the repo after cloning from a namespace

Signed-off-by: Johannes Löthberg <johannes@kyriasis.com>
---
 builtin/receive-pack.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 5292bb5..c189838 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -864,7 +864,9 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 {
 	const char *name = cmd->ref_name;
 	struct strbuf namespaced_name_buf = STRBUF_INIT;
-	const char *namespaced_name, *ret;
+	struct strbuf namespaced_head_buf = STRBUF_INIT;
+	const char *namespaced_name, *ret, *namespace;
+	const char *namespaced_head_path;
 	unsigned char *old_sha1 = cmd->old_sha1;
 	unsigned char *new_sha1 = cmd->new_sha1;
 
@@ -981,6 +983,14 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 		return NULL; /* good */
 	}
 	else {
+		namespace = get_git_namespace();
+		if (strcmp(namespace, "refs/namespaces/")) {
+			strbuf_addf(&namespaced_head_buf, "%s%s", namespace, "HEAD");
+			namespaced_head_path = strbuf_detach(&namespaced_head_buf, NULL);
+
+			create_symref(namespaced_head_path, namespaced_name, NULL);
+		}
+
 		struct strbuf err = STRBUF_INIT;
 		if (shallow_update && si->shallow_ref[cmd->index] &&
 		    update_shallow_ref(cmd, si))
-- 
2.4.2

[PATCH v2 2/2] t: Add test for cloning from ref namespace

[Johannes Löthberg]

Test that the master ref is set up properly when cloning from a ref
namespace

Signed-off-by: Johannes Löthberg <johannes@kyriasis.com>
---
 t/t9904-clone-from-ref-namespace.sh | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)
 create mode 100755 t/t9904-clone-from-ref-namespace.sh

diff --git a/t/t9904-clone-from-ref-namespace.sh b/t/t9904-clone-from-ref-namespace.sh
new file mode 100755
index 0000000..60977f8
--- /dev/null
+++ b/t/t9904-clone-from-ref-namespace.sh
@@ -0,0 +1,33 @@
+#!/bin/sh
+#
+
+test_description='git clone from ref namespace
+
+This test checks that cloning from a ref namespace works'
+
+. ./test-lib.sh
+
+test_expect_success 'clone from ref namespace' '
+	rm -rf initial bare clone &&
+	git init initial &&
+	git init --bare bare &&
+	(
+		cd initial &&
+		echo "commit one" >> file &&
+		git add file &&
+		git commit -m "commit one" &&
+		git push ../bare master &&
+
+		echo "commit two" >> file &&
+		git add file &&
+		git commit -m "commit two"
+		GIT_NAMESPACE=new_namespace git push ../bare master
+	) &&
+	GIT_NAMESPACE=new_namespace git clone bare clone &&
+	(
+		cd clone &&
+		git show
+	)
+'
+
+test_done
-- 
2.4.2

Re: [PATCH v2 2/2] t: Add test for cloning from ref namespace

[Junio C Hamano]

Johannes Löthberg <johannes@kyriasis.com> writes:

> Test that the master ref is set up properly when cloning from a ref
> namespace
>
> Signed-off-by: Johannes Löthberg <johannes@kyriasis.com>
> ---
>  t/t9904-clone-from-ref-namespace.sh | 33 +++++++++++++++++++++++++++++++++

It seems that 5509 already has a few tests for namespaced transfer
in both directions.  Perhaps this new test would fit there better?

Also I think it probably is better to have these as a single patch.

> diff --git a/t/t9904-clone-from-ref-namespace.sh b/t/t9904-clone-from-ref-namespace.sh
> new file mode 100755
> index 0000000..60977f8
> --- /dev/null
> +++ b/t/t9904-clone-from-ref-namespace.sh
> @@ -0,0 +1,33 @@
> +#!/bin/sh
> +#
> +
> +test_description='git clone from ref namespace
> +
> +This test checks that cloning from a ref namespace works'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'clone from ref namespace' '
> +	rm -rf initial bare clone &&
> +	git init initial &&
> +	git init --bare bare &&
> +	(
> +		cd initial &&
> +		echo "commit one" >> file &&

minor style: drop SP between redirection and its target, i.e.

		echo "commit one" >file &&

> +		git add file &&
> +		git commit -m "commit one" &&
> +		git push ../bare master &&

You want to make sure not just "push" does not complain, but that it
left ../bare with the right result, i.e. something along the lines
of

		git -C ../bare symbolic-ref HEAD >actual &&
		echo refs/heads/master >expect &&
                test_cmp expect actual &&

		git -C ../bare rev-parse HEAD >actual &&
                git rev-parse HEAD >expect &&
                test_cmp expect actual &&


> +		echo "commit two" >> file &&

Likewise on style.

> +		git add file &&
> +		git commit -m "commit two"

Broken &&-chain.

> +		GIT_NAMESPACE=new_namespace git push ../bare master

Likewise on checking the result of the push.

> +	) &&
> +	GIT_NAMESPACE=new_namespace git clone bare clone &&
> +	(
> +		cd clone &&
> +		git show

Likewise on checking the result of the clone; not just it has HEAD
to cause "show" to succeed, you would want it shows the right commit
(i.e. not "one", but "two").  There may be other things you may want
to check, too.

> +	)
> +'
> +
> +test_done

Re: [PATCH v2 2/2] t: Add test for cloning from ref namespace

[Johannes Löthberg]

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

On 05/06, Junio C Hamano wrote:
>Johannes Löthberg <johannes@kyriasis.com> writes:
>>  +++++++++++++++++++++++++++++++++
>
>It seems that 5509 already has a few tests for namespaced transfer
>in both directions.  Perhaps this new test would fit there better?
>

Missed that, will move it there.

>Also I think it probably is better to have these as a single patch.
>

As you wish.

>> +		git add file &&
>> +		git commit -m "commit one" &&
>> +		git push ../bare master &&
>
>You want to make sure not just "push" does not complain, but that it
>left ../bare with the right result, i.e. something along the lines
>of
>
>		git -C ../bare symbolic-ref HEAD >actual &&
>		echo refs/heads/master >expect &&
>                test_cmp expect actual &&
>
>		git -C ../bare rev-parse HEAD >actual &&
>                git rev-parse HEAD >expect &&
>                test_cmp expect actual &&
>
>

Hmm, it seems that git-rev-parse doesn't handle GIT_NAMESPACE yet, so 
can't check it for the namespaced push right now. Not sure if I can fix 
that myself though.

>> +	) &&
>> +	GIT_NAMESPACE=new_namespace git clone bare clone &&
>> +	(
>> +		cd clone &&
>> +		git show
>
>Likewise on checking the result of the clone; not just it has HEAD
>to cause "show" to succeed, you would want it shows the right commit
>(i.e. not "one", but "two").  There may be other things you may want
>to check, too.
>

-- 
Sincerely,
  Johannes Löthberg
  PGP Key ID: 0x50FB9B273A9D0BB5
  https://theos.kyriasis.com/~kyrias/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 1495 bytes --]

Re: [PATCH v2 2/2] t: Add test for cloning from ref namespace

[Junio C Hamano]

Johannes Löthberg <johannes@kyriasis.com> writes:

> Hmm, it seems that git-rev-parse doesn't handle GIT_NAMESPACE yet, so
> can't check it for the namespaced push right now. Not sure if I can
> fix that myself though.

I do not see a need for rev-parse to pay attention to GIT_NAMESPACE
at all, though.

The destination that accepts the push with the enviornment variable,
i.e. your ../bare repository after this:

+		git commit -m "commit two"
+		GIT_NAMESPACE=new_namespace git push ../bare master

must be saving the result somewhere in ../bare/, and that is what
you want to check (and also no refs are affected outside that
hierarchy).

So perhaps along the lines of

        echo $(git rev-parse master) commit \
        	refs/namespaces/new_namespace/refs/heads/master >expect &&
	git -C ../bare for-each-ref refs/namespaces/ >actual &&
	test_cmp expect actual

or something?  You would want to also check that other refs are not
molested, so

	(
        	echo $(git rev-parse master^) commit \
                	refs/heads/master &&
	        echo $(git rev-parse master) commit \
	        	refs/namespaces/new_namespace/refs/heads/master
	) >expect &&
	git -C ../bare for-each-ref >actual &&
	test_cmp expect actual

might be a more appropriate test.

Re: [PATCH v2 2/2] t: Add test for cloning from ref namespace

[Johannes Löthberg]

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

On 05/06, Junio C Hamano wrote:
>Johannes Löthberg <johannes@kyriasis.com> writes:
>
>> Hmm, it seems that git-rev-parse doesn't handle GIT_NAMESPACE yet, so
>> can't check it for the namespaced push right now. Not sure if I can
>> fix that myself though.
>
>I do not see a need for rev-parse to pay attention to GIT_NAMESPACE
>at all, though.
>

The ref namespace has its own HEAD, so I'd expect

  GIT_NAMESPACE=foo git rev-parse HEAD

to act sensibly

>The destination that accepts the push with the enviornment variable,
>i.e. your ../bare repository after this:
>
>+		git commit -m "commit two"
>+		GIT_NAMESPACE=new_namespace git push ../bare master
>
>must be saving the result somewhere in ../bare/, and that is what
>you want to check (and also no refs are affected outside that
>hierarchy).
>
>So perhaps along the lines of
>
>        echo $(git rev-parse master) commit \
>        	refs/namespaces/new_namespace/refs/heads/master >expect &&
>	git -C ../bare for-each-ref refs/namespaces/ >actual &&
>	test_cmp expect actual
>
>or something?  You would want to also check that other refs are not
>molested, so
>
>	(
>        	echo $(git rev-parse master^) commit \
>                	refs/heads/master &&
>	        echo $(git rev-parse master) commit \
>	        	refs/namespaces/new_namespace/refs/heads/master
>	) >expect &&
>	git -C ../bare for-each-ref >actual &&
>	test_cmp expect actual
>
>might be a more appropriate test.

Sounds okay.

-- 
Sincerely,
  Johannes Löthberg
  PGP Key ID: 0x50FB9B273A9D0BB5
  https://theos.kyriasis.com/~kyrias/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 1495 bytes --]

Re: [PATCH v2 2/2] t: Add test for cloning from ref namespace

[Johannes Löthberg]

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

On 05/06, Johannes Löthberg wrote:
>On 05/06, Junio C Hamano wrote:
>>Johannes Löthberg <johannes@kyriasis.com> writes:
>>		git -C ../bare symbolic-ref HEAD >actual &&
>>		echo refs/heads/master >expect &&
>>               test_cmp expect actual &&
>>
>>		git -C ../bare rev-parse HEAD >actual &&
>>               git rev-parse HEAD >expect &&
>>               test_cmp expect actual &&
>>
>>
>
>Hmm, it seems that git-rev-parse doesn't handle GIT_NAMESPACE yet, so 
>can't check it for the namespaced push right now. Not sure if I can 
>fix that myself though.
>

Would it be acceptable to check against 
../bare/refs/namespaces/new_namespace/HEAD and 
../bare/refs/namespaces/new_namespace/refs/heads/master instead, until 
rev-parse is thaught about namespaces?

-- 
Sincerely,
  Johannes Löthberg
  PGP Key ID: 0x50FB9B273A9D0BB5
  https://theos.kyriasis.com/~kyrias/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 1495 bytes --]

Re: [PATCH v2 2/2] t: Add test for cloning from ref namespace

[Junio C Hamano]

Johannes Löthberg <johannes@kyriasis.com> writes:

> Would it be acceptable to check against
> ../bare/refs/namespaces/new_namespace/HEAD and
> ../bare/refs/namespaces/new_namespace/refs/heads/master instead, until
> rev-parse is thaught about namespaces?

Yes.

Because I do not immediately see any legitimate reason for the rest
of the system (including rev-parse) to ever start paying attention
to GIT_NAMESPACE, I think that is not even "instead, until" but is
the right solution.

Thanks.

[PATCH v3] receive-pack: Create a HEAD ref for ref namespace

[Johannes Löthberg]

Each ref namespace have their own separate branches, tags, and HEAD, so
when pushing to a namespace we need to make sure that there exists a
HEAD ref for the namespace, otherwise you will not be able to check out
the repo after cloning from a namespace

Signed-off-by: Johannes Löthberg <johannes@kyriasis.com>
---
since v2:
  * Added test case in t5509
  * Check that the remote refs get set properly in the test

 builtin/receive-pack.c           | 12 +++++++++-
 t/t5509-fetch-push-namespaces.sh | 49 +++++++++++++++++++++++++++++++++++++++-
 2 files changed, 59 insertions(+), 2 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index d2ec52b..0c18c92 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -864,7 +864,9 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 {
 	const char *name = cmd->ref_name;
 	struct strbuf namespaced_name_buf = STRBUF_INIT;
-	const char *namespaced_name, *ret;
+	struct strbuf namespaced_head_buf = STRBUF_INIT;
+	const char *namespaced_name, *ret, *namespace;
+	const char *namespaced_head_path;
 	unsigned char *old_sha1 = cmd->old_sha1;
 	unsigned char *new_sha1 = cmd->new_sha1;
 
@@ -981,6 +983,14 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 		return NULL; /* good */
 	}
 	else {
+		namespace = get_git_namespace();
+		if (strcmp(namespace, "refs/namespaces/")) {
+			strbuf_addf(&namespaced_head_buf, "%s%s", namespace, "HEAD");
+			namespaced_head_path = strbuf_detach(&namespaced_head_buf, NULL);
+
+			create_symref(namespaced_head_path, namespaced_name, NULL);
+		}
+
 		struct strbuf err = STRBUF_INIT;
 		if (shallow_update && si->shallow_ref[cmd->index] &&
 		    update_shallow_ref(cmd, si))
diff --git a/t/t5509-fetch-push-namespaces.sh b/t/t5509-fetch-push-namespaces.sh
index cc0b31f..7bc3a1f 100755
--- a/t/t5509-fetch-push-namespaces.sh
+++ b/t/t5509-fetch-push-namespaces.sh
@@ -1,6 +1,7 @@
 #!/bin/sh
 
-test_description='fetch/push involving ref namespaces'
+test_description='fetch/push/clone involving ref namespaces'
+
 . ./test-lib.sh
 
 test_expect_success setup '
@@ -82,4 +83,50 @@ test_expect_success 'mirroring a repository using a ref namespace' '
 	)
 '
 
+test_expect_success 'cloning from ref namespace' '
+	rm -rf initial bare clone &&
+	git init initial &&
+	git init --bare bare &&
+	(
+		cd initial &&
+		echo "commit one" >file &&
+		git add file &&
+		git commit -m "commit one" &&
+		git push ../bare master &&
+
+		echo refs/heads/master >expect &&
+		git -C ../bare symbolic-ref HEAD >actual &&
+		test_cmp expect actual &&
+
+		git rev-parse HEAD >expect &&
+		git -C ../bare rev-parse HEAD >actual &&
+		test_cmp expect actual &&
+
+		echo "commit two" >>file &&
+		git add file &&
+		git commit -m "commit two" &&
+		GIT_NAMESPACE=new_namespace git push ../bare master &&
+
+		echo "ref: refs/namespaces/new_namespace/refs/heads/master" >expect &&
+		test_cmp expect ../bare/refs/namespaces/new_namespace/HEAD  &&
+
+		(
+			printf "%s commit\t%s\n" $(git rev-parse master^) \
+			                         refs/heads/master &&
+			printf "%s commit\t%s\n" $(git rev-parse master) \
+			                         refs/namespaces/new_namespace/HEAD &&
+			printf "%s commit\t%s\n" $(git rev-parse master) \
+			                         refs/namespaces/new_namespace/refs/heads/master
+		) >expect &&
+		git -C ../bare for-each-ref refs/ >actual &&
+		test_cmp expect actual
+	) &&
+	GIT_NAMESPACE=new_namespace git clone bare clone &&
+	(
+		cd clone &&
+		git show
+	)
+'
+
+
 test_done
-- 
2.4.2

Re: [PATCH v3] receive-pack: Create a HEAD ref for ref namespace

[Johannes Löthberg]

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

I should also look into why the other tests in t5509 fail later.

-- 
Sincerely,
  Johannes Löthberg
  PGP Key ID: 0x50FB9B273A9D0BB5
  https://theos.kyriasis.com/~kyrias/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 1495 bytes --]

Re: [PATCH v3] receive-pack: Create a HEAD ref for ref namespace

[Junio C Hamano]

Johannes Löthberg <johannes@kyriasis.com> writes:

> diff --git a/t/t5509-fetch-push-namespaces.sh b/t/t5509-fetch-push-namespaces.sh
> index cc0b31f..7bc3a1f 100755
> --- a/t/t5509-fetch-push-namespaces.sh
> +++ b/t/t5509-fetch-push-namespaces.sh
> @@ -1,6 +1,7 @@
>  #!/bin/sh
>  
> -test_description='fetch/push involving ref namespaces'
> +test_description='fetch/push/clone involving ref namespaces'
> +

OK ;-)

>  . ./test-lib.sh
>  
>  test_expect_success setup '
> @@ -82,4 +83,50 @@ test_expect_success 'mirroring a repository using a ref namespace' '
>  	)
>  '
>  
> +test_expect_success 'cloning from ref namespace' '
> +	rm -rf initial bare clone &&
> +	git init initial &&
> +	git init --bare bare &&
> +	(
> +		cd initial &&
> +		echo "commit one" >file &&
> +		git add file &&
> +		git commit -m "commit one" &&
> +		git push ../bare master &&
> +
> +		echo refs/heads/master >expect &&
> +		git -C ../bare symbolic-ref HEAD >actual &&
> +		test_cmp expect actual &&
> +
> +		git rev-parse HEAD >expect &&
> +		git -C ../bare rev-parse HEAD >actual &&
> +		test_cmp expect actual &&
> +
> +		echo "commit two" >>file &&
> +		git add file &&
> +		git commit -m "commit two" &&
> +		GIT_NAMESPACE=new_namespace git push ../bare master &&
> +
> +		echo "ref: refs/namespaces/new_namespace/refs/heads/master" >expect &&
> +		test_cmp expect ../bare/refs/namespaces/new_namespace/HEAD  &&

Use "symbolic-ref refs/namespaces/new_namespace/HEAD"; HEAD is not
required to be expressed as a textual symref.

> +
> +		(
> +			printf "%s commit\t%s\n" $(git rev-parse master^) \
> +			                         refs/heads/master &&
> +			printf "%s commit\t%s\n" $(git rev-parse master) \
> +			                         refs/namespaces/new_namespace/HEAD &&
> +			printf "%s commit\t%s\n" $(git rev-parse master) \
> +			                         refs/namespaces/new_namespace/refs/heads/master
> +		) >expect &&

Use of 'printf' is clever and I like it.  Have you considered
letting it do the iteration as well?  I.e.

	printf "%s commit\t%s\n" \
        	one two \
                three four \
                five six \
	>expect &&

might be easier to read.

> +		git -C ../bare for-each-ref refs/ >actual &&
> +		test_cmp expect actual
> +	) &&
> +	GIT_NAMESPACE=new_namespace git clone bare clone &&
> +	(
> +		cd clone &&
> +		git show

We can accept any random commit at HEAD as long as it exists at this
point?  Don't we need to make sure that a ref whose tip is still "one"
is not propagated to this new clone?

> +	)
> +'
> +
> +
>  test_done

Re: [PATCH v3] receive-pack: Create a HEAD ref for ref namespace

[Johannes Löthberg]

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

On 05/06, Junio C Hamano wrote:
>Johannes Löthberg <johannes@kyriasis.com> writes:
>
>> +
>> +		echo "ref: refs/namespaces/new_namespace/refs/heads/master" >expect &&
>> +		test_cmp expect ../bare/refs/namespaces/new_namespace/HEAD  &&
>
>Use "symbolic-ref refs/namespaces/new_namespace/HEAD"; HEAD is not
>required to be expressed as a textual symref.
>

Gotcha.

>> +
>> +		(
>> +			printf "%s commit\t%s\n" $(git rev-parse master^) \
>> +			                         refs/heads/master &&
>> +			printf "%s commit\t%s\n" $(git rev-parse master) \
>> +			                         refs/namespaces/new_namespace/HEAD &&
>> +			printf "%s commit\t%s\n" $(git rev-parse master) \
>> +			                         refs/namespaces/new_namespace/refs/heads/master
>> +		) >expect &&
>
>Use of 'printf' is clever and I like it.  Have you considered
>letting it do the iteration as well?  I.e.
>
>	printf "%s commit\t%s\n" \
>        	one two \
>                three four \
>                five six \
>	>expect &&
>
>might be easier to read.
>

Didn't think about that actually. Will do.

>> +		git -C ../bare for-each-ref refs/ >actual &&
>> +		test_cmp expect actual
>> +	) &&
>> +	GIT_NAMESPACE=new_namespace git clone bare clone &&
>> +	(
>> +		cd clone &&
>> +		git show
>
>We can accept any random commit at HEAD as long as it exists at this
>point?  Don't we need to make sure that a ref whose tip is still "one"
>is not propagated to this new clone?
>

Oh crap, I just remembered that I forgot to address that part, sorry.

>> +	)
>> +'
>> +
>> +
>>  test_done

-- 
Sincerely,
  Johannes Löthberg
  PGP Key ID: 0x50FB9B273A9D0BB5
  https://theos.kyriasis.com/~kyrias/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 1495 bytes --]

[PATCH v4] receive-pack: Create a HEAD ref for ref namespace

[Johannes Löthberg]

Each ref namespace have their own separate branches, tags, and HEAD, so
when pushing to a namespace we need to make sure that there exists a
HEAD ref for the namespace, otherwise you will not be able to check out
the repo after cloning from a namespace

Signed-off-by: Johannes Löthberg <johannes@kyriasis.com>
---
Changes since v3:
  test:
    * Use a single printf statement
    * Check that the contents of the file and sha of the commits in the
      initial and cloned repositories matches

 builtin/receive-pack.c           | 12 +++++++++-
 t/t5509-fetch-push-namespaces.sh | 50 +++++++++++++++++++++++++++++++++++++++-
 2 files changed, 60 insertions(+), 2 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index d2ec52b..0c18c92 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -864,7 +864,9 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 {
 	const char *name = cmd->ref_name;
 	struct strbuf namespaced_name_buf = STRBUF_INIT;
-	const char *namespaced_name, *ret;
+	struct strbuf namespaced_head_buf = STRBUF_INIT;
+	const char *namespaced_name, *ret, *namespace;
+	const char *namespaced_head_path;
 	unsigned char *old_sha1 = cmd->old_sha1;
 	unsigned char *new_sha1 = cmd->new_sha1;
 
@@ -981,6 +983,14 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 		return NULL; /* good */
 	}
 	else {
+		namespace = get_git_namespace();
+		if (strcmp(namespace, "refs/namespaces/")) {
+			strbuf_addf(&namespaced_head_buf, "%s%s", namespace, "HEAD");
+			namespaced_head_path = strbuf_detach(&namespaced_head_buf, NULL);
+
+			create_symref(namespaced_head_path, namespaced_name, NULL);
+		}
+
 		struct strbuf err = STRBUF_INIT;
 		if (shallow_update && si->shallow_ref[cmd->index] &&
 		    update_shallow_ref(cmd, si))
diff --git a/t/t5509-fetch-push-namespaces.sh b/t/t5509-fetch-push-namespaces.sh
index cc0b31f..88c8aa9 100755
--- a/t/t5509-fetch-push-namespaces.sh
+++ b/t/t5509-fetch-push-namespaces.sh
@@ -1,6 +1,7 @@
 #!/bin/sh
 
-test_description='fetch/push involving ref namespaces'
+test_description='fetch/push/clone involving ref namespaces'
+
 . ./test-lib.sh
 
 test_expect_success setup '
@@ -82,4 +83,51 @@ test_expect_success 'mirroring a repository using a ref namespace' '
 	)
 '
 
+test_expect_success 'cloning from ref namespace' '
+	rm -rf initial bare clone &&
+	git init initial &&
+	git init --bare bare &&
+	(
+		cd initial &&
+		echo "commit one" >file &&
+		git add file &&
+		git commit -m "commit one" &&
+		git push ../bare master &&
+
+		echo refs/heads/master >expect &&
+		git -C ../bare symbolic-ref HEAD >actual &&
+		test_cmp expect actual &&
+
+		git rev-parse HEAD >expect &&
+		git -C ../bare rev-parse HEAD >actual &&
+		test_cmp expect actual &&
+
+		echo "commit two" >>file &&
+		git add file &&
+		git commit -m "commit two" &&
+		GIT_NAMESPACE=new_namespace git push ../bare master &&
+
+		echo "ref: refs/namespaces/new_namespace/refs/heads/master" >expect &&
+		test_cmp expect ../bare/refs/namespaces/new_namespace/HEAD &&
+
+		printf "%s commit\t%s\n" \
+		    $(git rev-parse master^) refs/heads/master \
+		    $(git rev-parse master) refs/namespaces/new_namespace/HEAD \
+		    $(git rev-parse master) refs/namespaces/new_namespace/refs/heads/master >expect &&
+		git -C ../bare for-each-ref refs/ >actual &&
+		test_cmp expect actual
+	) &&
+	GIT_NAMESPACE=new_namespace git clone bare clone &&
+	(
+		git -C initial cat-file blob master:file >expect &&
+		git -C clone cat-file blob master:file >actual &&
+		test_cmp expect actual &&
+
+		git -C initial rev-parse master >expect &&
+		git -C clone rev-parse master >actual &&
+		test_cmp expect actual
+	)
+'
+
+
 test_done
-- 
2.4.2

Re: [PATCH v4] receive-pack: Create a HEAD ref for ref namespace

[Johannes Löthberg]

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

On 05/06, Johannes Löthberg wrote:
>Each ref namespace have their own separate branches, tags, and HEAD, so
>when pushing to a namespace we need to make sure that there exists a
>HEAD ref for the namespace, otherwise you will not be able to check out
>the repo after cloning from a namespace
>
>Signed-off-by: Johannes Löthberg <johannes@kyriasis.com>
>---
>Changes since v3:
>  test:
>    * Use a single printf statement
>    * Check that the contents of the file and sha of the commits in the
>      initial and cloned repositories matches
>

Any other comments?

-- 
Sincerely,
  Johannes Löthberg
  PGP Key ID: 0x50FB9B273A9D0BB5
  https://theos.kyriasis.com/~kyrias/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 1495 bytes --]

Re: [PATCH v4] receive-pack: Create a HEAD ref for ref namespace

[Junio C Hamano]

Johannes Löthberg <johannes@kyriasis.com> writes:

> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index d2ec52b..0c18c92 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -864,7 +864,9 @@ static const char *update(struct command *cmd, struct shallow_info *si)
>  {
>  	const char *name = cmd->ref_name;
>  	struct strbuf namespaced_name_buf = STRBUF_INIT;
> -	const char *namespaced_name, *ret;
> +	struct strbuf namespaced_head_buf = STRBUF_INIT;
> +	const char *namespaced_name, *ret, *namespace;
> +	const char *namespaced_head_path;
>  	unsigned char *old_sha1 = cmd->old_sha1;
>  	unsigned char *new_sha1 = cmd->new_sha1;
>  
> @@ -981,6 +983,14 @@ static const char *update(struct command *cmd, struct shallow_info *si)
>  		return NULL; /* good */
>  	}
>  	else {
> +		namespace = get_git_namespace();
> +		if (strcmp(namespace, "refs/namespaces/")) {
> +			strbuf_addf(&namespaced_head_buf, "%s%s", namespace, "HEAD");
> +			namespaced_head_path = strbuf_detach(&namespaced_head_buf, NULL);
> +
> +			create_symref(namespaced_head_path, namespaced_name, NULL);

In a normal environment without any namespace, get_git_namespace()
will return an empty string, which is not "refs/namespaces/", so we
create a symref HEAD (that is .git/HEAD) that points at whatever
name the command is about.  And this is done every time any ref is
updated, flipping the HEAD to point at whatever was pushed the last,
isn't it?

Why is this a good change?  I am puzzled...

> +		}
> +
>  		struct strbuf err = STRBUF_INIT;

This adds decl-after-stmt.

Going back to the proposed log message...

> Each ref namespace have their own separate branches, tags, and HEAD, so
> when pushing to a namespace we need to make sure that there exists a
> HEAD ref for the namespace, otherwise you will not be able to check out
> the repo after cloning from a namespace

What this paragraph describes is entirely correct, I think.  But I
do not think receive-pack is the "we" in that paragraph.

When setting up a "namespace" a and b, shouldn't you be doing
essentially

	r=refs/namespaces/
	for ns in a b
        do
		git symbolic-ref $r$ns/HEAD $r$ns/refs/heads/master
	done

or something, which is an equivalent to what "git init" does to a
normal repository?

Re: [PATCH v4] receive-pack: Create a HEAD ref for ref namespace

[Johannes Löthberg]

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

On 15/06, Junio C Hamano wrote:
>Johannes Löthberg <johannes@kyriasis.com> writes:
>>  	else {
>> +		namespace = get_git_namespace();
>> +		if (strcmp(namespace, "refs/namespaces/")) {
>> +			strbuf_addf(&namespaced_head_buf, "%s%s", namespace, "HEAD");
>> +			namespaced_head_path = strbuf_detach(&namespaced_head_buf, NULL);
>> +
>> +			create_symref(namespaced_head_path, namespaced_name, NULL);
>
>In a normal environment without any namespace, get_git_namespace()
>will return an empty string, which is not "refs/namespaces/", so we
>create a symref HEAD (that is .git/HEAD) that points at whatever
>name the command is about.  And this is done every time any ref is
>updated, flipping the HEAD to point at whatever was pushed the last,
>isn't it?
>
>Why is this a good change?  I am puzzled...
>

This creates a HEAD symref in the namespace itself, since there's no 
other place that creates it. It could probably be done better, but I'm 
not very familiar with the Git codebase.

>> +		}
>> +
>>  		struct strbuf err = STRBUF_INIT;
>
>This adds decl-after-stmt.
>
>Going back to the proposed log message...
>
>> Each ref namespace have their own separate branches, tags, and HEAD, so
>> when pushing to a namespace we need to make sure that there exists a
>> HEAD ref for the namespace, otherwise you will not be able to check out
>> the repo after cloning from a namespace
>
>What this paragraph describes is entirely correct, I think.  But I
>do not think receive-pack is the "we" in that paragraph.
>
>When setting up a "namespace" a and b, shouldn't you be doing
>essentially
>
>	r=refs/namespaces/
>	for ns in a b
>        do
>		git symbolic-ref $r$ns/HEAD $r$ns/refs/heads/master
>	done
>
>or something, which is an equivalent to what "git init" does to a
>normal repository?
>

The only way to set up a namespace is by pushing to it.

-- 
Sincerely,
  Johannes Löthberg
  PGP Key ID: 0x50FB9B273A9D0BB5
  https://theos.kyriasis.com/~kyrias/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 1495 bytes --]