threads / patch / 39488

patchreceive-pack: Create a HEAD ref for ref namespace

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

## tl;dr

22 messages between Jun 1, 2015 and Jun 15, 2015. Diffs are folded; open one to read it.

replies: 21people: 3as markdown or json

Johannes Löthberg· Jun 1, 2015, 21:24 UTC · lore

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

Show changes to builtin/receive-pack.c +11 −1

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

Johannes Löthberg· Jun 5, 2015, 11:53 UTC · re: Johannes Löthberg · lore

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

Ping.

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

Michael J Gruber· Jun 5, 2015, 12:55 UTC · re: Johannes Löthberg · lore

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

Johannes Löthberg venit, vidit, dixit 05.06.2015 13:53:

Show 7 quoted lines

> 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

Johannes Löthberg· Jun 5, 2015, 13:50 UTC · re: Michael J Gruber · lore

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

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/

Johannes Löthberg· Jun 5, 2015, 14:10 UTC · re: Michael J Gruber · lore

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

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/

Johannes Löthberg· Jun 5, 2015, 14:12 UTC · re: Johannes Löthberg · lore

[PATCH v2] Fix cloning from ref namespace

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

Johannes Löthberg· Jun 5, 2015, 14:12 UTC · re: Johannes Löthberg · lore

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

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

Show changes to builtin/receive-pack.c +11 −1

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

Johannes Löthberg· Jun 5, 2015, 14:12 UTC · re: Johannes Löthberg · lore

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

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

Show changes to t/t9904-clone-from-ref-namespace.sh +33 −0

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

Junio C Hamano· Jun 5, 2015, 15:33 UTC · re: Johannes Löthberg · lore

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

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

Show 6 quoted lines

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

Show 22 quoted lines

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

Show 5 quoted lines

> +	) &&
> +	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

Johannes Löthberg· Jun 5, 2015, 16:12 UTC · re: Junio C Hamano · lore

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

On 05/06, Junio C Hamano wrote:

Show 6 quoted lines

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

Show 17 quoted lines

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

Show 11 quoted lines

>> +	) &&
>> +	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/

Junio C Hamano· Jun 5, 2015, 16:22 UTC · re: Johannes Löthberg · lore

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

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.

Johannes Löthberg· Jun 5, 2015, 16:31 UTC · re: Junio C Hamano · lore

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

On 05/06, Junio C Hamano wrote:

Show 9 quoted lines

>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

Show 30 quoted lines

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

Johannes Löthberg· Jun 5, 2015, 16:25 UTC · re: Johannes Löthberg · lore

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

On 05/06, Johannes Löthberg wrote:

Show 16 quoted lines

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

Junio C Hamano· Jun 5, 2015, 16:46 UTC · re: Johannes Löthberg · lore

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

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.

Johannes Löthberg· Jun 5, 2015, 17:02 UTC · re: Johannes Löthberg · lore

[PATCH v3] receive-pack: Create a HEAD ref for ref 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(-)

Show changes to 2 files +59 −2

builtin/receive-pack.c, t/t5509-fetch-push-namespaces.sh

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

Johannes Löthberg· Jun 5, 2015, 17:08 UTC · re: Johannes Löthberg · lore

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

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/

Junio C Hamano· Jun 5, 2015, 17:19 UTC · re: Johannes Löthberg · lore

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

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

Show 10 quoted lines

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

Show 33 quoted lines

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

Show 9 quoted lines

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

Show 7 quoted lines

> +		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?

Show 5 quoted lines

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

Johannes Löthberg· Jun 5, 2015, 17:27 UTC · re: Junio C Hamano · lore

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

On 05/06, Junio C Hamano wrote:

Show 9 quoted lines

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

Show 21 quoted lines

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

Show 12 quoted lines

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

Show 5 quoted lines

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

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

Johannes Löthberg· Jun 5, 2015, 17:42 UTC · re: Johannes Löthberg · lore

[PATCH v4] receive-pack: Create a HEAD ref for ref 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(-)

Show changes to 2 files +60 −2

builtin/receive-pack.c, t/t5509-fetch-push-namespaces.sh

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

Johannes Löthberg· Jun 10, 2015, 23:39 UTC · re: Johannes Löthberg · lore

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

On 05/06, Johannes Löthberg wrote:

Show 13 quoted lines

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

Junio C Hamano· Jun 15, 2015, 20:48 UTC · re: Johannes Löthberg · lore

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

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

Show 25 quoted lines

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

Johannes Löthberg· Jun 15, 2015, 20:59 UTC · re: Junio C Hamano · lore

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

On 15/06, Junio C Hamano wrote:

Show 18 quoted lines

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

Show 28 quoted lines

>> +		}
>> +
>>  		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/