[PATCH v2 0/4] Improve hideRefs when used with namespaces

[PATCH v2 0/4] Improve hideRefs when used with namespaces

[Lukas Fleischer]

This is a first set of patches improving documentation and behavior of
the transfer.hideRefs feature as discussed in [1]. In particular,
hideRefs is changed to generally match stripped refs by default and
match full refs when prefixed with "^". The documentation is updated
accordingly. Basic tests are added.

Changes since v1:

* Improvements in the code for denying pushing hidden refs.
* Simplification of the tests as suggested by Eric.
* New tests for the code for denying pushing hidden refs.
* Comments.

[1] http://marc.info/?l=git&m=144604694223920

Lukas Fleischer (4):
  Document the semantics of hideRefs with namespaces
  upload-pack: strip refs before calling ref_is_hidden()
  Add support for matching full refs in hideRefs
  t5509: add basic tests for hideRefs

 Documentation/config.txt         |  8 ++++++++
 builtin/receive-pack.c           | 27 ++++++++++++++++++++------
 refs.c                           | 15 ++++++++++++---
 refs.h                           | 10 +++++++++-
 t/t5509-fetch-push-namespaces.sh | 41 ++++++++++++++++++++++++++++++++++++++++
 upload-pack.c                    | 13 ++++++++-----
 6 files changed, 99 insertions(+), 15 deletions(-)

-- 
2.6.2

[PATCH v2 1/4] Document the semantics of hideRefs with namespaces

[Lukas Fleischer]

Right now, there is no clear definition of how transfer.hideRefs should
behave when a namespace is set. Explain that hideRefs prefixes match
stripped names in that case. This is how hideRefs patterns are currently
handled in receive-pack.

Signed-off-by: Lukas Fleischer <lfleischer@lfos.de>
---
 Documentation/config.txt | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1204072..3da97a1 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2684,6 +2684,13 @@ You may also include a `!` in front of the ref name to negate the entry,
 explicitly exposing it, even if an earlier entry marked it as hidden.
 If you have multiple hideRefs values, later entries override earlier ones
 (and entries in more-specific config files override less-specific ones).
++
+If a namespace is set, references are stripped before matching. For example, if
+the prefix `refs/heads/master` is specified in `transfer.hideRefs` and the
+current namespace is `foo`, then `refs/namespaces/foo/refs/heads/master` is
+omitted from the advertisements but `refs/heads/master` and
+`refs/namespaces/bar/refs/heads/master` are still advertised as so-called
+"have" lines.
 
 transfer.unpackLimit::
 	When `fetch.unpackLimit` or `receive.unpackLimit` are
-- 
2.6.2

[PATCH v2 2/4] upload-pack: strip refs before calling ref_is_hidden()

[Lukas Fleischer]

Make hideRefs handling in upload-pack consistent with the behavior
described in the documentation by stripping refs before comparing them
with prefixes in hideRefs.

Signed-off-by: Lukas Fleischer <lfleischer@lfos.de>
---
 upload-pack.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/upload-pack.c b/upload-pack.c
index d0bc3ca..4ca960e 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -692,7 +692,7 @@ static int mark_our_ref(const char *refname, const struct object_id *oid)
 {
 	struct object *o = lookup_unknown_object(oid->hash);
 
-	if (ref_is_hidden(refname)) {
+	if (refname && ref_is_hidden(refname)) {
 		o->flags |= HIDDEN_REF;
 		return 1;
 	}
@@ -703,7 +703,7 @@ static int mark_our_ref(const char *refname, const struct object_id *oid)
 static int check_ref(const char *refname, const struct object_id *oid,
 		     int flag, void *cb_data)
 {
-	mark_our_ref(refname, oid);
+	mark_our_ref(strip_namespace(refname), oid);
 	return 0;
 }
 
@@ -726,7 +726,7 @@ static int send_ref(const char *refname, const struct object_id *oid,
 	const char *refname_nons = strip_namespace(refname);
 	struct object_id peeled;
 
-	if (mark_our_ref(refname, oid))
+	if (mark_our_ref(refname_nons, oid))
 		return 0;
 
 	if (capabilities) {
-- 
2.6.2

[PATCH v2 3/4] Add support for matching full refs in hideRefs

[Lukas Fleischer]

In addition to matching stripped refs, one can now add hideRefs patterns
that the full (unstripped) ref is matched against. To distinguish
between stripped and full matches, those new patterns must be prefixed
with a circumflex (^).

This commit also removes support for the undocumented and unintended
hideRefs settings "have" (suppressing all "have" lines) and
"capabilities^{}" (suppressing the capabilities line).

Signed-off-by: Lukas Fleischer <lfleischer@lfos.de>
---
 Documentation/config.txt |  3 ++-
 builtin/receive-pack.c   | 27 +++++++++++++++++++++------
 refs.c                   | 15 ++++++++++++---
 refs.h                   | 10 +++++++++-
 upload-pack.c            | 13 ++++++++-----
 5 files changed, 52 insertions(+), 16 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 3da97a1..91ed6a5 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2690,7 +2690,8 @@ the prefix `refs/heads/master` is specified in `transfer.hideRefs` and the
 current namespace is `foo`, then `refs/namespaces/foo/refs/heads/master` is
 omitted from the advertisements but `refs/heads/master` and
 `refs/namespaces/bar/refs/heads/master` are still advertised as so-called
-"have" lines.
+"have" lines. In order to match refs before stripping, add a `^` in front of
+the ref name. If you combine `!` and `^`, `!` must be specified first.
 
 transfer.unpackLimit::
 	When `fetch.unpackLimit` or `receive.unpackLimit` are
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index bcb624b..9939de1 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -195,9 +195,6 @@ static int receive_pack_config(const char *var, const char *value, void *cb)
 
 static void show_ref(const char *path, const unsigned char *sha1)
 {
-	if (ref_is_hidden(path))
-		return;
-
 	if (sent_capabilities) {
 		packet_write(1, "%s %s\n", sha1_to_hex(sha1), path);
 	} else {
@@ -219,9 +216,14 @@ static void show_ref(const char *path, const unsigned char *sha1)
 	}
 }
 
-static int show_ref_cb(const char *path, const struct object_id *oid, int flag, void *unused)
+static int show_ref_cb(const char *path_full, const struct object_id *oid,
+		       int flag, void *unused)
 {
-	path = strip_namespace(path);
+	const char *path = strip_namespace(path_full);
+
+	if (ref_is_hidden(path, path_full))
+		return 1;
+
 	/*
 	 * Advertise refs outside our current namespace as ".have"
 	 * refs, so that the client can use them to minimize data
@@ -1195,16 +1197,29 @@ static int iterate_receive_command_list(void *cb_data, unsigned char sha1[20])
 
 static void reject_updates_to_hidden(struct command *commands)
 {
+	struct strbuf refname_full = STRBUF_INIT;
+	size_t prefix_len;
 	struct command *cmd;
 
+	strbuf_addstr(&refname_full, get_git_namespace());
+	prefix_len = refname_full.len;
+
 	for (cmd = commands; cmd; cmd = cmd->next) {
-		if (cmd->error_string || !ref_is_hidden(cmd->ref_name))
+		if (cmd->error_string)
+			continue;
+
+		strbuf_setlen(&refname_full, prefix_len);
+		strbuf_addstr(&refname_full, cmd->ref_name);
+
+		if (!ref_is_hidden(cmd->ref_name, refname_full.buf))
 			continue;
 		if (is_null_sha1(cmd->new_sha1))
 			cmd->error_string = "deny deleting a hidden ref";
 		else
 			cmd->error_string = "deny updating a hidden ref";
 	}
+
+	strbuf_release(&refname_full);
 }
 
 static int should_process_cmd(struct command *cmd)
diff --git a/refs.c b/refs.c
index 72d96ed..892fffb 100644
--- a/refs.c
+++ b/refs.c
@@ -321,7 +321,7 @@ int parse_hide_refs_config(const char *var, const char *value, const char *secti
 	return 0;
 }
 
-int ref_is_hidden(const char *refname)
+int ref_is_hidden(const char *refname, const char *refname_full)
 {
 	int i;
 
@@ -329,6 +329,7 @@ int ref_is_hidden(const char *refname)
 		return 0;
 	for (i = hide_refs->nr - 1; i >= 0; i--) {
 		const char *match = hide_refs->items[i].string;
+		const char *subject;
 		int neg = 0;
 		int len;
 
@@ -337,10 +338,18 @@ int ref_is_hidden(const char *refname)
 			match++;
 		}
 
-		if (!starts_with(refname, match))
+		if (*match == '^') {
+			subject = refname_full;
+			match++;
+		} else {
+			subject = refname;
+		}
+
+		/* refname can be NULL when namespaces are used. */
+		if (!subject || !starts_with(subject, match))
 			continue;
 		len = strlen(match);
-		if (!refname[len] || refname[len] == '/')
+		if (!subject[len] || subject[len] == '/')
 			return !neg;
 	}
 	return 0;
diff --git a/refs.h b/refs.h
index 69fa4df..116c461 100644
--- a/refs.h
+++ b/refs.h
@@ -604,7 +604,15 @@ int update_ref(const char *msg, const char *refname,
 
 extern int parse_hide_refs_config(const char *var, const char *value, const char *);
 
-extern int ref_is_hidden(const char *);
+/*
+ * Check whether a ref is hidden. If no namespace is set, both the first and
+ * the second parameter point to the full ref name. If a namespace is set and
+ * the ref is inside that namespace, the first parameter is a pointer to the
+ * name of the ref with the namespace prefix removed. If a namespace is set and
+ * the ref is outside that namespace, the first parameter is NULL. The second
+ * parameter always points to the full ref name.
+ */
+extern int ref_is_hidden(const char *, const char *);
 
 enum ref_type {
 	REF_TYPE_PER_WORKTREE,
diff --git a/upload-pack.c b/upload-pack.c
index 4ca960e..08efb1d 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -688,11 +688,12 @@ static void receive_needs(void)
 }
 
 /* return non-zero if the ref is hidden, otherwise 0 */
-static int mark_our_ref(const char *refname, const struct object_id *oid)
+static int mark_our_ref(const char *refname, const char *refname_full,
+			const struct object_id *oid)
 {
 	struct object *o = lookup_unknown_object(oid->hash);
 
-	if (refname && ref_is_hidden(refname)) {
+	if (ref_is_hidden(refname, refname_full)) {
 		o->flags |= HIDDEN_REF;
 		return 1;
 	}
@@ -700,10 +701,12 @@ static int mark_our_ref(const char *refname, const struct object_id *oid)
 	return 0;
 }
 
-static int check_ref(const char *refname, const struct object_id *oid,
+static int check_ref(const char *refname_full, const struct object_id *oid,
 		     int flag, void *cb_data)
 {
-	mark_our_ref(strip_namespace(refname), oid);
+	const char *refname = strip_namespace(refname_full);
+
+	mark_our_ref(refname, refname_full, oid);
 	return 0;
 }
 
@@ -726,7 +729,7 @@ static int send_ref(const char *refname, const struct object_id *oid,
 	const char *refname_nons = strip_namespace(refname);
 	struct object_id peeled;
 
-	if (mark_our_ref(refname_nons, oid))
+	if (mark_our_ref(refname_nons, refname, oid))
 		return 0;
 
 	if (capabilities) {
-- 
2.6.2

[PATCH v2 4/4] t5509: add basic tests for hideRefs

[Lukas Fleischer]

Test whether regular and full hideRefs patterns work as expected when
namespaces are used.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Lukas Fleischer <lfleischer@lfos.de>
---
 t/t5509-fetch-push-namespaces.sh | 41 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/t/t5509-fetch-push-namespaces.sh b/t/t5509-fetch-push-namespaces.sh
index cc0b31f..bc44ac3 100755
--- a/t/t5509-fetch-push-namespaces.sh
+++ b/t/t5509-fetch-push-namespaces.sh
@@ -82,4 +82,45 @@ test_expect_success 'mirroring a repository using a ref namespace' '
 	)
 '
 
+test_expect_success 'hide namespaced refs with transfer.hideRefs' '
+	GIT_NAMESPACE=namespace \
+		git -C pushee -c transfer.hideRefs=refs/tags \
+		ls-remote "ext::git %s ." >actual &&
+	printf "$commit1\trefs/heads/master\n" >expected &&
+	test_cmp expected actual
+'
+
+test_expect_success 'check that transfer.hideRefs does not match unstripped refs' '
+	GIT_NAMESPACE=namespace \
+		git -C pushee -c transfer.hideRefs=refs/namespaces/namespace/refs/tags \
+		ls-remote "ext::git %s ." >actual &&
+	printf "$commit1\trefs/heads/master\n" >expected &&
+	printf "$commit0\trefs/tags/0\n" >>expected &&
+	printf "$commit1\trefs/tags/1\n" >>expected &&
+	test_cmp expected actual
+'
+
+test_expect_success 'hide full refs with transfer.hideRefs' '
+	GIT_NAMESPACE=namespace \
+		git -C pushee -c transfer.hideRefs="^refs/namespaces/namespace/refs/tags" \
+		ls-remote "ext::git %s ." >actual &&
+	printf "$commit1\trefs/heads/master\n" >expected &&
+	test_cmp expected actual
+'
+
+test_expect_success 'try to update a hidden ref' '
+	test_config -C pushee transfer.hideRefs refs/heads/master &&
+	test_must_fail git -C original push pushee-namespaced master
+'
+
+test_expect_success 'try to update a ref that is not hidden' '
+	test_config -C pushee transfer.hideRefs refs/namespaces/namespace/refs/heads/master &&
+	git -C original push pushee-namespaced master
+'
+
+test_expect_success 'try to update a hidden full ref' '
+	test_config -C pushee transfer.hideRefs "^refs/namespaces/namespace/refs/heads/master" &&
+	test_must_fail git -C original push pushee-namespaced master
+'
+
 test_done
-- 
2.6.2

Re: [PATCH v2 3/4] Add support for matching full refs in hideRefs

[Junio C Hamano]

Lukas Fleischer <lfleischer@lfos.de> writes:

>  static void show_ref(const char *path, const unsigned char *sha1)
>  {
> -	if (ref_is_hidden(path))
> -		return;
> -
>  	if (sent_capabilities) {
>  		packet_write(1, "%s %s\n", sha1_to_hex(sha1), path);
>  	} else {
> @@ -219,9 +216,14 @@ static void show_ref(const char *path, const unsigned char *sha1)
>  	}
>  }
>  
> -static int show_ref_cb(const char *path, const struct object_id *oid, int flag, void *unused)
> +static int show_ref_cb(const char *path_full, const struct object_id *oid,
> +		       int flag, void *unused)
>  {
> -	path = strip_namespace(path);
> +	const char *path = strip_namespace(path_full);
> +
> +	if (ref_is_hidden(path, path_full))
> +		return 1;
> +

These two hunks are doing a bit more than the primary topic of this
step, aren't they?

The semantics of "hideRefs" used to be that "refs can be hidden from
ls-remote" (i.e. prevents fetching, avoids contaminating "--mirror")
and "refs can be hidden from 'push'", but the objects being known
tips of histories that are complete, they still participate in
common ancestor discovery.  

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 3da97a1..91ed6a5 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2690,7 +2690,8 @@ the prefix `refs/heads/master` is specified in `transfer.hideRefs` and the
>  current namespace is `foo`, then `refs/namespaces/foo/refs/heads/master` is
>  omitted from the advertisements but `refs/heads/master` and
>  `refs/namespaces/bar/refs/heads/master` are still advertised as so-called
> -"have" lines.
> +"have" lines. In order to match refs before stripping, add a `^` in front of
> +the ref name. If you combine `!` and `^`, `!` must be specified first.

I think the changes in the above two hunks prevent the hidden refs
from participating in common ancestor discovery (which is probably a
good thing), making the above description stale.

Re: [PATCH v2 1/4] Document the semantics of hideRefs with namespaces

[Junio C Hamano]

Lukas Fleischer <lfleischer@lfos.de> writes:

> ++
> +If a namespace is set, references are stripped before matching. For example, if

The first sentence is probably not understood by many readers, as
you do not define what is "to strip references".  The namespace
prefix is stripped from the reference before a reference is matched
against the hideRefs setting.

> +the prefix `refs/heads/master` is specified in `transfer.hideRefs` and the
> +current namespace is `foo`, then `refs/namespaces/foo/refs/heads/master` is
> +omitted from the advertisements but `refs/heads/master` and
> +`refs/namespaces/bar/refs/heads/master` are still advertised as so-called
> +"have" lines.

Re: [PATCH v2 3/4] Add support for matching full refs in hideRefs

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> The semantics of "hideRefs" used to be that "refs can be hidden from
> ls-remote" (i.e. prevents fetching, avoids contaminating "--mirror")
> and "refs can be hidden from 'push'", but the objects being known
> tips of histories that are complete, they still participate in
> common ancestor discovery.  
>
>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>> index 3da97a1..91ed6a5 100644
>> --- a/Documentation/config.txt
>> +++ b/Documentation/config.txt
>> @@ -2690,7 +2690,8 @@ the prefix `refs/heads/master` is specified in `transfer.hideRefs` and the
>>  current namespace is `foo`, then `refs/namespaces/foo/refs/heads/master` is
>>  omitted from the advertisements but `refs/heads/master` and
>>  `refs/namespaces/bar/refs/heads/master` are still advertised as so-called
>> -"have" lines.
>> +"have" lines. In order to match refs before stripping, add a `^` in front of
>> +the ref name. If you combine `!` and `^`, `!` must be specified first.
>
> I think the changes in the above two hunks prevent the hidden refs
> from participating in common ancestor discovery (which is probably a
> good thing), making the above description stale.

Ah, I was confused.  The description only says "refs from outer
namespaces are not advertised, but still participate the common
ancestor discovery in the form of .have lines", and moving the
location where ref-is-hidden is called in this patch does not affect
that fact at all.

So this looks good as-is.

Thanks.

Re: [PATCH v2 4/4] t5509: add basic tests for hideRefs

[Eric Sunshine]

On Tuesday, November 3, 2015, Lukas Fleischer <lfleischer@lfos.de> wrote:
> Test whether regular and full hideRefs patterns work as expected when
> namespaces are used.
>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Lukas Fleischer <lfleischer@lfos.de>
> ---
> diff --git a/t/t5509-fetch-push-namespaces.sh b/t/t5509-fetch-push-namespaces.sh
> @@ -82,4 +82,45 @@ test_expect_success 'mirroring a repository using a ref namespace' '
> +test_expect_success 'hide namespaced refs with transfer.hideRefs' '
> +       GIT_NAMESPACE=namespace \
> +               git -C pushee -c transfer.hideRefs=refs/tags \
> +               ls-remote "ext::git %s ." >actual &&
> +       printf "$commit1\trefs/heads/master\n" >expected &&
> +       test_cmp expected actual
> +'
> +
> +test_expect_success 'check that transfer.hideRefs does not match unstripped refs' '
> +       GIT_NAMESPACE=namespace \
> +               git -C pushee -c transfer.hideRefs=refs/namespaces/namespace/refs/tags \
> +               ls-remote "ext::git %s ." >actual &&
> +       printf "$commit1\trefs/heads/master\n" >expected &&
> +       printf "$commit0\trefs/tags/0\n" >>expected &&
> +       printf "$commit1\trefs/tags/1\n" >>expected &&
> +       test_cmp expected actual
> +'
> +
> +test_expect_success 'hide full refs with transfer.hideRefs' '
> +       GIT_NAMESPACE=namespace \
> +               git -C pushee -c transfer.hideRefs="^refs/namespaces/namespace/refs/tags" \
> +               ls-remote "ext::git %s ." >actual &&
> +       printf "$commit1\trefs/heads/master\n" >expected &&
> +       test_cmp expected actual
> +'
> +
> +test_expect_success 'try to update a hidden ref' '
> +       test_config -C pushee transfer.hideRefs refs/heads/master &&
> +       test_must_fail git -C original push pushee-namespaced master

In above tests, you use -c to set the configuration temporarily for
the git invocation, but not in this and following tests. Is that
because the -c isn't visible to sub-commands which git-push invokes?
(Genuine question; I want to make sure I understand the reasoning.)

> +'
> +
> +test_expect_success 'try to update a ref that is not hidden' '
> +       test_config -C pushee transfer.hideRefs refs/namespaces/namespace/refs/heads/master &&
> +       git -C original push pushee-namespaced master
> +'
> +
> +test_expect_success 'try to update a hidden full ref' '
> +       test_config -C pushee transfer.hideRefs "^refs/namespaces/namespace/refs/heads/master" &&
> +       test_must_fail git -C original push pushee-namespaced master
> +'
> +
>  test_done
> --
> 2.6.2
>

Re: [PATCH v2 4/4] t5509: add basic tests for hideRefs

[Jeff King]

On Wed, Nov 04, 2015 at 02:36:54PM -0500, Eric Sunshine wrote:

> > +test_expect_success 'try to update a hidden ref' '
> > +       test_config -C pushee transfer.hideRefs refs/heads/master &&
> > +       test_must_fail git -C original push pushee-namespaced master
> 
> In above tests, you use -c to set the configuration temporarily for
> the git invocation, but not in this and following tests. Is that
> because the -c isn't visible to sub-commands which git-push invokes?
> (Genuine question; I want to make sure I understand the reasoning.)

Yes, we explicitly clear "-c" variables when we cross repo boundaries.
You can do it like:

  git push --receive-pack='git -c transfer.hideRefs=...  receive-pack'

but that is probably more obfuscated than using test_config.

I was going to complain that "test_config -C" does not actually work,
but somehow I missed 5fafc07 (test-lib-functions: support "test_config
-C <dir> ...", 2015-09-05) going by. Very cool.

-Peff