negative hideRefs for overriding

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

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.

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

index 43bb53c..448eb9d 100644
@@ -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

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

---
 Documentation/config.txt |  5 +++++
 refs.c                   | 18 +++++++++++++-----
 t/t5512-ls-remote.sh     | 15 +++++++++++++++
 3 files changed, 33 insertions(+), 5 deletions(-)

index 448eb9d..a7fbd0a 100644
@@ -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

index 7ac05cf..68f2cb0 100644
@@ -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;
 }

index 3bd9759..728a1dc 100755
@@ -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

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.

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.

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

---
 Documentation/config.txt |  5 +++++
 refs.c                   | 18 +++++++++++++-----
 t/t5512-ls-remote.sh     | 22 ++++++++++++++++++++++
 3 files changed, 40 insertions(+), 5 deletions(-)

index 448eb9d..a7fbd0a 100644
@@ -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

index 7ac05cf..68f2cb0 100644
@@ -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;
 }

index 3bd9759..afde495 100755
@@ -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

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