[PATCH v2 0/3] transfer.hiderefs

[PATCH v2 0/3] transfer.hiderefs

[Junio C Hamano]

So this is the second round, unifying the handling of "hidden refs"
a hosting site administrator may want to handle differently from
normal refs available to upload-pack and receive-pack.  Big thanks
to Peff for making me realize that what is shown to "git push"
should match what is shown to "git fetch/clone".

The configuration variable has been changed to "transfer.hiderefs"
in this round.  I picked refs.[ch] as the shared location to place
the machinery that deals with this configuration, but I am not proud
of the choice---there may be a more suitable place.

The third patch is new; it prevents a hidden ref to be deleted or
updated via "git push" to ensure that its contents are controlled
only from the server side using some side channel (e.g. pull-request
UI on the Web).

There is no "git fetch $there $exact_sha1_obtained_out_of_band"
yet; that must come next, together with a mechanism to control the
availability of it on the server side.

For those who missed it, the first round starts at 

    http://thread.gmane.org/gmane.comp.version-control.git/213951

Junio C Hamano (3):
  upload-pack: share more code
  upload-pack: allow hiding ref hiearchies
  receive-pack: reject an attempt to update/delete a hidden ref

 Documentation/config.txt |  9 +++++++++
 builtin/receive-pack.c   | 24 +++++++++++++++++++++++
 refs.c                   | 41 +++++++++++++++++++++++++++++++++++++++
 refs.h                   |  3 +++
 t/t5512-ls-remote.sh     |  9 +++++++++
 t/t5516-fetch-push.sh    | 24 +++++++++++++++++++++++
 upload-pack.c            | 50 ++++++++++++++++++++++++++++++++----------------
 7 files changed, 144 insertions(+), 16 deletions(-)

-- 
1.8.1.2.548.g0e4986f

[PATCH v2 1/3] upload-pack: share more code

[Junio C Hamano]

We mark the objects pointed at our refs with "OUR_REF" flag in two
functions (mark_our_ref() and send_ref()), but we can just use the
former as a helper for the latter.

Update the way mark_our_ref() prepares in-core object to use
lookup_unknown_object() to delay reading the actual object data,
just like we did in 435c833 (upload-pack: use peel_ref for ref
advertisements, 2012-10-04).

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 upload-pack.c | 31 ++++++++++++++-----------------
 1 file changed, 14 insertions(+), 17 deletions(-)

diff --git a/upload-pack.c b/upload-pack.c
index 95d8313..3dd220d 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -722,15 +722,28 @@ static void receive_needs(void)
 	free(shallows.objects);
 }
 
+static int mark_our_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)
+{
+	struct object *o = lookup_unknown_object(sha1);
+	if (!o)
+		die("git upload-pack: cannot find object %s:", sha1_to_hex(sha1));
+	if (!(o->flags & OUR_REF)) {
+		o->flags |= OUR_REF;
+		nr_our_refs++;
+	}
+	return 0;
+}
+
 static int send_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)
 {
 	static const char *capabilities = "multi_ack thin-pack side-band"
 		" side-band-64k ofs-delta shallow no-progress"
 		" include-tag multi_ack_detailed";
-	struct object *o = lookup_unknown_object(sha1);
 	const char *refname_nons = strip_namespace(refname);
 	unsigned char peeled[20];
 
+	mark_our_ref(refname, sha1, flag, cb_data);
+
 	if (capabilities)
 		packet_write(1, "%s %s%c%s%s agent=%s\n",
 			     sha1_to_hex(sha1), refname_nons,
@@ -740,27 +753,11 @@ static int send_ref(const char *refname, const unsigned char *sha1, int flag, vo
 	else
 		packet_write(1, "%s %s\n", sha1_to_hex(sha1), refname_nons);
 	capabilities = NULL;
-	if (!(o->flags & OUR_REF)) {
-		o->flags |= OUR_REF;
-		nr_our_refs++;
-	}
 	if (!peel_ref(refname, peeled))
 		packet_write(1, "%s %s^{}\n", sha1_to_hex(peeled), refname_nons);
 	return 0;
 }
 
-static int mark_our_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)
-{
-	struct object *o = parse_object(sha1);
-	if (!o)
-		die("git upload-pack: cannot find object %s:", sha1_to_hex(sha1));
-	if (!(o->flags & OUR_REF)) {
-		o->flags |= OUR_REF;
-		nr_our_refs++;
-	}
-	return 0;
-}
-
 static void upload_pack(void)
 {
 	if (advertise_refs || !stateless_rpc) {
-- 
1.8.1.2.548.g0e4986f

[PATCH v2 2/3] upload-pack: allow hiding ref hiearchies

[Junio C Hamano]

Teach upload-pack to omit some refs from the initial advertisement
by introducing the uploadPack.hiderefs multivalued configuration
variable.  Any ref that is under the hierarchies listed on the value
of this variable is excluded from responses to "ls-remote", "fetch"
or "clone" requests.  One typical use case may be

	[uploadPack]
		hiderefs = refs/changes

Note that the underlying protocol still allows a request to fetch
objects at the tip of any ref, including the hidden ones, but on the
client side (e.g. fetch-pack), the requests are checked against the
ls-remote response, so it cannot ask for refs/changes/14/432432, or
for the object name (which is what eventually goes over the wire on
"want" line) the user may obtain out of band (e.g. Gerrit dashboard).
A new capability "allow-tip-sha1-in-want" is returned by upload-pack
to signal updated clients that it may be OK to ask for objects that
were not advertised.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/config.txt |  8 ++++++++
 builtin/receive-pack.c   |  8 ++++++++
 refs.c                   | 41 +++++++++++++++++++++++++++++++++++++++++
 refs.h                   |  3 +++
 t/t5512-ls-remote.sh     |  9 +++++++++
 upload-pack.c            | 25 +++++++++++++++++++++++--
 6 files changed, 92 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index ef45c99..8e2853e 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2057,6 +2057,14 @@ transfer.fsckObjects::
 	not set, the value of this variable is used instead.
 	Defaults to false.
 
+transfer.hiderefs::
+	String(s) `upload-pack` and `receive-pack` use to decide
+	which refs to omit from their initial advertisement.  Use
+	more than one transfer.hiderefs configuration variables to
+	specify multiple prefix strings. A ref that has one of these
+	strings as its prefix is excluded, and is hidden from
+	`git clone --mirror` and `git push :`.
+
 transfer.unpackLimit::
 	When `fetch.unpackLimit` or `receive.unpackLimit` are
 	not set, the value of this variable is used instead.
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index ff781fe..5ed40be 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -59,6 +59,11 @@ static enum deny_action parse_deny_action(const char *var, const char *value)
 
 static int receive_pack_config(const char *var, const char *value, void *cb)
 {
+	int status = parse_hide_refs_config(var, value, cb);
+
+	if (status)
+		return status;
+
 	if (strcmp(var, "receive.denydeletes") == 0) {
 		deny_deletes = git_config_bool(var, value);
 		return 0;
@@ -119,6 +124,9 @@ 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
diff --git a/refs.c b/refs.c
index 541fec2..e3574ca 100644
--- a/refs.c
+++ b/refs.c
@@ -3,6 +3,7 @@
 #include "object.h"
 #include "tag.h"
 #include "dir.h"
+#include "string-list.h"
 
 /*
  * Make sure "ref" is something reasonable to have under ".git/refs/";
@@ -2556,3 +2557,43 @@ char *shorten_unambiguous_ref(const char *refname, int strict)
 	free(short_name);
 	return xstrdup(refname);
 }
+
+static struct string_list *hide_refs;
+
+int parse_hide_refs_config(const char *var, const char *value, void *unused)
+{
+	if (!strcmp("transfer.hiderefs", var)) {
+		char *ref;
+		int len;
+
+		if (!value)
+			return config_error_nonbool(var);
+		ref = xstrdup(value);
+		len = strlen(ref);
+		while (len && ref[len - 1] == '/')
+			ref[--len] = '\0';
+		if (!hide_refs) {
+			hide_refs = xcalloc(1, sizeof(*hide_refs));
+			hide_refs->strdup_strings = 1;
+		}
+		string_list_append(hide_refs, ref);
+	}
+	return 0;
+}
+
+int ref_is_hidden(const char *refname)
+{
+	struct string_list_item *item;
+
+	if (!hide_refs)
+		return 0;
+	for_each_string_list_item(item, hide_refs) {
+		int len;
+		if (prefixcmp(refname, item->string))
+			continue;
+		len = strlen(item->string);
+		if (!refname[len] || refname[len] == '/')
+			return 1;
+	}
+	return 0;
+}
diff --git a/refs.h b/refs.h
index d6c2fe2..50b233f 100644
--- a/refs.h
+++ b/refs.h
@@ -147,4 +147,7 @@ int update_ref(const char *action, const char *refname,
 		const unsigned char *sha1, const unsigned char *oldval,
 		int flags, enum action_on_err onerr);
 
+extern int parse_hide_refs_config(const char *var, const char *value, void *);
+extern int ref_is_hidden(const char *);
+
 #endif /* REFS_H */
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index d16e5d3..d0702ed 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -126,4 +126,13 @@ test_expect_success 'Report match with --exit-code' '
 	test_cmp expect actual
 '
 
+test_expect_success 'Hide some refs' '
+	test_config transfer.hiderefs refs/tags &&
+	git ls-remote . >actual &&
+	test_unconfig transfer.hiderefs &&
+	git ls-remote . |
+	sed -e "/	refs\/tags\//d" >expect &&
+	test_cmp expect actual
+'
+
 test_done
diff --git a/upload-pack.c b/upload-pack.c
index 3dd220d..31ce954 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -12,6 +12,7 @@
 #include "run-command.h"
 #include "sigchain.h"
 #include "version.h"
+#include "string-list.h"
 
 static const char upload_pack_usage[] = "git upload-pack [--strict] [--timeout=<n>] <dir>";
 
@@ -28,10 +29,12 @@ static const char upload_pack_usage[] = "git upload-pack [--strict] [--timeout=<
 
 static unsigned long oldest_have;
 
-static int multi_ack, nr_our_refs;
+static int multi_ack;
+static int nr_our_refs; /* This counts both advertised and unadvertised */
 static int no_done;
 static int use_thin_pack, use_ofs_delta, use_include_tag;
 static int no_progress, daemon_mode;
+static int allow_tip_sha1_in_want;
 static int shallow_nr;
 static struct object_array have_obj;
 static struct object_array want_obj;
@@ -743,11 +746,14 @@ static int send_ref(const char *refname, const unsigned char *sha1, int flag, vo
 	unsigned char peeled[20];
 
 	mark_our_ref(refname, sha1, flag, cb_data);
+	if (ref_is_hidden(refname))
+		return 0;
 
 	if (capabilities)
-		packet_write(1, "%s %s%c%s%s agent=%s\n",
+		packet_write(1, "%s %s%c%s%s%s agent=%s\n",
 			     sha1_to_hex(sha1), refname_nons,
 			     0, capabilities,
+			     allow_tip_sha1_in_want ? " allow-tip-sha1-in-want" : "",
 			     stateless_rpc ? " no-done" : "",
 			     git_user_agent_sanitized());
 	else
@@ -758,11 +764,20 @@ static int send_ref(const char *refname, const unsigned char *sha1, int flag, vo
 	return 0;
 }
 
+static int check_hidden_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)
+{
+	if (!ref_is_hidden(refname))
+		return 0;
+	allow_tip_sha1_in_want = 1;
+	return 1; /* terminate iteration over refs early */
+}
+
 static void upload_pack(void)
 {
 	if (advertise_refs || !stateless_rpc) {
 		reset_timeout();
 		head_ref_namespaced(send_ref, NULL);
+		for_each_namespaced_ref(check_hidden_ref, NULL);
 		for_each_namespaced_ref(send_ref, NULL);
 		packet_flush(1);
 	} else {
@@ -779,6 +794,11 @@ static void upload_pack(void)
 	}
 }
 
+static int upload_pack_config(const char *var, const char *value, void *unused)
+{
+	return parse_hide_refs_config(var, value, unused);
+}
+
 int main(int argc, char **argv)
 {
 	char *dir;
@@ -830,6 +850,7 @@ int main(int argc, char **argv)
 		die("'%s' does not appear to be a git repository", dir);
 	if (is_repository_shallow())
 		die("attempt to fetch/clone from a shallow repository");
+	git_config(upload_pack_config, NULL);
 	if (getenv("GIT_DEBUG_SEND_PACK"))
 		debug_fd = atoi(getenv("GIT_DEBUG_SEND_PACK"));
 	upload_pack();
-- 
1.8.1.2.548.g0e4986f

[PATCH v2 3/3] receive-pack: reject an attempt to update/delete a hidden ref

[Junio C Hamano]

An attempt to update or delete a ref that is hidden from "git push"
is rejected.  With this the server side can implement refs that are
only available for its own use, e.g. refs/pull/11/head used to hold
an incoming pull request at GitHub.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/config.txt |  3 ++-
 builtin/receive-pack.c   | 16 ++++++++++++++++
 t/t5516-fetch-push.sh    | 24 ++++++++++++++++++++++++
 3 files changed, 42 insertions(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 8e2853e..b7b407b 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2063,7 +2063,8 @@ transfer.hiderefs::
 	more than one transfer.hiderefs configuration variables to
 	specify multiple prefix strings. A ref that has one of these
 	strings as its prefix is excluded, and is hidden from
-	`git clone --mirror` and `git push :`.
+	`git clone --mirror` and `git push :`.  An attempt to update
+	or delete a hidden ref by `git push` is	rejected.
 
 transfer.unpackLimit::
 	When `fetch.unpackLimit` or `receive.unpackLimit` are
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 5ed40be..a8248d9 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -696,6 +696,20 @@ static int iterate_receive_command_list(void *cb_data, unsigned char sha1[20])
 	return -1; /* end of list */
 }
 
+static void reject_updates_to_hidden(struct command *commands)
+{
+	struct command *cmd;
+
+	for (cmd = commands; cmd; cmd = cmd->next) {
+		if (cmd->error_string || !ref_is_hidden(cmd->ref_name))
+			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";
+	}
+}
+
 static void execute_commands(struct command *commands, const char *unpacker_error)
 {
 	struct command *cmd;
@@ -712,6 +726,8 @@ static void execute_commands(struct command *commands, const char *unpacker_erro
 				       0, &cmd))
 		set_connectivity_errors(commands);
 
+	reject_updates_to_hidden(commands);
+
 	if (run_receive_hook(commands, pre_receive_hook, 0)) {
 		for (cmd = commands; cmd; cmd = cmd->next) {
 			if (!cmd->error_string)
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 6009372..8e8287a 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1037,4 +1037,28 @@ test_expect_success 'push --prune refspec' '
 	! check_push_result $the_first_commit tmp/foo tmp/bar
 '
 
+test_expect_success 'push to update a hidden ref' '
+	mk_test heads/master hidden/one hidden/two hidden/three &&
+	(
+		cd testrepo &&
+		git config transfer.hiderefs refs/hidden
+	) &&
+
+	# push to unhidden ref succeeds normally
+	git push testrepo master:refs/heads/master &&
+	check_push_result $the_commit heads/master &&
+
+	# push to update a hidden ref should fail
+	test_must_fail git push testrepo master:refs/hidden/one &&
+	check_push_result $the_first_commit hidden/one &&
+
+	# push to delete a hidden ref should fail
+	test_must_fail git push testrepo master:refs/hidden/two &&
+	check_push_result $the_first_commit hidden/two &&
+
+	# idempotent push to update a hidden ref should fail
+	test_must_fail git push testrepo $the_first_commit:refs/hidden/three &&
+	check_push_result $the_first_commit hidden/three
+'
+
 test_done
-- 
1.8.1.2.548.g0e4986f

Re: [PATCH v2 0/3] transfer.hiderefs

[Junio C Hamano]

Please take this as just a preview of early WIP.  I think I may end
up doing moderate amount of refactoring as a preparatory step before
these patches, so nitpick-reviews are likely to become waste of
reviewer's time at this point.

Re: [PATCH v2 0/3] transfer.hiderefs

[Junio C Hamano]

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

> Please take this as just a preview of early WIP.  I think I may end
> up doing moderate amount of refactoring as a preparatory step before
> these patches, so nitpick-reviews are likely to become waste of
> reviewer's time at this point.

I've pushed out a mostly-done reroll on 'pu'; I'll send them out as
patches for review tomorrow.  I think I am making a good progress.