[PATCH 0/2] negative hideRefs for overriding

[PATCH 0/2] negative hideRefs for overriding

[Jeff King]

At GitHub, we keep some information in a private part of the refs
namespace. We use transfer.hideRefs so that users do not see it when
fetching or pushing, and that works well. However, sometimes we want to
expose part of the hidden namespace (e.g., for an internal fetch or
push), and that cannot be done with the existing code. Usually config
variables are last-one-wins, so you can simply overwrite any existing
values with "git -c ...". But for multi-valued config like hideRefs,
that just appends to the list, and the ref remains hidden.

This series adds negation to hideRefs, which gives us a reasonable
last-one-wins setup. That allows "git -c" to unhide a specific set of
refs (though note you have to use "--upload-pack='git -c ...'" since
upload-pack does not share our environment). It also allows more complex
config, like hiding all of "refs/foo" except for "refs/foo/bar". We
don't use that ourselves, but it might come in handy (and logically
falls out of the last-one-wins setup).

The first patch is some documentation cleanup. The interesting bit is in
the second one.

  [1/2]: docs/config.txt: reorder hideRefs config
  [2/2]: refs: support negative transfer.hideRefs

-Peff

[PATCH 1/2] docs/config.txt: reorder hideRefs config

[Jeff King]

The descriptions for receive.hideRefs and
uploadpack.hideRefs are largely the same, and then
transfer.hideRefs refers to both of them. Instead, let's
make transfer.hideRefs the "master" source, and refer to it
from the other sites (with appropriate program-specific
annotations).

This avoids duplication, and will make it easier to document
changes to the config option without having to copy and
paste the description in two places.

While we're at it, this fixes some bogus subject/verb
agreement in the original description.

Signed-off-by: Jeff King <peff@peff.net>
---
I think this makes sense, though I could also see an argument that the
reader would rather hit the individual program's hideRefs config first
and not get redirected. The second patch relies on this textually, but
if we want to drop this one, I can rebase the second to just add the new
text in both places.

 Documentation/config.txt | 32 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 17 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 43bb53c..448eb9d 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2242,13 +2242,10 @@ receive.denyNonFastForwards::
 	set when initializing a shared repository.
 
 receive.hideRefs::
-	String(s) `receive-pack` uses to decide which refs to omit
-	from its initial advertisement.  Use more than one
-	definitions to specify multiple prefix strings. A ref that
-	are under the hierarchies listed on the value of this
-	variable is excluded, and is hidden when responding to `git
-	push`, and an attempt to update or delete a hidden ref by
-	`git push` is rejected.
+	This variable is the same as `transfer.hideRefs`, but applies
+	only to `receive-pack` (and so affects pushes, but not fetches).
+	An attempt to update or delete a hidden ref by `git push` is
+	rejected.
 
 receive.updateServerInfo::
 	If set to true, git-receive-pack will run git-update-server-info
@@ -2536,9 +2533,13 @@ transfer.fsckObjects::
 	Defaults to false.
 
 transfer.hideRefs::
-	This variable can be used to set both `receive.hideRefs`
-	and `uploadpack.hideRefs` at the same time to the same
-	values.  See entries for these other variables.
+	String(s) `receive-pack` and `upload-pack` use to decide which
+	refs to omit from their initial advertisements.  Use more than
+	one definition to specify multiple prefix strings. A ref that is
+	under the hierarchies listed in the value of this variable is
+	excluded, and is hidden when responding to `git push` or `git
+	fetch`.  See `receive.hideRefs` and `uploadpack.hideRefs` for
+	program-specific versions of this config.
 
 transfer.unpackLimit::
 	When `fetch.unpackLimit` or `receive.unpackLimit` are
@@ -2553,13 +2554,10 @@ uploadarchive.allowUnreachable::
 	`false`.
 
 uploadpack.hideRefs::
-	String(s) `upload-pack` uses to decide which refs to omit
-	from its initial advertisement.  Use more than one
-	definitions to specify multiple prefix strings. A ref that
-	are under the hierarchies listed on the value of this
-	variable is excluded, and is hidden from `git ls-remote`,
-	`git fetch`, etc.  An attempt to fetch a hidden ref by `git
-	fetch` will fail.  See also `uploadpack.allowTipSHA1InWant`.
+	This variable is the same as `transfer.hideRefs`, but applies
+	only to `upload-pack` (and so affects only fetches, not pushes).
+	An attempt to fetch a hidden ref by `git fetch` will fail.  See
+	also `uploadpack.allowTipSHA1InWant`.
 
 uploadpack.allowTipSHA1InWant::
 	When `uploadpack.hideRefs` is in effect, allow `upload-pack`
-- 
2.5.0.rc3.557.g17a1555

[PATCH 2/2] refs: support negative transfer.hideRefs

[Jeff King]

If you hide a hierarchy of refs using the transfer.hideRefs
config, there is no way to later override that config to
"unhide" it. This patch implements a "negative" hide which
causes matches to immediately be marked as unhidden, even if
another match would hide it. We take care to apply the
matches in reverse-order from how they are fed to us by the
config machinery, as that lets our usual "last one wins"
config precedence work (and entries in .git/config, for
example, will override /etc/gitconfig).

There are two alternatives that were considered and
rejected:

  1. A generic config mechanism for removing an item from a
     list. E.g.: (e.g., "[transfer] hideRefs -= refs/foo").

     This is nice because it could apply to other
     multi-valued config, as well. But it is not nearly as
     flexible. There is no way to say:

       [transfer]
       hideRefs = refs/secret
       hideRefs = refs/secret/not-so-secret

     Having explicit negative specifications means we can
     override previous entries, even if they are not the
     same literal string.

  2. Adding another variable to override some parts of
     hideRefs (e.g., "exposeRefs").

     This solves the problem from alternative (1), but it
     cannot easily obey the normal config precedence,
     because it would use two separate lists. For example:

       [transfer]
       hideRefs = refs/secret
       exposeRefs = refs/secret/not-so-secret
       hideRefs = refs/secret/not-so-secret/no-really-its-secret

     With two lists, we have to apply the "expose" rules
     first, and only then apply the "hide" rules. But that
     does not match what the above config intends.

     Of course we could internally parse that to a single
     list, respecting the ordering, which saves us having to
     invent the new "!" syntax. But using a single name
     communicates to the user that the ordering _is_
     important. And "!" is well-known for negation, and
     should not appear at the beginning of a ref (it is
     actually valid in a ref-name, but all entries here
     should be fully-qualified, starting with "refs/").

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/config.txt |  5 +++++
 refs.c                   | 18 +++++++++++++-----
 t/t5512-ls-remote.sh     | 15 +++++++++++++++
 3 files changed, 33 insertions(+), 5 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 448eb9d..a7fbd0a 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2540,6 +2540,11 @@ transfer.hideRefs::
 	excluded, and is hidden when responding to `git push` or `git
 	fetch`.  See `receive.hideRefs` and `uploadpack.hideRefs` for
 	program-specific versions of this config.
++
+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).
 
 transfer.unpackLimit::
 	When `fetch.unpackLimit` or `receive.unpackLimit` are
diff --git a/refs.c b/refs.c
index 7ac05cf..68f2cb0 100644
--- a/refs.c
+++ b/refs.c
@@ -4159,17 +4159,25 @@ int parse_hide_refs_config(const char *var, const char *value, const char *secti
 
 int ref_is_hidden(const char *refname)
 {
-	struct string_list_item *item;
+	int i;
 
 	if (!hide_refs)
 		return 0;
-	for_each_string_list_item(item, hide_refs) {
+	for (i = hide_refs->nr - 1; i >= 0; i--) {
+		const char *match = hide_refs->items[i].string;
+		int neg = 0;
 		int len;
-		if (!starts_with(refname, item->string))
+
+		if (*match == '!') {
+			neg = 1;
+			match++;
+		}
+
+		if (!starts_with(refname, match))
 			continue;
-		len = strlen(item->string);
+		len = strlen(match);
 		if (!refname[len] || refname[len] == '/')
-			return 1;
+			return !neg;
 	}
 	return 0;
 }
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 3bd9759..728a1dc 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -128,6 +128,11 @@ test_expect_success 'Report match with --exit-code' '
 	test_cmp expect actual
 '
 
+test_expect_success 'set up some extra tags for ref hiding' '
+	git tag magic/one &&
+	git tag magic/two
+'
+
 for configsection in transfer uploadpack
 do
 	test_expect_success "Hide some refs with $configsection.hiderefs" '
@@ -138,6 +143,16 @@ do
 		sed -e "/	refs\/tags\//d" >expect &&
 		test_cmp expect actual
 	'
+
+	test_expect_success "Override hiding of $configsection.hiderefs" '
+		test_when_finished "test_unconfig $configsection.hiderefs" &&
+		git config --add $configsection.hiderefs refs/tags &&
+		git config --add $configsection.hiderefs "!refs/tags/magic" &&
+		git config --add $configsection.hiderefs refs/tags/magic/one &&
+		git ls-remote . >actual &&
+		verbose grep refs/tags/magic/two actual
+	'
+
 done
 
 test_done
-- 
2.5.0.rc3.557.g17a1555

Re: [PATCH 2/2] refs: support negative transfer.hideRefs

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> If you hide a hierarchy of refs using the transfer.hideRefs
> config, there is no way to later override that config to
> "unhide" it. This patch implements a "negative" hide which
> causes matches to immediately be marked as unhidden, even if
> another match would hide it. We take care to apply the
> matches in reverse-order from how they are fed to us by the
> config machinery, as that lets our usual "last one wins"
> config precedence work (and entries in .git/config, for
> example, will override /etc/gitconfig).
>
> There are two alternatives that were considered and
> rejected:
> ...
>   1. A generic config mechanism for removing an item from a
>      ...
>   2. Adding another variable to override some parts of
>      ...
>      Of course we could internally parse that to a single
>      list, respecting the ordering, which saves us having to
>      invent the new "!" syntax. But using a single name
>      communicates to the user that the ordering _is_
>      important. And "!" is well-known for negation, and
>      should not appear at the beginning of a ref (it is
>      actually valid in a ref-name, but all entries here
>      should be fully-qualified, starting with "refs/").

I notice that the only time you said that you chose '!' prefix as
the way to express this new "negative" is as a side note to the
rejected second variant ;-).  The first paragraph would have been a
good place to say that, because the first thing I wondered after
reading three lines (including the subject) into the log was "ok, it
makes sense and I know what alternatives were considered and
discarded for what reasons without reading the rest, now did he use
prefix '-', prefix '~', prefix '^', or prefix '!' for the new
syntax, or did he use something else?"

It would have been very nice if you chose an invalid ref character
as the negative prefix, and unfortunately '!', which would also have
been my first choice for this prefix, is not an invalid character,
which is a bit sad.

Both patches make sense.  Will queue.

Thanks.

Re: [PATCH 2/2] refs: support negative transfer.hideRefs

[Jeff King]

On Tue, Jul 28, 2015 at 01:14:47PM -0700, Junio C Hamano wrote:

> I notice that the only time you said that you chose '!' prefix as
> the way to express this new "negative" is as a side note to the
> rejected second variant ;-).  The first paragraph would have been a
> good place to say that, because the first thing I wondered after
> reading three lines (including the subject) into the log was "ok, it
> makes sense and I know what alternatives were considered and
> discarded for what reasons without reading the rest, now did he use
> prefix '-', prefix '~', prefix '^', or prefix '!' for the new
> syntax, or did he use something else?"

Yeah, I agree it should come earlier, probably as an example. Below is a
re-roll. I also realized I do not explicitly test that mixing and
matching transfer.* and uploadpack.* works (they should override each
other as if they are part of the same list). It does work, and the
re-roll below has an extra test.

> It would have been very nice if you chose an invalid ref character
> as the negative prefix, and unfortunately '!', which would also have
> been my first choice for this prefix, is not an invalid character,
> which is a bit sad.

Yeah, I considered "^" for that reason, but IMHO it doesn't _quite_ mean
negation. Here "!" means the same thing as it does in .gitignore and
elsewhere. And since the refs are fully qualified, I think it is
effectively invalid (we do not look at "$GIT_DIR/!refs/" in the ref
advertisement, after all).

-- >8 --
Subject: refs: support negative transfer.hideRefs

If you hide a hierarchy of refs using the transfer.hideRefs
config, there is no way to later override that config to
"unhide" it. This patch implements a "negative" hide which
causes matches to immediately be marked as unhidden, even if
another match would hide it. We take care to apply the
matches in reverse-order from how they are fed to us by the
config machinery, as that lets our usual "last one wins"
config precedence work (and entries in .git/config, for
example, will override /etc/gitconfig).

So you can now do:

  $ git config --system transfer.hideRefs refs/secret
  $ git config transfer.hideRefs '!refs/secret/not-so-secret'

to hide refs/secret in all repos, except for one public bit
in one specific repo. Or you can even do:

  $ git clone \
      -u "git -c transfer.hiderefs="!refs/foo" upload-pack" \
      remote:repo.git

to clone remote:repo.git, overriding any hiding it has
configured.

There are two alternatives that were considered and
rejected:

  1. A generic config mechanism for removing an item from a
     list. E.g.: (e.g., "[transfer] hideRefs -= refs/foo").

     This is nice because it could apply to other
     multi-valued config, as well. But it is not nearly as
     flexible. There is no way to say:

       [transfer]
       hideRefs = refs/secret
       hideRefs = refs/secret/not-so-secret

     Having explicit negative specifications means we can
     override previous entries, even if they are not the
     same literal string.

  2. Adding another variable to override some parts of
     hideRefs (e.g., "exposeRefs").

     This solves the problem from alternative (1), but it
     cannot easily obey the normal config precedence,
     because it would use two separate lists. For example:

       [transfer]
       hideRefs = refs/secret
       exposeRefs = refs/secret/not-so-secret
       hideRefs = refs/secret/not-so-secret/no-really-its-secret

     With two lists, we have to apply the "expose" rules
     first, and only then apply the "hide" rules. But that
     does not match what the above config intends.

     Of course we could internally parse that to a single
     list, respecting the ordering, which saves us having to
     invent the new "!" syntax. But using a single name
     communicates to the user that the ordering _is_
     important. And "!" is well-known for negation, and
     should not appear at the beginning of a ref (it is
     actually valid in a ref-name, but all entries here
     should be fully-qualified, starting with "refs/").

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/config.txt |  5 +++++
 refs.c                   | 18 +++++++++++++-----
 t/t5512-ls-remote.sh     | 22 ++++++++++++++++++++++
 3 files changed, 40 insertions(+), 5 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 448eb9d..a7fbd0a 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2540,6 +2540,11 @@ transfer.hideRefs::
 	excluded, and is hidden when responding to `git push` or `git
 	fetch`.  See `receive.hideRefs` and `uploadpack.hideRefs` for
 	program-specific versions of this config.
++
+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).
 
 transfer.unpackLimit::
 	When `fetch.unpackLimit` or `receive.unpackLimit` are
diff --git a/refs.c b/refs.c
index 7ac05cf..68f2cb0 100644
--- a/refs.c
+++ b/refs.c
@@ -4159,17 +4159,25 @@ int parse_hide_refs_config(const char *var, const char *value, const char *secti
 
 int ref_is_hidden(const char *refname)
 {
-	struct string_list_item *item;
+	int i;
 
 	if (!hide_refs)
 		return 0;
-	for_each_string_list_item(item, hide_refs) {
+	for (i = hide_refs->nr - 1; i >= 0; i--) {
+		const char *match = hide_refs->items[i].string;
+		int neg = 0;
 		int len;
-		if (!starts_with(refname, item->string))
+
+		if (*match == '!') {
+			neg = 1;
+			match++;
+		}
+
+		if (!starts_with(refname, match))
 			continue;
-		len = strlen(item->string);
+		len = strlen(match);
 		if (!refname[len] || refname[len] == '/')
-			return 1;
+			return !neg;
 	}
 	return 0;
 }
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 3bd9759..afde495 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -128,6 +128,11 @@ test_expect_success 'Report match with --exit-code' '
 	test_cmp expect actual
 '
 
+test_expect_success 'set up some extra tags for ref hiding' '
+	git tag magic/one &&
+	git tag magic/two
+'
+
 for configsection in transfer uploadpack
 do
 	test_expect_success "Hide some refs with $configsection.hiderefs" '
@@ -138,6 +143,23 @@ do
 		sed -e "/	refs\/tags\//d" >expect &&
 		test_cmp expect actual
 	'
+
+	test_expect_success "Override hiding of $configsection.hiderefs" '
+		test_when_finished "test_unconfig $configsection.hiderefs" &&
+		git config --add $configsection.hiderefs refs/tags &&
+		git config --add $configsection.hiderefs "!refs/tags/magic" &&
+		git config --add $configsection.hiderefs refs/tags/magic/one &&
+		git ls-remote . >actual &&
+		verbose grep refs/tags/magic/two actual
+	'
+
 done
 
+test_expect_success 'overrides work between mixed transfer/upload-pack hideRefs' '
+	test_config uploadpack.hiderefs refs/tags &&
+	test_config transfer.hiderefs "!refs/tags/magic" &&
+	git ls-remote . >actual &&
+	verbose grep refs/tags/magic actual
+'
+
 test_done
-- 
2.5.0.rc3.557.g17a1555

Re: [PATCH 2/2] refs: support negative transfer.hideRefs

[Eric Sunshine]

On Tue, Jul 28, 2015 at 3:59 PM, Jeff King <peff@peff.net> wrote:
> If you hide a hierarchy of refs using the transfer.hideRefs
> config, there is no way to later override that config to
> "unhide" it. This patch implements a "negative" hide which
> causes matches to immediately be marked as unhidden, even if
> another match would hide it. We take care to apply the
> matches in reverse-order from how they are fed to us by the
> config machinery, as that lets our usual "last one wins"
> config precedence work (and entries in .git/config, for
> example, will override /etc/gitconfig).
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> +test_expect_success 'set up some extra tags for ref hiding' '
> +       git tag magic/one &&
> +       git tag magic/two
> +'
> +
>  for configsection in transfer uploadpack
>  do
>         test_expect_success "Hide some refs with $configsection.hiderefs" '
> @@ -138,6 +143,16 @@ do
>                 sed -e "/       refs\/tags\//d" >expect &&
>                 test_cmp expect actual
>         '
> +
> +       test_expect_success "Override hiding of $configsection.hiderefs" '
> +               test_when_finished "test_unconfig $configsection.hiderefs" &&
> +               git config --add $configsection.hiderefs refs/tags &&
> +               git config --add $configsection.hiderefs "!refs/tags/magic" &&
> +               git config --add $configsection.hiderefs refs/tags/magic/one &&
> +               git ls-remote . >actual &&
> +               verbose grep refs/tags/magic/two actual

For completeness, do you also want to add

    !grep refs/tags/magic/one actual

to confirm that the third hideRefs did indeed override the second one?

> +       '
> +
>  done
>
>  test_done
> --
> 2.5.0.rc3.557.g17a1555

Re: [PATCH 2/2] refs: support negative transfer.hideRefs

[Jeff King]

On Thu, Jul 30, 2015 at 04:17:35PM -0400, Eric Sunshine wrote:

> > +       test_expect_success "Override hiding of $configsection.hiderefs" '
> > +               test_when_finished "test_unconfig $configsection.hiderefs" &&
> > +               git config --add $configsection.hiderefs refs/tags &&
> > +               git config --add $configsection.hiderefs "!refs/tags/magic" &&
> > +               git config --add $configsection.hiderefs refs/tags/magic/one &&
> > +               git ls-remote . >actual &&
> > +               verbose grep refs/tags/magic/two actual
> 
> For completeness, do you also want to add
> 
>     !grep refs/tags/magic/one actual
> 
> to confirm that the third hideRefs did indeed override the second one?

Yeah, I think that is a good change. We could also test_cmp the whole
output, but I didn't want to be dependent on other junk from previous
tests.

-Peff