protocol v2 and hidden refs

When using the v2 protocol, hidden-ref config is not respected at all:

  $ git config transfer.hiderefs refs/tags/
  $ git -c protocol.version=0 ls-remote . | grep -c refs/tags
  0
  $ git -c protocol.version=2 ls-remote . | grep -c refs/tags
  1424

The fix in patch 3 is pretty straightforward, but note:

- I'm a little worried this may happen again with future features. The root cause is that "ls-refs" follows a different code path than the ref advertisement for "upload-pack". So if we add any new config, it needs to go both places (non ref-advertisement config is OK, as the v2 "fetch" command is a lot closer to a v0 upload-pack).

I think this is just an issue for any future features. I looked for other existing features which might be missing in v2, but couldn't find any.

I don't know if there's a good solution. I tried running the whole test suite with v2 as the default. It does find this bug, but it has a bunch of other problems (notably fetch-pack won't run as v2, but some other tests I think also depend on v0's reachability rules, which v2 is documented not to enforce).

- The "serve" command is funky, because it has no concept of whether the "ls-refs" is for fetching or pushing. Is git-serve even a thing that we want to support going forward? I know part of the original v2 conception was that one would be able to just connect to "git-serve" and do a number of operations. But in practice the v2 probing requires saying "I'd like to git-upload-pack, and v2 if you please". So no client ever calls git-serve.

Is this something we plan to eventually move to? Or can it be considered a funny vestige of the development? In the latter case, I think we should consider removing it.

I've worked around it here with patch 2, but note that "git serve" would not ever respect uploadpack.hiderefs nor receive.hiderefs (since it has no idea which operation it's doing).

The patches are:

[1/3]: serve: pass "config context" through to individual commands [2/3]: parse_hide_refs_config: handle NULL section [3/3]: upload-pack: support hidden refs with protocol v2

 builtin/upload-pack.c |  1 +
 ls-refs.c             | 16 +++++++++++++++-
 ls-refs.h             |  3 ++-
 refs.c                |  3 ++-
 serve.c               |  9 +++++----
 serve.h               |  7 +++++++
 t/t5512-ls-remote.sh  |  6 ++++++
 upload-pack.c         |  4 ++--
 upload-pack.h         |  4 ++--
 9 files changed, 42 insertions(+), 11 deletions(-)

-Peff

In protocol v2, instead of just running "upload-pack", we have a generic "serve" loop which runs command requests from the client. What used to be "upload-pack" is now generally split into two operations: "ls-refs" and "fetch". The latter knows it must respect uploadpack.* config, but the former is not actually specific to a fetch operation (we do not yet do v2 receive-pack, but eventually we may, and ls-refs would support both operations).

However, ls-refs does need to know which operation we're performing, in order to read the correct config (e.g., uploadpack.hiderefs versus receive.hiderefs; we don't read _either_ right now, but this is the first step to fixing that).

In the generic "git-serve" program, we don't have that information. But in the protocol as it is actually used by clients, the client still asks to run "git-upload-pack", and then issues an "ls-refs" from there. So we _do_ know at that point that "uploadpack" is the right config context. This patch teaches the protocol v2 "serve" code to pass that context through to the individual commands (a future patch will teach ls-refs to actually use it).

---
 builtin/upload-pack.c | 1 +
 ls-refs.c             | 4 +++-
 ls-refs.h             | 3 ++-
 serve.c               | 9 +++++----
 serve.h               | 7 +++++++
 upload-pack.c         | 4 ++--
 upload-pack.h         | 4 ++--
 7 files changed, 22 insertions(+), 10 deletions(-)

index 42dc4da5a1..67192cfa9e 100644
@@ -52,6 +52,7 @@ int cmd_upload_pack(int argc, const char **argv, const char *prefix)
 	case protocol_v2:
 		serve_opts.advertise_capabilities = opts.advertise_refs;
 		serve_opts.stateless_rpc = opts.stateless_rpc;
+		serve_opts.config_context = "uploadpack";
 		serve(&serve_opts);
 		break;
 	case protocol_v1:

index a06f12eca8..e8e31475f0 100644
@@ -69,7 +69,9 @@ static int send_ref(const char *refname, const struct object_id *oid,
 	return 0;
 }
 
-int ls_refs(struct repository *r, struct argv_array *keys,
+int ls_refs(struct repository *r,
+	    const char *config_section,
+	    struct argv_array *keys,
 	    struct packet_reader *request)
 {
 	struct ls_refs_data data;

index b62877e8da..da26fc9824 100644
@@ -4,7 +4,8 @@
 struct repository;
 struct argv_array;
 struct packet_reader;
-extern int ls_refs(struct repository *r, struct argv_array *keys,
+extern int ls_refs(struct repository *r, const char *config_context,
+		   struct argv_array *keys,
 		   struct packet_reader *request);
 
 #endif /* LS_REFS_H */

index bda085f09c..70f89cf0d9 100644
@@ -48,6 +48,7 @@ struct protocol_capability {
 	 * This field should be NULL for capabilities which are not commands.
 	 */
 	int (*command)(struct repository *r,
+		       const char *config_context,
 		       struct argv_array *keys,
 		       struct packet_reader *request);
 };

@@ -158,7 +159,7 @@ enum request_state {
 	PROCESS_REQUEST_DONE,
 };
 
-static int process_request(void)
+static int process_request(struct serve_options *opts)
 {
 	enum request_state state = PROCESS_REQUEST_KEYS;
 	struct packet_reader reader;

@@ -222,7 +223,7 @@ static int process_request(void)
 	if (!command)
 		die("no command requested");
 
-	command->command(the_repository, &keys, &reader);
+	command->command(the_repository, opts->config_context, &keys, &reader);
 
 	argv_array_clear(&keys);
 	return 0;

@@ -249,10 +250,10 @@ void serve(struct serve_options *options)
 	 * a single request/response exchange
 	 */
 	if (options->stateless_rpc) {
-		process_request();
+		process_request(options);
 	} else {
 		for (;;)
-			if (process_request())
+			if (process_request(options))
 				break;
 	}
 }

index fe65ba9f46..d527224cbb 100644
@@ -8,6 +8,13 @@ extern int has_capability(const struct argv_array *keys, const char *capability,
 struct serve_options {
 	unsigned advertise_capabilities;
 	unsigned stateless_rpc;
+
+	/*
+	 * Some operations may need to know the context when looking up config;
+	 * e.g., set this to "uploadpack" to respect "uploadpack.hiderefs" (as
+	 * opposed to "receive.hiderefs").
+	 */
+	const char *config_context;
 };
 #define SERVE_OPTIONS_INIT { 0 }
 extern void serve(struct serve_options *options);

index 5e81f1ff24..914bbb40bc 100644
@@ -1413,8 +1413,8 @@ enum fetch_state {
 	FETCH_DONE,
 };
 
-int upload_pack_v2(struct repository *r, struct argv_array *keys,
-		   struct packet_reader *request)
+int upload_pack_v2(struct repository *r, const char *config_context,
+		   struct argv_array *keys, struct packet_reader *request)
 {
 	enum fetch_state state = FETCH_PROCESS_ARGS;
 	struct upload_pack_data data;

index cab2178796..2a9f51197c 100644
@@ -13,8 +13,8 @@ void upload_pack(struct upload_pack_options *options);
 struct repository;
 struct argv_array;
 struct packet_reader;
-extern int upload_pack_v2(struct repository *r, struct argv_array *keys,
-			  struct packet_reader *request);
+extern int upload_pack_v2(struct repository *r, const char *config_context,
+			  struct argv_array *keys, struct packet_reader *request);
 
 struct strbuf;
 extern int upload_pack_advertise(struct repository *r,

2.20.0.734.gb4f2c0d2be

Thanks for a clear description of ugly status quo X-<.

I actually started by doing that, but "struct serve_options" is not currently known by ls-refs.c, upload-pack.c, etc. So they'd have to start including "serve.h". I don't think that's the end of the world, but it felt like a funny mutual circular to me (my mental model now is that ls-refs, upload-pack, etc are low-level commands, tied together by the "serve" stuff).

-Peff

That matches my mental model, too. I think the difference between us is what *_opt struct is. I viewed that it was like diff_options struct where the driving machinery keeps state of the ongoing operation performed by lower level routines to fulfill the request by the API caller, i.e. holding both wish from the caller, and scratchpad data for the mchinery and the lower level routine to communicate with each other.

And the new field feels like the last "scratchpad used by the machinery to tell lower-level ls-refs helper what context it is operting in".

I dunno.

Yeah, I agree that such a "pass this through" struct full of options and context would make sense. I just wouldn't tie it to the "serve" machinery.

Did you read the side-thread between me and Jonathan? Another option here is to just have ls-refs assume that the client will tell it the context (and assume uploadpack for now, since that's all that v2 currently does).

That would make this patch go away entirely. :)

-Peff

Yes, I'd be 100% happy with that, too. And it certainly is simpler.

Thanks.

P.S. I expect to be mostly offline for the coming 72 hours, as I and my wife are both down with a cold. I am guessing that we will enter slower weeks in many parts of the world, and hoping this won't be too disruptive.

OK, let's do that, then. The user-visible behavior is no different, so we can always reverse course later when the v2 push scheme materializes. Patch is below.

Hope you are feeling better. I'll be active through the rest of this week, and then probably offline quite a bit for the two weeks following.

-Peff

-- >8 --
Subject: [PATCH] upload-pack: support hidden refs with protocol v2

In the v2 protocol, upload-pack's advertisement has been moved to the "ls-refs" command. That command does not respect hidden-ref config (like transfer.hiderefs) at all, and advertises everything.

While there are some features that are not supported in v2 (e.g., v2 always allows fetching any sha1 without respect to advertisements), the lack of this feature is not documented and is likely just a bug. Let's make it work, as otherwise upgrading a server to a v2-capable git will start exposing these refs that the repository admin has asked to remain hidden.

Note that we assume we're operating on behalf of a fetch here, since that's the only thing implemented in v2 at this point. See the in-code comment. We'll have to figure out how this works when the v2 push protocol is designed (both here in ls-refs, but also rejecting updates to hidden refs).

---
 ls-refs.c            | 16 ++++++++++++++++
 t/t5512-ls-remote.sh |  6 ++++++
 2 files changed, 22 insertions(+)

index a06f12eca8..9c9a7c647f 100644
@@ -5,6 +5,7 @@
 #include "argv-array.h"
 #include "ls-refs.h"
 #include "pkt-line.h"
+#include "config.h"
 
 /*
  * Check if one of the prefixes is a prefix of the ref.

@@ -40,6 +41,9 @@ static int send_ref(const char *refname, const struct object_id *oid,
 	const char *refname_nons = strip_namespace(refname);
 	struct strbuf refline = STRBUF_INIT;
 
+	if (ref_is_hidden(refname_nons, refname))
+		return 0;
+
 	if (!ref_match(&data->prefixes, refname))
 		return 0;
 

@@ -69,6 +73,16 @@ static int send_ref(const char *refname, const struct object_id *oid,
 	return 0;
 }
 
+static int ls_refs_config(const char *var, const char *value, void *data)
+{
+	/*
+	 * We only serve fetches over v2 for now, so respect only "uploadpack"
+	 * config. This may need to eventually be expanded to "receive", but we
+	 * don't yet know how that information will be passed to ls-refs.
+	 */
+	return parse_hide_refs_config(var, value, "uploadpack");
+}
+
 int ls_refs(struct repository *r, struct argv_array *keys,
 	    struct packet_reader *request)
 {

@@ -76,6 +90,8 @@ int ls_refs(struct repository *r, struct argv_array *keys,
 
 	memset(&data, 0, sizeof(data));
 
+	git_config(ls_refs_config, NULL);
+
 	while (packet_reader_read(request) != PACKET_READ_FLUSH) {
 		const char *arg = request->line;
 		const char *out;

index 32e722db2e..ca69636fd5 100755
@@ -204,6 +204,12 @@ test_expect_success 'overrides work between mixed transfer/upload-pack hideRefs'
 	grep refs/tags/magic actual
 '
 
+test_expect_success 'protocol v2 supports hiderefs' '
+	test_config uploadpack.hiderefs refs/tags &&
+	git -c protocol.version=2 ls-remote . >actual &&
+	! grep refs/tags actual
+'
+
 test_expect_success 'ls-remote --symref' '
 	git fetch origin &&
 	cat >expect <<-EOF &&

2.20.1.748.g4743e5683b

Hi,

I think I'm missing something. Why wouldn't "ls-refs for push" not pass the information that it's for push as part of the *body* of the ls-refs request?

(That's a separate issue from whether we need to have ls-refs for push at all, as opposed to specifying a policy for the requested ref updates and getting a list of "have"s without ref names attached. But that's a discussion for another day.)

Is there some other more immediate motivation for this patch? In the spirit of YAGNI, I would rather understand that motivation instead of one that in many possible designs would never materialize.

Thanks,
Jonathan

I don't know. Why doesn't the current "ls-refs" say "ls-refs for fetch"?

Certainly if that information was carried from the client request it would work fine, and ls-refs would have enough to know which config to respect. But I could not find any documentation on this, nor discussion of plans for a v2 push. Since that information isn't passed now, we'd have to assume that "ls-refs" without "for-push" always means "for fetch".

I'm conceptually OK with that, but if that is the plan for going forward, it was not at all obvious to me (and it does feel rather implicit).

Without this information, in patch 3 ls-refs cannot know to look at uploadpack.hiderefs, unless it makes the implicit assumption that it is always serving a fetch.

-Peff

Also YAGNI. ;-)

In other words, since the design for push isn't set in stone yet, we had nothing to be consistent with. And if there's an option to ls-ref to indicate whether it's for fetch or for push, then it can default to fetch.

As an aside, my experience from teaching people about Git protocol is that the concept of "ls-remote for push" producing a different result from "git ls-remote" is very confusing. Given what it is used for, I am motivated to replace it with something more tailored.

Interesting. The last discussion of push v2 plans was in
https://public-inbox.org/git/[email protected]/.
Expect to hear more soon.

Don't get me wrong: I haven't wrapped my head around config context and how it fits into the broader picture yet, but it may be a very good thing to have. So please consider this comment to be about the commit message only.

Based on the motivation you're describing here, I think treating it as uploadpack and adding a NEEDSWORK comment would be a good way forward. If we're moving toward a world with more protocol commands that don't fit in the upload-pack / receive-pack categories, then we need to figure out in more detail what that world looks like:

- do we keep on adding new endpoints, in the same spirit as upload-archive? If so, what endpoint should a client use to get capabilities before it decides which endpoint to use?

- do we merge everything in "git serve" except where a specific endpoint is needed for protocol v0 compatibility? That would lose the ability to distinguish fetches from pushes without looking at the body of requests (which is useful to some people for monitoring, blocking, etc) --- do we consider that to be an acceptable loss?

- once we've decided what the future should look like, how does the transition to that future look?

I think that's a reasonable assumption to make, especially if made explicit using a simple comment. :)

Thanks for explaining,
Jonathan

The words "ls-refs" and "advertisement" are notably absent from that thread. ;)

If we're OK with accepting that the client will pass along the fetch/push context for each individual command, then I don't think we would ever need this. It is literally about relaying the fact of "the original request was via upload-pack". If the commands already know the context the client is interested in from another method, then I don't think they should ever need to care about that fact.

I agree those are all interesting open questions. I didn't want to solve any of them now, but just fix this (IMHO pretty serious) regression. I was mostly trying to do so without making any assumptions about where we'd go in the future (and even NEEDSWORK feels a little funny; it's not clear to me whether that work is going to be needed or not).

The big danger is that somebody does implement a "push" command and forgets to touch ls-refs. That would be wrong and buggy for more reasons than this (as you noted earlier, it should handle .have refs somehow). But what worries me is that the failure mode for the bug is to start exposing refs which are meant to be hidden. Which to me is a little more serious than "the new functionality doesn't work".

So I guess I considered it to mostly be defensive (and I'd be fine if it was later ripped out when a more elegant approach becomes obvious).

That said, I'm not totally opposed to the implicit thing if that's where we all think the protocol code should be headed. The patch is certainly smaller. The whole series could be replaced with this:

-- >8 --
Subject: [PATCH] upload-pack: support hidden refs with protocol v2

In the v2 protocol, upload-pack's advertisement has been moved to the "ls-refs" command. That command does not respect hidden-ref config (like transfer.hiderefs) at all, and advertises everything.

While there are some features that are not supported in v2 (e.g., v2 always allows fetching any sha1 without respect to advertisements), the lack of this feature is not documented and is likely just a bug. Let's make it work, as otherwise upgrading a server to a v2-capable git will start exposing these refs that the repository admin has asked to remain hidden.

Note that we assume we're operating on behalf of a fetch here, since that's the only thing implemented in v2 at this point. See the in-code comment.

---
 ls-refs.c            | 16 ++++++++++++++++
 t/t5512-ls-remote.sh |  6 ++++++
 2 files changed, 22 insertions(+)

index a06f12eca8..9c9a7c647f 100644
@@ -5,6 +5,7 @@
 #include "argv-array.h"
 #include "ls-refs.h"
 #include "pkt-line.h"
+#include "config.h"
 
 /*
  * Check if one of the prefixes is a prefix of the ref.

@@ -40,6 +41,9 @@ static int send_ref(const char *refname, const struct object_id *oid,
 	const char *refname_nons = strip_namespace(refname);
 	struct strbuf refline = STRBUF_INIT;
 
+	if (ref_is_hidden(refname_nons, refname))
+		return 0;
+
 	if (!ref_match(&data->prefixes, refname))
 		return 0;
 

@@ -69,6 +73,16 @@ static int send_ref(const char *refname, const struct object_id *oid,
 	return 0;
 }
 
+static int ls_refs_config(const char *var, const char *value, void *data)
+{
+	/*
+	 * We only serve fetches over v2 for now, so respect only "uploadpack"
+	 * config. This may need to eventually be expanded to "receive", but we
+	 * don't yet know how that information will be passed to ls-refs.
+	 */
+	return parse_hide_refs_config(var, value, "uploadpack");
+}
+
 int ls_refs(struct repository *r, struct argv_array *keys,
 	    struct packet_reader *request)
 {

@@ -76,6 +90,8 @@ int ls_refs(struct repository *r, struct argv_array *keys,
 
 	memset(&data, 0, sizeof(data));
 
+	git_config(ls_refs_config, NULL);
+
 	while (packet_reader_read(request) != PACKET_READ_FLUSH) {
 		const char *arg = request->line;
 		const char *out;

index 32e722db2e..ca69636fd5 100755
@@ -204,6 +204,12 @@ test_expect_success 'overrides work between mixed transfer/upload-pack hideRefs'
 	grep refs/tags/magic actual
 '
 
+test_expect_success 'protocol v2 supports hiderefs' '
+	test_config uploadpack.hiderefs refs/tags &&
+	git -c protocol.version=2 ls-remote . >actual &&
+	! grep refs/tags actual
+'
+
 test_expect_success 'ls-remote --symref' '
 	git fetch origin &&
 	cat >expect <<-EOF &&

2.20.0.738.gdb22cab611

This helper function looks for config in two places: transfer.hiderefs, or $section.hiderefs, where $section is passed in by the caller (and is "uploadpack" or "receive", depending on the context).

In preparation for callers which do not even have that context (namely the "git-serve" command), let's handle a NULL by looking only at transfer.hiderefs (as opposed to segfaulting).

---
 refs.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index f9936355cd..099e91d9cc 100644
@@ -1267,7 +1267,8 @@ int parse_hide_refs_config(const char *var, const char *value, const char *secti
 {
 	const char *key;
 	if (!strcmp("transfer.hiderefs", var) ||
-	    (!parse_config_key(var, section, NULL, NULL, &key) &&
+	    (section &&
+	     !parse_config_key(var, section, NULL, NULL, &key) &&
 	     !strcmp(key, "hiderefs"))) {
 		char *ref;
 		int len;

2.20.0.734.gb4f2c0d2be

Makes sense, and I too wonder if we want to keep the "serve" thing in the longer term, at least in the current form.

In the v2 protocol, upload-pack's advertisement has been moved to the "ls-refs" command. That command does not respect hidden-ref config (like transfer.hiderefs) at all, and advertises everything.

While there are some features that are not supported in v2 (e.g., v2 always allows fetching any sha1 without respect to advertisements), the lack of this feature is not documented and is likely just a bug. Let's make it work, as otherwise upgrading a server to a v2-capable git will start exposing these refs that the repository admin has asked to remain hidden.

Note that we depend on the config_context from the caller here to realize that we should respect uploadpack.hiderefs. When run from the "git-serve" tool, we won't have that context and will only respect transfer.hiderefs. This should be OK, as git-serve is not actually used for normal protocol operations.

---
 ls-refs.c            | 12 ++++++++++++
 t/t5512-ls-remote.sh |  6 ++++++
 2 files changed, 18 insertions(+)

index e8e31475f0..8a8143338b 100644
@@ -5,6 +5,7 @@
 #include "argv-array.h"
 #include "ls-refs.h"
 #include "pkt-line.h"
+#include "config.h"
 
 /*
  * Check if one of the prefixes is a prefix of the ref.

@@ -40,6 +41,9 @@ static int send_ref(const char *refname, const struct object_id *oid,
 	const char *refname_nons = strip_namespace(refname);
 	struct strbuf refline = STRBUF_INIT;
 
+	if (ref_is_hidden(refname_nons, refname))
+		return 0;
+
 	if (!ref_match(&data->prefixes, refname))
 		return 0;
 

@@ -69,6 +73,12 @@ static int send_ref(const char *refname, const struct object_id *oid,
 	return 0;
 }
 
+static int ls_refs_config(const char *var, const char *value,
+			  void *config_context)
+{
+	return parse_hide_refs_config(var, value, config_context);
+}
+
 int ls_refs(struct repository *r,
 	    const char *config_section,
 	    struct argv_array *keys,

@@ -78,6 +88,8 @@ int ls_refs(struct repository *r,
 
 	memset(&data, 0, sizeof(data));
 
+	git_config(ls_refs_config, (void *)config_section);
+
 	while (packet_reader_read(request) != PACKET_READ_FLUSH) {
 		const char *arg = request->line;
 		const char *out;

index 32e722db2e..ca69636fd5 100755
@@ -204,6 +204,12 @@ test_expect_success 'overrides work between mixed transfer/upload-pack hideRefs'
 	grep refs/tags/magic actual
 '
 
+test_expect_success 'protocol v2 supports hiderefs' '
+	test_config uploadpack.hiderefs refs/tags &&
+	git -c protocol.version=2 ls-remote . >actual &&
+	! grep refs/tags actual
+'
+
 test_expect_success 'ls-remote --symref' '
 	git fetch origin &&
 	cat >expect <<-EOF &&

2.20.0.734.gb4f2c0d2be

I think a global test mode for it would be a very good idea.

Does this issue rise to the level of needing a security point-release (which I'm discussing here as the details are already public). The transfer.hideRefs docs have said:

Even if you hide refs, a client may still be able to steal the target objects via the techniques described in the "SECURITY" section of the gitnamespaces(7) man page; it’s best to keep private data in a separate repository.

So we never promised to hide the objects, but definitely promised to hide the ref names. I don't know if anyone uses this in practice for secret ref names, but if they do they have a data leak if they enable protocol v2.

More importantly, the docs for receive.hideRefs say. "An attempt to update or delete a hidden ref by git push is rejected.". It seems this bit was enforced, i.e. this passes before and after your 3/3, but I have not dug enough to be 100% satisfied with that.

    diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
    index ca69636fd5..20059c3308 100755
    --- a/t/t5512-ls-remote.sh
    +++ b/t/t5512-ls-remote.sh
    @@ -210,6 +210,13 @@ test_expect_success 'protocol v2 supports hiderefs' '
     	! grep refs/tags actual
     '

    +test_expect_success 'protocol v2 respects hiderefs when pushing' '
    +	git init --bare server.git &&
    +	git -C server.git config transfer.hideRefs refs/tags &&
    +	test_must_fail git -c protocol.version=0 push "file://$PWD/server.git" mark &&
    +	test_must_fail git -c protocol.version=2 push "file://$PWD/server.git" mark
    +'
    +
     test_expect_success 'ls-remote --symref' '
     	git fetch origin &&
     	cat >expect <<-EOF &&

If there's some bug where you can bypass this push protection that would be much worse. E.g. GitLab uses "keep-around" refs to track its own internal state, and it would be bad if users could manipulate it.

Yeah, but somebody needs to pick through the dozens of false positives for it to be useful.

Yes, that was my line of thinking. You can't really consider such items secure, though it is unfortunate that this leak makes it way easier to access them (you can just fetch them, rather than playing oracle-guessing games with deltas).

At GitHub we keep some internal book-keeping refs, but exposing them to the user is mostly an annoyance.

One thing to note is that there's no "enable protocol v2". If you're running a recent enough Git (v2.18+?) on the server, anybody can ask for these refs.

This part is OK. There is no v2 push protocol yet, so you end up running the regular v0 receive-pack.

-Peff

Here's that test mode. As noted in 3/3 there may be more bugs revealed by this, but let's first start by marking where the behavior differs.

Ævar Arnfjörð Bjarmason (3):
tests: add a special setup where for protocol.version
tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=1
tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2

 protocol.c                           | 13 ++++++++++++-
 t/README                             |  4 ++++
 t/t0410-partial-clone.sh             |  1 +
 t/t5400-send-pack.sh                 |  2 +-
 t/t5500-fetch-pack.sh                |  4 +++-
 t/t5503-tagfollow.sh                 |  8 ++++----
 t/t5512-ls-remote.sh                 |  8 ++++----
 t/t5515-fetch-merge-logic.sh         |  1 +
 t/t5516-fetch-push.sh                |  4 +++-
 t/t5537-fetch-shallow.sh             |  3 ++-
 t/t5552-skipping-fetch-negotiator.sh |  1 +
 t/t5601-clone.sh                     |  1 +
 t/t5616-partial-clone.sh             |  3 ++-
 t/t5700-protocol-v1.sh               |  1 +
 t/t5702-protocol-v2.sh               |  1 +
 t/t7406-submodule-update.sh          |  3 ++-
 16 files changed, 43 insertions(+), 15 deletions(-)

2.20.0.405.gbc1bbc6f85

...and I forgot to mention. This goes on top of Jeff's series here to fix this "hidden refs" case.

Add a GIT_TEST_PROTOCOL_VERSION=X test mode which is equivalent to running with protocol.version=X. This is needed to spot regressions and differences such as "ls-refs" behaving differently with transfer.hideRefs. See
https://public-inbox.org/git/[email protected]/
for a fix for that regression.

With this all tests pass with GIT_TEST_PROTOCOL_VERSION=0, but fail with GIT_TEST_PROTOCOL_VERSION=[1|2]. That's OK since this is a new test mode, subsequent patches will fix up these test failures.

---
 protocol.c | 13 ++++++++++++-
 t/README   |  4 ++++
 2 files changed, 16 insertions(+), 1 deletion(-)

index 5e636785d1..cb58cbb29a 100644
@@ -17,7 +17,18 @@ static enum protocol_version parse_protocol_version(const char *value)
 enum protocol_version get_protocol_version_config(void)
 {
 	const char *value;
-	if (!git_config_get_string_const("protocol.version", &value)) {
+	const char *git_test_k = "GIT_TEST_PROTOCOL_VERSION";
+	const char *git_test_v = getenv(git_test_k);
+
+	if (git_test_v) {
+		enum protocol_version version = parse_protocol_version(git_test_v);
+
+		if (version == protocol_unknown_version)
+			die("unknown value for %s: %s", git_test_k,
+			    git_test_v);
+
+		return version;
+	} else if (!git_config_get_string_const("protocol.version", &value)) {
 		enum protocol_version version = parse_protocol_version(value);
 
 		if (version == protocol_unknown_version)

index 28711cc508..c5762a92bc 100644
@@ -358,6 +358,10 @@ GIT_TEST_MULTI_PACK_INDEX=<boolean>, when true, forces the multi-pack-
 index to be written after every 'git repack' command, and overrides the
 'core.multiPackIndex' setting to true.
 
+GIT_TEST_PROTOCOL_VERSION=<'protocol.version' config value>, when set,
+runs the test suite with the given protocol.version. E.g. "0", "1" or
+"2".
+
 Naming Tests
 ------------
 

2.20.0.405.gbc1bbc6f85

Thanks, Aevar for looking into this. I haven't looked into detail, but:

- s/where/var/ in your patch 1
- I think that variables should be unset with env --unset instead of sane_unset, because (as far as I can tell) the effects of sane_unset "leak" from test block to test block.
- On my computer, some tests still fail. I have included a patch (patch
1) that makes these tests succeed.
- I noticed that some of the tests fail because fetch-pack doesn't support protocol v2. It turns out that adding support for protocol v2 in fetch-pack wasn't too difficult, so I have done it. This is patch 2.
- With that support added, some of the other tests no longer need the unsetting of the environment variable. This is patch 3.

If you agree with the general direction I'm going in, when you send out another version, I would add patch 2 somewhere near the beginning of the set, and then squash both patch 1 and patch 3 in the G_T_P_V=2 patch.

Jonathan Tan (3):
squash this into your patch
builtin/fetch-pack: support protocol version 2
also squash this into your patch

 builtin/fetch-pack.c          |  9 ++++++---
 t/t5500-fetch-pack.sh         | 13 +++++++------
 t/t5539-fetch-http-shallow.sh | 12 ++++++++----
 t/t5541-http-push-smart.sh    | 10 ++++++++--
 t/t5551-http-fetch-smart.sh   | 23 +++++++++++++++--------
 t/t5570-git-daemon.sh         |  2 +-
 t/t5616-partial-clone.sh      |  3 +--
 7 files changed, 46 insertions(+), 26 deletions(-)

2.19.0.271.gfe8321ec05.dirty

---
 t/t5539-fetch-http-shallow.sh | 12 ++++++++----
 t/t5541-http-push-smart.sh    | 10 ++++++++--
 t/t5551-http-fetch-smart.sh   | 23 +++++++++++++++--------
 t/t5570-git-daemon.sh         |  2 +-
 4 files changed, 32 insertions(+), 15 deletions(-)

index 5fbf67c446..996ce060cd 100755
@@ -67,7 +67,8 @@ test_expect_success 'no shallow lines after receiving ACK ready' '
 		cd clone &&
 		git checkout --orphan newnew &&
 		test_commit new-too &&
-		GIT_TRACE_PACKET="$TRASH_DIRECTORY/trace" git fetch --depth=2 &&
+		GIT_TRACE_PACKET="$TRASH_DIRECTORY/trace" env --unset=GIT_TEST_PROTOCOL_VERSION \
+			git fetch --depth=2 &&
 		grep "fetch-pack< ACK .* ready" ../trace &&
 		! grep "fetch-pack> done" ../trace
 	)

@@ -114,7 +115,8 @@ test_expect_success 'shallow clone exclude tag two' '
 '
 
 test_expect_success 'fetch exclude tag one' '
-	git -C shallow12 fetch --shallow-exclude one origin &&
+	env --unset=GIT_TEST_PROTOCOL_VERSION \
+		git -C shallow12 fetch --shallow-exclude one origin &&
 	git -C shallow12 log --pretty=tformat:%s origin/master >actual &&
 	test_write_lines three two >expected &&
 	test_cmp expected actual

@@ -128,14 +130,16 @@ test_expect_success 'fetching deepen' '
 	test_commit two &&
 	test_commit three &&
 	mv .git "$HTTPD_DOCUMENT_ROOT_PATH/shallow-deepen.git" &&
-	git clone --depth 1 $HTTPD_URL/smart/shallow-deepen.git deepen &&
+	env --unset=GIT_TEST_PROTOCOL_VERSION \
+		git clone --depth 1 $HTTPD_URL/smart/shallow-deepen.git deepen &&
 	mv "$HTTPD_DOCUMENT_ROOT_PATH/shallow-deepen.git" .git &&
 	test_commit four &&
 	git -C deepen log --pretty=tformat:%s master >actual &&
 	echo three >expected &&
 	test_cmp expected actual &&
 	mv .git "$HTTPD_DOCUMENT_ROOT_PATH/shallow-deepen.git" &&
-	git -C deepen fetch --deepen=1 &&
+	env --unset=GIT_TEST_PROTOCOL_VERSION \
+		git -C deepen fetch --deepen=1 &&
 	git -C deepen log --pretty=tformat:%s origin/master >actual &&
 	cat >expected <<-\EOF &&
 	four

index 5475afc052..20f1308578 100755
@@ -45,14 +45,20 @@ test_expect_success 'no empty path components' '
 	# In the URL, add a trailing slash, and see if git appends yet another
 	# slash.
 	cd "$ROOT_PATH" &&
-	git clone $HTTPD_URL/smart/test_repo.git/ test_repo_clone &&
+	# Other protocol versions may make different requests, so perform this
+	# clone with protocol v0.
+	env --unset=GIT_TEST_PROTOCOL_VERSION \
+		git clone $HTTPD_URL/smart/test_repo.git/ test_repo_clone &&
 
 	check_access_log exp
 '
 
 test_expect_success 'clone remote repository' '
 	rm -rf test_repo_clone &&
-	git clone $HTTPD_URL/smart/test_repo.git test_repo_clone &&
+	# Other protocol versions may make different requests, so perform this
+	# clone with protocol v0.
+	env --unset=GIT_TEST_PROTOCOL_VERSION \
+		git clone $HTTPD_URL/smart/test_repo.git test_repo_clone &&
 	(
 		cd test_repo_clone && git config push.default matching
 	)

index 8630b0cc39..56fd9351a6 100755
@@ -43,7 +43,8 @@ test_expect_success 'clone http repository' '
 	< Cache-Control: no-cache, max-age=0, must-revalidate
 	< Content-Type: application/x-git-upload-pack-result
 	EOF
-	GIT_TRACE_CURL=true git clone --quiet $HTTPD_URL/smart/repo.git clone 2>err &&
+	GIT_TRACE_CURL=true env --unset=GIT_TEST_PROTOCOL_VERSION \
+		git clone --quiet $HTTPD_URL/smart/repo.git clone 2>err &&
 	test_cmp file clone/file &&
 	tr '\''\015'\'' Q <err |
 	sed -e "

@@ -92,7 +93,7 @@ test_expect_success 'fetch changes via http' '
 	echo content >>file &&
 	git commit -a -m two &&
 	git push public &&
-	(cd clone && git pull) &&
+	(cd clone && env --unset=GIT_TEST_PROTOCOL_VERSION git pull) &&
 	test_cmp file clone/file
 '
 

@@ -143,7 +144,8 @@ test_expect_success 'clone from auth-only-for-push repository' '
 test_expect_success 'clone from auth-only-for-objects repository' '
 	echo two >expect &&
 	set_askpass user@host pass@host &&
-	git clone --bare "$HTTPD_URL/auth-fetch/smart/repo.git" half-auth &&
+	env --unset=GIT_TEST_PROTOCOL_VERSION \
+		git clone --bare "$HTTPD_URL/auth-fetch/smart/repo.git" half-auth &&
 	expect_askpass both user@host &&
 	git --git-dir=half-auth log -1 --format=%s >actual &&
 	test_cmp expect actual

@@ -151,7 +153,8 @@ test_expect_success 'clone from auth-only-for-objects repository' '
 
 test_expect_success 'no-op half-auth fetch does not require a password' '
 	set_askpass wrong &&
-	git --git-dir=half-auth fetch &&
+	env --unset=GIT_TEST_PROTOCOL_VERSION \
+		git --git-dir=half-auth fetch &&
 	expect_askpass none
 '
 

@@ -187,7 +190,8 @@ test_expect_success 'create namespaced refs' '
 '
 
 test_expect_success 'smart clone respects namespace' '
-	git clone "$HTTPD_URL/smart_namespace/repo.git" ns-smart &&
+	env --unset=GIT_TEST_PROTOCOL_VERSION \
+		git clone "$HTTPD_URL/smart_namespace/repo.git" ns-smart &&
 	echo namespaced >expect &&
 	git --git-dir=ns-smart/.git log -1 --format=%s >actual &&
 	test_cmp expect actual

@@ -214,7 +218,8 @@ test_expect_success 'cookies stored in http.cookiefile when http.savecookies set
 	EOF
 	git config http.cookiefile cookies.txt &&
 	git config http.savecookies true &&
-	git ls-remote $HTTPD_URL/smart_cookies/repo.git master &&
+	env --unset=GIT_TEST_PROTOCOL_VERSION \
+		git ls-remote $HTTPD_URL/smart_cookies/repo.git master &&
 	tail -3 cookies.txt | sort >cookies_tail.txt &&
 	test_cmp expect_cookies.txt cookies_tail.txt
 '

@@ -306,7 +311,8 @@ test_expect_success 'test allowreachablesha1inwant with unreachable' '
 
 	git init --bare test_reachable.git &&
 	git -C test_reachable.git remote add origin "$HTTPD_URL/smart/repo.git" &&
-	test_must_fail git -C test_reachable.git fetch origin "$(git rev-parse HEAD)"
+	test_must_fail env --unset=GIT_TEST_PROTOCOL_VERSION \
+		git -C test_reachable.git fetch origin "$(git rev-parse HEAD)"
 '
 
 test_expect_success 'test allowanysha1inwant with unreachable' '

@@ -325,7 +331,8 @@ test_expect_success 'test allowanysha1inwant with unreachable' '
 
 	git init --bare test_reachable.git &&
 	git -C test_reachable.git remote add origin "$HTTPD_URL/smart/repo.git" &&
-	test_must_fail git -C test_reachable.git fetch origin "$(git rev-parse HEAD)" &&
+	test_must_fail env --unset=GIT_TEST_PROTOCOL_VERSION \
+		git -C test_reachable.git fetch origin "$(git rev-parse HEAD)" &&
 
 	git -C "$server" config uploadpack.allowanysha1inwant 1 &&
 	git -C test_reachable.git fetch origin "$(git rev-parse HEAD)"

index 7466aad111..4a2a937bb0 100755
@@ -190,7 +190,7 @@ test_expect_success 'daemon log records all attributes' '
 	EOF
 	>daemon.log &&
 	GIT_OVERRIDE_VIRTUAL_HOST=localhost \
-		git -c protocol.version=1 \
+		env --unset=GIT_TEST_PROTOCOL_VERSION git -c protocol.version=1 \
 			ls-remote "$GIT_DAEMON_URL/interp.git" &&
 	grep -i extended.attribute daemon.log | cut -d" " -f2- >actual &&
 	test_cmp expect actual

2.19.0.271.gfe8321ec05.dirty

Currently, if support for running Git's entire test suite with protocol v2 were added, some tests would fail because the fetch-pack CLI command dies if it encounters protocol v2. To avoid this, teach fetch-pack support for protocol v2.

---
 builtin/fetch-pack.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

index 63e69a5801..f6a513495e 100644
@@ -55,6 +55,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 	struct oid_array shallow = OID_ARRAY_INIT;
 	struct string_list deepen_not = STRING_LIST_INIT_DUP;
 	struct packet_reader reader;
+	enum protocol_version version;
 
 	fetch_if_missing = 0;
 

@@ -219,9 +220,11 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 			   PACKET_READ_CHOMP_NEWLINE |
 			   PACKET_READ_GENTLE_ON_EOF);
 
-	switch (discover_version(&reader)) {
+	version = discover_version(&reader);
+	switch (version) {
 	case protocol_v2:
-		die("support for protocol v2 not implemented yet");
+		get_remote_refs(fd[1], &reader, &ref, 0, NULL, NULL);
+		break;
 	case protocol_v1:
 	case protocol_v0:
 		get_remote_heads(&reader, &ref, 0, NULL, &shallow);

@@ -231,7 +234,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 	}
 
 	ref = fetch_pack(&args, fd, conn, ref, dest, sought, nr_sought,
-			 &shallow, pack_lockfile_ptr, protocol_v0);
+			 &shallow, pack_lockfile_ptr, version);
 	if (pack_lockfile) {
 		printf("lock %s\n", pack_lockfile);
 		fflush(stdout);

2.19.0.271.gfe8321ec05.dirty

---
 t/t5500-fetch-pack.sh    | 13 +++++++------
 t/t5616-partial-clone.sh |  3 +--
 2 files changed, 8 insertions(+), 8 deletions(-)

index 9c18875c9c..a5a8f348a2 100755
@@ -41,8 +41,7 @@ pull_to_client () {
 	test_expect_success "$number pull" '
 		(
 			cd client &&
-			GIT_TEST_PROTOCOL_VERSION=0 \
-				git fetch-pack -k -v .. $heads &&
+			git fetch-pack -k -v .. $heads &&
 
 			case "$heads" in
 			    *A*)

@@ -441,7 +440,6 @@ test_expect_success 'setup tests for the --stdin parameter' '
 '
 
 test_expect_success 'fetch refs from cmdline' '
-	sane_unset GIT_TEST_PROTOCOL_VERSION &&
 	(
 		cd client &&
 		git fetch-pack --no-progress .. $(cat ../input)

@@ -630,7 +628,10 @@ test_expect_success 'fetch-pack cannot fetch a raw sha1 that is not advertised a
 	test_commit -C server 6 &&
 
 	git init client &&
-	test_must_fail git -C client fetch-pack ../server \
+
+	# Other protocol versions (e.g. 2) allow fetching an unadvertised
+	# object, so run this test with the default protocol version (0).
+	test_must_fail env --unset=GIT_TEST_PROTOCOL_VERSION git -C client fetch-pack ../server \
 		$(git -C server rev-parse refs/heads/master^) 2>err &&
 	test_i18ngrep "Server does not allow request for unadvertised object" err
 '

@@ -790,7 +791,7 @@ test_expect_success 'shallow clone exclude tag two' '
 '
 
 test_expect_success 'fetch exclude tag one' '
-	git -C shallow12 fetch --shallow-exclude one origin &&
+	env --unset=GIT_TEST_PROTOCOL_VERSION git -C shallow12 fetch --shallow-exclude one origin &&
 	git -C shallow12 log --pretty=tformat:%s origin/master >actual &&
 	test_write_lines three two >expected &&
 	test_cmp expected actual

@@ -808,7 +809,7 @@ test_expect_success 'fetching deepen' '
 	git -C deepen log --pretty=tformat:%s master >actual &&
 	echo three >expected &&
 	test_cmp expected actual &&
-	git -C deepen fetch --deepen=1 &&
+	env --unset=GIT_TEST_PROTOCOL_VERSION git -C deepen fetch --deepen=1 &&
 	git -C deepen log --pretty=tformat:%s origin/master >actual &&
 	cat >expected <<-\EOF &&
 	four

index feedf84ce1..336f02a41a 100755
@@ -144,8 +144,7 @@ test_expect_success 'manual prefetch of missing objects' '
 	sort >observed.oids &&
 
 	test_line_count = 6 observed.oids &&
-	GIT_TEST_PROTOCOL_VERSION=0 \
-		git -C pc1 fetch-pack --stdin "file://$(pwd)/srv.bare" <observed.oids &&
+	git -C pc1 fetch-pack --stdin "file://$(pwd)/srv.bare" <observed.oids &&
 
 	git -C pc1 rev-list --quiet --objects --missing=print \
 		master..origin/master >revs &&

2.19.0.271.gfe8321ec05.dirty

Thanks, both. I'll let these sit in the list archive and expect an updated series ready to be picked up.

I figured it would be easier for everyone if I rolled this all into one series instead of Junio & us needing to keep track of what's based on what.

The only change I made to Jeff's patches is my SOB and adding a paragraph to the end of his 3/3 saying that the v2 push protocol doesn't have the same issue (because it doesn't exist yet). I had that question in this thread, and thought it was useful to clarify it.

No changes to Jonathan's one patch, except my SOB.

For the rest I incorporated Jonathan's suggestions / fixes with some amendments. The suggestion to use env --unset isn't portable (and there's now a check for that while we're at it), so instead we support "GIT_TEST_PROTOCOL_VERSION=" which'll ignore the environment value.

Other changes in my patches are more narrowly skipping tests, i.e. no "unset" anymore except for those tests where we're only doing v1 and v2 tests. I also removed the "env" use in those cases that don't need it (where we use e.g. test_must_fail), instead we just set the env variable ourselves with native shell syntax.

Jeff King (3):
serve: pass "config context" through to individual commands
parse_hide_refs_config: handle NULL section
upload-pack: support hidden refs with protocol v2

Jonathan Tan (1):
builtin/fetch-pack: support protocol version 2

Ævar Arnfjörð Bjarmason (4):
tests: add a check for unportable env --unset
tests: add a special setup where for protocol.version
tests: mark & fix tests broken under GIT_TEST_PROTOCOL_VERSION=1
tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2

 builtin/fetch-pack.c                 |  9 ++++++---
 builtin/upload-pack.c                |  1 +
 ls-refs.c                            | 16 +++++++++++++++-
 ls-refs.h                            |  3 ++-
 protocol.c                           | 13 ++++++++++++-
 refs.c                               |  3 ++-
 serve.c                              |  9 +++++----
 serve.h                              |  7 +++++++
 t/README                             |  6 ++++++
 t/check-non-portable-shell.pl        |  1 +
 t/t0410-partial-clone.sh             |  3 ++-
 t/t5400-send-pack.sh                 |  2 +-
 t/t5500-fetch-pack.sh                |  9 ++++++---
 t/t5503-tagfollow.sh                 |  8 ++++----
 t/t5512-ls-remote.sh                 | 14 ++++++++++----
 t/t5515-fetch-merge-logic.sh         |  2 +-
 t/t5516-fetch-push.sh                | 20 +++++++++++++-------
 t/t5537-fetch-shallow.sh             |  3 ++-
 t/t5539-fetch-http-shallow.sh        |  9 +++++----
 t/t5541-http-push-smart.sh           |  9 +++++++--
 t/t5551-http-fetch-smart.sh          | 19 +++++++++++--------
 t/t5552-skipping-fetch-negotiator.sh |  4 ++--
 t/t5570-git-daemon.sh                |  2 +-
 t/t5601-clone.sh                     | 11 +++++++++--
 t/t5700-protocol-v1.sh               |  1 +
 t/t5702-protocol-v2.sh               |  1 +
 t/t7406-submodule-update.sh          |  3 ++-
 upload-pack.c                        |  4 ++--
 upload-pack.h                        |  4 ++--
 29 files changed, 139 insertions(+), 57 deletions(-)

2.20.0.405.gbc1bbc6f85

The v2 of this series conflicted with Josh Steadmon's work when merged in "pu". That's still outstanding, see
https://public-inbox.org/git/[email protected]/

Then my just-sent
https://public-inbox.org/git/[email protected]/
conflicts with even more things in it.

So I'm dropping "GIT_TEST_PROTOCOL_VERSION" for now until things settle down. That can land after all this protocol activity settles.

No changes to Jeff's patches since v2, for Jonathan's no changes to the C code, but I added a test which I extracted from the GIT_TEST_PROTOCOL_VERSION=2 work.

Jeff King (3):
serve: pass "config context" through to individual commands
parse_hide_refs_config: handle NULL section
upload-pack: support hidden refs with protocol v2

Jonathan Tan (1):
fetch-pack: support protocol version 2

 builtin/fetch-pack.c  |  9 ++++++---
 builtin/upload-pack.c |  1 +
 ls-refs.c             | 16 +++++++++++++++-
 ls-refs.h             |  3 ++-
 refs.c                |  3 ++-
 serve.c               |  9 +++++----
 serve.h               |  7 +++++++
 t/t5500-fetch-pack.sh | 22 +++++++++++++++-------
 t/t5512-ls-remote.sh  |  6 ++++++
 upload-pack.c         |  4 ++--
 upload-pack.h         |  4 ++--
 11 files changed, 63 insertions(+), 21 deletions(-)

2.20.0.405.gbc1bbc6f85

These should be dropped in favor of:

https://public-inbox.org/git/[email protected]/

that I just sent (I tweaked the commit message to address the questions you had in push for patch 3).

-Peff

From: Jeff King <[email protected]>

In protocol v2, instead of just running "upload-pack", we have a generic "serve" loop which runs command requests from the client. What used to be "upload-pack" is now generally split into two operations: "ls-refs" and "fetch". The latter knows it must respect uploadpack.* config, but the former is not actually specific to a fetch operation (we do not yet do v2 receive-pack, but eventually we may, and ls-refs would support both operations).

However, ls-refs does need to know which operation we're performing, in order to read the correct config (e.g., uploadpack.hiderefs versus receive.hiderefs; we don't read _either_ right now, but this is the first step to fixing that).

In the generic "git-serve" program, we don't have that information. But in the protocol as it is actually used by clients, the client still asks to run "git-upload-pack", and then issues an "ls-refs" from there. So we _do_ know at that point that "uploadpack" is the right config context. This patch teaches the protocol v2 "serve" code to pass that context through to the individual commands (a future patch will teach ls-refs to actually use it).

---
 builtin/upload-pack.c | 1 +
 ls-refs.c             | 4 +++-
 ls-refs.h             | 3 ++-
 serve.c               | 9 +++++----
 serve.h               | 7 +++++++
 upload-pack.c         | 4 ++--
 upload-pack.h         | 4 ++--
 7 files changed, 22 insertions(+), 10 deletions(-)

index 42dc4da5a1f..67192cfa9e9 100644
@@ -52,6 +52,7 @@ int cmd_upload_pack(int argc, const char **argv, const char *prefix)
 	case protocol_v2:
 		serve_opts.advertise_capabilities = opts.advertise_refs;
 		serve_opts.stateless_rpc = opts.stateless_rpc;
+		serve_opts.config_context = "uploadpack";
 		serve(&serve_opts);
 		break;
 	case protocol_v1:

index a06f12eca8d..e8e31475f06 100644
@@ -69,7 +69,9 @@ static int send_ref(const char *refname, const struct object_id *oid,
 	return 0;
 }
 
-int ls_refs(struct repository *r, struct argv_array *keys,
+int ls_refs(struct repository *r,
+	    const char *config_section,
+	    struct argv_array *keys,
 	    struct packet_reader *request)
 {
 	struct ls_refs_data data;

index b62877e8dad..da26fc9824d 100644
@@ -4,7 +4,8 @@
 struct repository;
 struct argv_array;
 struct packet_reader;
-extern int ls_refs(struct repository *r, struct argv_array *keys,
+extern int ls_refs(struct repository *r, const char *config_context,
+		   struct argv_array *keys,
 		   struct packet_reader *request);
 
 #endif /* LS_REFS_H */

index bda085f09c8..70f89cf0d98 100644
@@ -48,6 +48,7 @@ struct protocol_capability {
 	 * This field should be NULL for capabilities which are not commands.
 	 */
 	int (*command)(struct repository *r,
+		       const char *config_context,
 		       struct argv_array *keys,
 		       struct packet_reader *request);
 };

@@ -158,7 +159,7 @@ enum request_state {
 	PROCESS_REQUEST_DONE,
 };
 
-static int process_request(void)
+static int process_request(struct serve_options *opts)
 {
 	enum request_state state = PROCESS_REQUEST_KEYS;
 	struct packet_reader reader;

@@ -222,7 +223,7 @@ static int process_request(void)
 	if (!command)
 		die("no command requested");
 
-	command->command(the_repository, &keys, &reader);
+	command->command(the_repository, opts->config_context, &keys, &reader);
 
 	argv_array_clear(&keys);
 	return 0;

@@ -249,10 +250,10 @@ void serve(struct serve_options *options)
 	 * a single request/response exchange
 	 */
 	if (options->stateless_rpc) {
-		process_request();
+		process_request(options);
 	} else {
 		for (;;)
-			if (process_request())
+			if (process_request(options))
 				break;
 	}
 }

index fe65ba9f469..d527224cbb1 100644
@@ -8,6 +8,13 @@ extern int has_capability(const struct argv_array *keys, const char *capability,
 struct serve_options {
 	unsigned advertise_capabilities;
 	unsigned stateless_rpc;
+
+	/*
+	 * Some operations may need to know the context when looking up config;
+	 * e.g., set this to "uploadpack" to respect "uploadpack.hiderefs" (as
+	 * opposed to "receive.hiderefs").
+	 */
+	const char *config_context;
 };
 #define SERVE_OPTIONS_INIT { 0 }
 extern void serve(struct serve_options *options);

index 5e81f1ff24f..914bbb40bcd 100644
@@ -1413,8 +1413,8 @@ enum fetch_state {
 	FETCH_DONE,
 };
 
-int upload_pack_v2(struct repository *r, struct argv_array *keys,
-		   struct packet_reader *request)
+int upload_pack_v2(struct repository *r, const char *config_context,
+		   struct argv_array *keys, struct packet_reader *request)
 {
 	enum fetch_state state = FETCH_PROCESS_ARGS;
 	struct upload_pack_data data;

index cab2178796a..2a9f51197c4 100644
@@ -13,8 +13,8 @@ void upload_pack(struct upload_pack_options *options);
 struct repository;
 struct argv_array;
 struct packet_reader;
-extern int upload_pack_v2(struct repository *r, struct argv_array *keys,
-			  struct packet_reader *request);
+extern int upload_pack_v2(struct repository *r, const char *config_context,
+			  struct argv_array *keys, struct packet_reader *request);
 
 struct strbuf;
 extern int upload_pack_advertise(struct repository *r,

2.20.0.405.gbc1bbc6f85

From: Jeff King <[email protected]>

This helper function looks for config in two places: transfer.hiderefs, or $section.hiderefs, where $section is passed in by the caller (and is "uploadpack" or "receive", depending on the context).

In preparation for callers which do not even have that context (namely the "git-serve" command), let's handle a NULL by looking only at transfer.hiderefs (as opposed to segfaulting).

---
 refs.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index f9936355cda..099e91d9cc0 100644
@@ -1267,7 +1267,8 @@ int parse_hide_refs_config(const char *var, const char *value, const char *secti
 {
 	const char *key;
 	if (!strcmp("transfer.hiderefs", var) ||
-	    (!parse_config_key(var, section, NULL, NULL, &key) &&
+	    (section &&
+	     !parse_config_key(var, section, NULL, NULL, &key) &&
 	     !strcmp(key, "hiderefs"))) {
 		char *ref;
 		int len;

2.20.0.405.gbc1bbc6f85

From: Jeff King <[email protected]>

In the v2 protocol, upload-pack's advertisement has been moved to the "ls-refs" command. That command does not respect hidden-ref config (like transfer.hiderefs) at all, and advertises everything.

While there are some features that are not supported in v2 (e.g., v2 always allows fetching any sha1 without respect to advertisements), the lack of this feature is not documented and is likely just a bug. Let's make it work, as otherwise upgrading a server to a v2-capable git will start exposing these refs that the repository admin has asked to remain hidden.

Note that we depend on the config_context from the caller here to realize that we should respect uploadpack.hiderefs. When run from the "git-serve" tool, we won't have that context and will only respect transfer.hiderefs. This should be OK, as git-serve is not actually used for normal protocol operations.

Note also that we don't need to worry about the "An attempt to update or delete a hidden ref by git push is rejected" feature of receive.hideRefs, see git-config(1). This is because there's no v2 push protocol yet.

---
 ls-refs.c            | 12 ++++++++++++
 t/t5512-ls-remote.sh |  6 ++++++
 2 files changed, 18 insertions(+)

index e8e31475f06..8a8143338b4 100644
@@ -5,6 +5,7 @@
 #include "argv-array.h"
 #include "ls-refs.h"
 #include "pkt-line.h"
+#include "config.h"
 
 /*
  * Check if one of the prefixes is a prefix of the ref.

@@ -40,6 +41,9 @@ static int send_ref(const char *refname, const struct object_id *oid,
 	const char *refname_nons = strip_namespace(refname);
 	struct strbuf refline = STRBUF_INIT;
 
+	if (ref_is_hidden(refname_nons, refname))
+		return 0;
+
 	if (!ref_match(&data->prefixes, refname))
 		return 0;
 

@@ -69,6 +73,12 @@ static int send_ref(const char *refname, const struct object_id *oid,
 	return 0;
 }
 
+static int ls_refs_config(const char *var, const char *value,
+			  void *config_context)
+{
+	return parse_hide_refs_config(var, value, config_context);
+}
+
 int ls_refs(struct repository *r,
 	    const char *config_section,
 	    struct argv_array *keys,

@@ -78,6 +88,8 @@ int ls_refs(struct repository *r,
 
 	memset(&data, 0, sizeof(data));
 
+	git_config(ls_refs_config, (void *)config_section);
+
 	while (packet_reader_read(request) != PACKET_READ_FLUSH) {
 		const char *arg = request->line;
 		const char *out;

index 32e722db2ed..ca69636fd52 100755
@@ -204,6 +204,12 @@ test_expect_success 'overrides work between mixed transfer/upload-pack hideRefs'
 	grep refs/tags/magic actual
 '
 
+test_expect_success 'protocol v2 supports hiderefs' '
+	test_config uploadpack.hiderefs refs/tags &&
+	git -c protocol.version=2 ls-remote . >actual &&
+	! grep refs/tags actual
+'
+
 test_expect_success 'ls-remote --symref' '
 	git fetch origin &&
 	cat >expect <<-EOF &&

2.20.0.405.gbc1bbc6f85

From: Jonathan Tan <[email protected]>

When the scaffolding for protocol version 2 was initially added in 8f6982b4e1 ("protocol: introduce enum protocol_version value protocol_v2", 2018-03-14). As seen in:

    git log -p -G'support for protocol v2 not implemented yet' --full-diff --reverse v2.17.0..v2.20.0

Many of those scaffolding "die" placeholders were removed, but we hadn't gotten around to fetch-pack yet.

The test here for "fetch refs from cmdline" is very minimal. There's much better coverage when running the entire test suite under the WIP GIT_TEST_PROTOCOL_VERSION=2 mode[1], we should ideally have better coverage without needing to invoke a special test mode.

1. https://public-inbox.org/git/[email protected]/

---
 builtin/fetch-pack.c  |  9 ++++++---
 t/t5500-fetch-pack.sh | 22 +++++++++++++++-------
 2 files changed, 21 insertions(+), 10 deletions(-)

index 63e69a58011..f6a513495ea 100644
@@ -55,6 +55,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 	struct oid_array shallow = OID_ARRAY_INIT;
 	struct string_list deepen_not = STRING_LIST_INIT_DUP;
 	struct packet_reader reader;
+	enum protocol_version version;
 
 	fetch_if_missing = 0;
 

@@ -219,9 +220,11 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 			   PACKET_READ_CHOMP_NEWLINE |
 			   PACKET_READ_GENTLE_ON_EOF);
 
-	switch (discover_version(&reader)) {
+	version = discover_version(&reader);
+	switch (version) {
 	case protocol_v2:
-		die("support for protocol v2 not implemented yet");
+		get_remote_refs(fd[1], &reader, &ref, 0, NULL, NULL);
+		break;
 	case protocol_v1:
 	case protocol_v0:
 		get_remote_heads(&reader, &ref, 0, NULL, &shallow);

@@ -231,7 +234,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 	}
 
 	ref = fetch_pack(&args, fd, conn, ref, dest, sought, nr_sought,
-			 &shallow, pack_lockfile_ptr, protocol_v0);
+			 &shallow, pack_lockfile_ptr, version);
 	if (pack_lockfile) {
 		printf("lock %s\n", pack_lockfile);
 		fflush(stdout);

index 086f2c40f68..49c540b1e1d 100755
@@ -439,15 +439,23 @@ test_expect_success 'setup tests for the --stdin parameter' '
 	) >input.dup
 '
 
-test_expect_success 'fetch refs from cmdline' '
-	(
-		cd client &&
-		git fetch-pack --no-progress .. $(cat ../input)
-	) >output &&
-	cut -d " " -f 2 <output | sort >actual &&
-	test_cmp expect actual
+test_expect_success 'setup fetch refs from cmdline v[12]' '
+	cp -r client client1 &&
+	cp -r client client2
 '
 
+for version in '' 1 2
+do
+	test_expect_success "protocol.version=$version fetch refs from cmdline" "
+		(
+			cd client$version &&
+			GIT_TEST_PROTOCOL_VERSION=$version git fetch-pack --no-progress .. \$(cat ../input)
+		) >output &&
+		cut -d ' ' -f 2 <output | sort >actual &&
+		test_cmp expect actual
+	"
+done
+
 test_expect_success 'fetch refs from stdin' '
 	(
 		cd client &&

2.20.0.405.gbc1bbc6f85

I was looking at the topics in 'pu' and noticed that I had v2 of this series, wanted to update to v3, but major part of it was superseded by another topic (jk/proto-v2-hidden-refs-fix). That leaves only this patch in the v3 of this series.

Is this one still relevant?

This patch is more relevant to the GIT_TEST_PROTOCOL_VERSION patches, since it means that several tests work even if GIT_TEST_PROTOCOL_VERSION=2 is set. I think it can be dropped until someone restarts the GIT_TEST_PROTOCOL_VERSION effort, but I'm not sure if Ævar has another opinion.

I think it's independently useful. If you set protocol.version=2 in your config, then you can no longer run fetch-pack directly. Most people don't, but it's possible their scripts might (we also use fetch-pack under the hood for smart-http, but I wasn't able to get it to complain, though).

-Peff

From: Jeff King <[email protected]>

In protocol v2, instead of just running "upload-pack", we have a generic "serve" loop which runs command requests from the client. What used to be "upload-pack" is now generally split into two operations: "ls-refs" and "fetch". The latter knows it must respect uploadpack.* config, but the former is not actually specific to a fetch operation (we do not yet do v2 receive-pack, but eventually we may, and ls-refs would support both operations).

However, ls-refs does need to know which operation we're performing, in order to read the correct config (e.g., uploadpack.hiderefs versus receive.hiderefs; we don't read _either_ right now, but this is the first step to fixing that).

In the generic "git-serve" program, we don't have that information. But in the protocol as it is actually used by clients, the client still asks to run "git-upload-pack", and then issues an "ls-refs" from there. So we _do_ know at that point that "uploadpack" is the right config context. This patch teaches the protocol v2 "serve" code to pass that context through to the individual commands (a future patch will teach ls-refs to actually use it).

---
 builtin/upload-pack.c | 1 +
 ls-refs.c             | 4 +++-
 ls-refs.h             | 3 ++-
 serve.c               | 9 +++++----
 serve.h               | 7 +++++++
 upload-pack.c         | 4 ++--
 upload-pack.h         | 4 ++--
 7 files changed, 22 insertions(+), 10 deletions(-)

index 42dc4da5a1..67192cfa9e 100644
@@ -52,6 +52,7 @@ int cmd_upload_pack(int argc, const char **argv, const char *prefix)
 	case protocol_v2:
 		serve_opts.advertise_capabilities = opts.advertise_refs;
 		serve_opts.stateless_rpc = opts.stateless_rpc;
+		serve_opts.config_context = "uploadpack";
 		serve(&serve_opts);
 		break;
 	case protocol_v1:

index a06f12eca8..e8e31475f0 100644
@@ -69,7 +69,9 @@ static int send_ref(const char *refname, const struct object_id *oid,
 	return 0;
 }
 
-int ls_refs(struct repository *r, struct argv_array *keys,
+int ls_refs(struct repository *r,
+	    const char *config_section,
+	    struct argv_array *keys,
 	    struct packet_reader *request)
 {
 	struct ls_refs_data data;

index b62877e8da..da26fc9824 100644
@@ -4,7 +4,8 @@
 struct repository;
 struct argv_array;
 struct packet_reader;
-extern int ls_refs(struct repository *r, struct argv_array *keys,
+extern int ls_refs(struct repository *r, const char *config_context,
+		   struct argv_array *keys,
 		   struct packet_reader *request);
 
 #endif /* LS_REFS_H */

index bda085f09c..70f89cf0d9 100644
@@ -48,6 +48,7 @@ struct protocol_capability {
 	 * This field should be NULL for capabilities which are not commands.
 	 */
 	int (*command)(struct repository *r,
+		       const char *config_context,
 		       struct argv_array *keys,
 		       struct packet_reader *request);
 };

@@ -158,7 +159,7 @@ enum request_state {
 	PROCESS_REQUEST_DONE,
 };
 
-static int process_request(void)
+static int process_request(struct serve_options *opts)
 {
 	enum request_state state = PROCESS_REQUEST_KEYS;
 	struct packet_reader reader;

@@ -222,7 +223,7 @@ static int process_request(void)
 	if (!command)
 		die("no command requested");
 
-	command->command(the_repository, &keys, &reader);
+	command->command(the_repository, opts->config_context, &keys, &reader);
 
 	argv_array_clear(&keys);
 	return 0;

@@ -249,10 +250,10 @@ void serve(struct serve_options *options)
 	 * a single request/response exchange
 	 */
 	if (options->stateless_rpc) {
-		process_request();
+		process_request(options);
 	} else {
 		for (;;)
-			if (process_request())
+			if (process_request(options))
 				break;
 	}
 }

index fe65ba9f46..d527224cbb 100644
@@ -8,6 +8,13 @@ extern int has_capability(const struct argv_array *keys, const char *capability,
 struct serve_options {
 	unsigned advertise_capabilities;
 	unsigned stateless_rpc;
+
+	/*
+	 * Some operations may need to know the context when looking up config;
+	 * e.g., set this to "uploadpack" to respect "uploadpack.hiderefs" (as
+	 * opposed to "receive.hiderefs").
+	 */
+	const char *config_context;
 };
 #define SERVE_OPTIONS_INIT { 0 }
 extern void serve(struct serve_options *options);

index 5e81f1ff24..914bbb40bc 100644
@@ -1413,8 +1413,8 @@ enum fetch_state {
 	FETCH_DONE,
 };
 
-int upload_pack_v2(struct repository *r, struct argv_array *keys,
-		   struct packet_reader *request)
+int upload_pack_v2(struct repository *r, const char *config_context,
+		   struct argv_array *keys, struct packet_reader *request)
 {
 	enum fetch_state state = FETCH_PROCESS_ARGS;
 	struct upload_pack_data data;

index cab2178796..2a9f51197c 100644
@@ -13,8 +13,8 @@ void upload_pack(struct upload_pack_options *options);
 struct repository;
 struct argv_array;
 struct packet_reader;
-extern int upload_pack_v2(struct repository *r, struct argv_array *keys,
-			  struct packet_reader *request);
+extern int upload_pack_v2(struct repository *r, const char *config_context,
+			  struct argv_array *keys, struct packet_reader *request);
 
 struct strbuf;
 extern int upload_pack_advertise(struct repository *r,

2.20.0.405.gbc1bbc6f85

From: Jeff King <[email protected]>

This helper function looks for config in two places: transfer.hiderefs, or $section.hiderefs, where $section is passed in by the caller (and is "uploadpack" or "receive", depending on the context).

In preparation for callers which do not even have that context (namely the "git-serve" command), let's handle a NULL by looking only at transfer.hiderefs (as opposed to segfaulting).

---
 refs.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index f9936355cd..099e91d9cc 100644
@@ -1267,7 +1267,8 @@ int parse_hide_refs_config(const char *var, const char *value, const char *secti
 {
 	const char *key;
 	if (!strcmp("transfer.hiderefs", var) ||
-	    (!parse_config_key(var, section, NULL, NULL, &key) &&
+	    (section &&
+	     !parse_config_key(var, section, NULL, NULL, &key) &&
 	     !strcmp(key, "hiderefs"))) {
 		char *ref;
 		int len;

2.20.0.405.gbc1bbc6f85

From: Jeff King <[email protected]>

In the v2 protocol, upload-pack's advertisement has been moved to the "ls-refs" command. That command does not respect hidden-ref config (like transfer.hiderefs) at all, and advertises everything.

While there are some features that are not supported in v2 (e.g., v2 always allows fetching any sha1 without respect to advertisements), the lack of this feature is not documented and is likely just a bug. Let's make it work, as otherwise upgrading a server to a v2-capable git will start exposing these refs that the repository admin has asked to remain hidden.

Note that we depend on the config_context from the caller here to realize that we should respect uploadpack.hiderefs. When run from the "git-serve" tool, we won't have that context and will only respect transfer.hiderefs. This should be OK, as git-serve is not actually used for normal protocol operations.

Note also that we don't need to worry about the "An attempt to update or delete a hidden ref by git push is rejected" feature of receive.hideRefs, see git-config(1). This is because there's no v2 push protocol yet.

---
 ls-refs.c            | 12 ++++++++++++
 t/t5512-ls-remote.sh |  6 ++++++
 2 files changed, 18 insertions(+)

index e8e31475f0..8a8143338b 100644
@@ -5,6 +5,7 @@
 #include "argv-array.h"
 #include "ls-refs.h"
 #include "pkt-line.h"
+#include "config.h"
 
 /*
  * Check if one of the prefixes is a prefix of the ref.

@@ -40,6 +41,9 @@ static int send_ref(const char *refname, const struct object_id *oid,
 	const char *refname_nons = strip_namespace(refname);
 	struct strbuf refline = STRBUF_INIT;
 
+	if (ref_is_hidden(refname_nons, refname))
+		return 0;
+
 	if (!ref_match(&data->prefixes, refname))
 		return 0;
 

@@ -69,6 +73,12 @@ static int send_ref(const char *refname, const struct object_id *oid,
 	return 0;
 }
 
+static int ls_refs_config(const char *var, const char *value,
+			  void *config_context)
+{
+	return parse_hide_refs_config(var, value, config_context);
+}
+
 int ls_refs(struct repository *r,
 	    const char *config_section,
 	    struct argv_array *keys,

@@ -78,6 +88,8 @@ int ls_refs(struct repository *r,
 
 	memset(&data, 0, sizeof(data));
 
+	git_config(ls_refs_config, (void *)config_section);
+
 	while (packet_reader_read(request) != PACKET_READ_FLUSH) {
 		const char *arg = request->line;
 		const char *out;

index 32e722db2e..ca69636fd5 100755
@@ -204,6 +204,12 @@ test_expect_success 'overrides work between mixed transfer/upload-pack hideRefs'
 	grep refs/tags/magic actual
 '
 
+test_expect_success 'protocol v2 supports hiderefs' '
+	test_config uploadpack.hiderefs refs/tags &&
+	git -c protocol.version=2 ls-remote . >actual &&
+	! grep refs/tags actual
+'
+
 test_expect_success 'ls-remote --symref' '
 	git fetch origin &&
 	cat >expect <<-EOF &&

2.20.0.405.gbc1bbc6f85

The "env --unset=*" argument isn't portable. Neither Solaris or AIX have it, and probably not a bunch of other POSIX-like OS's.

Using this was suggested on-list[1]. Let's add a check for it so it doesn't sneak into the codebase in the future.

1. https://public-inbox.org/git/[email protected]/

---
 t/check-non-portable-shell.pl | 1 +
 1 file changed, 1 insertion(+)

index b45bdac688..1cfb4608ee 100755
@@ -35,6 +35,7 @@ sub err {
 		chomp;
 	}
 
+	/\benv (?:-u|--unset)\b/ and err 'env [-u|--unset] is not portable';
 	/\bsed\s+-i/ and err 'sed -i is not portable';
 	/\becho\s+-[neE]/ and err 'echo with option is not portable (use printf)';
 	/^\s*declare\s+/ and err 'arrays/declare not portable';

2.20.0.405.gbc1bbc6f85

Add a GIT_TEST_PROTOCOL_VERSION=X test mode which is equivalent to running with protocol.version=X. This is needed to spot regressions and differences such as "ls-refs" behaving differently with transfer.hideRefs. See
https://public-inbox.org/git/[email protected]/
for a fix for that regression.

With this all tests pass with GIT_TEST_PROTOCOL_VERSION=0, but fail with GIT_TEST_PROTOCOL_VERSION=[1|2]. That's OK since this is a new test mode, subsequent patches will fix up these test failures.

---
 protocol.c               | 13 ++++++++++++-
 t/README                 |  6 ++++++
 t/t0410-partial-clone.sh |  3 ++-
 t/t5516-fetch-push.sh    |  3 ++-
 t/t5700-protocol-v1.sh   |  1 +
 t/t5702-protocol-v2.sh   |  1 +
 6 files changed, 24 insertions(+), 3 deletions(-)

index 5e636785d1..aa06c3b5bc 100644
@@ -17,7 +17,18 @@ static enum protocol_version parse_protocol_version(const char *value)
 enum protocol_version get_protocol_version_config(void)
 {
 	const char *value;
-	if (!git_config_get_string_const("protocol.version", &value)) {
+	const char *git_test_k = "GIT_TEST_PROTOCOL_VERSION";
+	const char *git_test_v = getenv(git_test_k);
+
+	if (git_test_v && strlen(git_test_v)) {
+		enum protocol_version version = parse_protocol_version(git_test_v);
+
+		if (version == protocol_unknown_version)
+			die("unknown value for %s: %s", git_test_k,
+			    git_test_v);
+
+		return version;
+	} else if (!git_config_get_string_const("protocol.version", &value)) {
 		enum protocol_version version = parse_protocol_version(value);
 
 		if (version == protocol_unknown_version)

index 28711cc508..89629c5818 100644
@@ -311,6 +311,12 @@ marked strings" in po/README for details.
 GIT_TEST_SPLIT_INDEX=<boolean> forces split-index mode on the whole
 test suite. Accept any boolean values that are accepted by git-config.
 
+GIT_TEST_PROTOCOL_VERSION=<'protocol.version' config value>, when set,
+runs the test suite with the given protocol.version. E.g. "0", "1" or
+"2". Can be set to the empty string within tests themselves (e.g. "env
+GIT_TEST_PROTOCOL_VERSION= <cmd>") to unset the value in the
+environment as a workaround for "env --unset" not being portable.
+
 GIT_TEST_FULL_IN_PACK_ARRAY=<boolean> exercises the uncommon
 pack-objects code path where there are more than 1024 packs even if
 the actual number of packs in repository is below this limit. Accept

index ba3887f178..8ba3d9b5ab 100755
@@ -178,7 +178,8 @@ test_expect_success 'fetching of missing objects works with ref-in-want enabled'
 
 	rm -rf repo/.git/objects/* &&
 	rm -f trace &&
-	GIT_TRACE_PACKET="$(pwd)/trace" git -C repo cat-file -p "$HASH" &&
+	GIT_TRACE_PACKET="$(pwd)/trace" env GIT_TEST_PROTOCOL_VERSION= \
+		git -C repo cat-file -p "$HASH" &&
 	grep "git< fetch=.*ref-in-want" trace
 '
 

index 37e8e80893..08cdee0b95 100755
@@ -1187,7 +1187,8 @@ test_expect_success 'fetch exact SHA1 in protocol v2' '
 
 	# fetching the hidden object succeeds by default
 	# NEEDSWORK: should this match the v0 behavior instead?
-	git -C child fetch -v ../testrepo $the_commit:refs/heads/copy
+	env GIT_TEST_PROTOCOL_VERSION= \
+		git -C child fetch -v ../testrepo $the_commit:refs/heads/copy
 '
 
 for configallowtipsha1inwant in true false

index ba86a44eb1..e4d375c462 100755
@@ -8,6 +8,7 @@ TEST_NO_CREATE_REPO=1
 
 # Test protocol v1 with 'git://' transport
 #
+unset GIT_TEST_PROTOCOL_VERSION
 . "$TEST_DIRECTORY"/lib-git-daemon.sh
 start_git_daemon --export-all --enable=receive-pack
 daemon_parent=$GIT_DAEMON_DOCUMENT_ROOT_PATH/parent

index 0f2b09ebb8..d1549f294e 100755
@@ -8,6 +8,7 @@ TEST_NO_CREATE_REPO=1
 
 # Test protocol v2 with 'git://' transport
 #
+unset GIT_TEST_PROTOCOL_VERSION
 . "$TEST_DIRECTORY"/lib-git-daemon.sh
 start_git_daemon --export-all --enable=receive-pack
 daemon_parent=$GIT_DAEMON_DOCUMENT_ROOT_PATH/parent

2.20.0.405.gbc1bbc6f85

Regarding the subject, I think you mean "add a special setup var", not "add a special setup where".

There's one t5400-send-pack.sh test which is broken under GIT_TEST_PROTOCOL_VERSION=1. It's easier to just coerce it to run under protocol 1. The difference in the output is that there'll be a "version 1" line which'll make the subsequent "test_cmp". See protocol.version in git-config(1) which notes that "1" is just "0" with a version number. the trace output will include fail.

Similarly in t5601-clone.sh we'll be passing an option to ssh, but since so many tests would fail in this file let's go above & beyond and make them pass by only testing the relevant part of the output.

---
 t/t5400-send-pack.sh |  2 +-
 t/t5601-clone.sh     | 11 +++++++++--
 2 files changed, 10 insertions(+), 3 deletions(-)

index f1932ea431..571d620aed 100755
@@ -288,7 +288,7 @@ test_expect_success 'receive-pack de-dupes .have lines' '
 	$shared .have
 	EOF
 
-	GIT_TRACE_PACKET=$(pwd)/trace \
+	GIT_TRACE_PACKET=$(pwd)/trace GIT_TEST_PROTOCOL_VERSION= \
 	    git push \
 		--receive-pack="unset GIT_TRACE_PACKET; git-receive-pack" \
 		fork HEAD:foo &&

index 8bbc7068ac..a7b7ab327c 100755
@@ -325,7 +325,7 @@ copy_ssh_wrapper_as () {
 
 expect_ssh () {
 	test_when_finished '
-		(cd "$TRASH_DIRECTORY" && rm -f ssh-expect && >ssh-output)
+		(cd "$TRASH_DIRECTORY" && rm -f ssh-expect ssh-output.munged && >ssh-output)
 	' &&
 	{
 		case "$#" in

@@ -341,7 +341,14 @@ expect_ssh () {
 			echo "ssh: $1 $2 git-upload-pack '$3' $4"
 		esac
 	} >"$TRASH_DIRECTORY/ssh-expect" &&
-	(cd "$TRASH_DIRECTORY" && test_cmp ssh-expect ssh-output)
+	(
+		cd "$TRASH_DIRECTORY" &&
+		# We don't care about this trivial difference in
+		# output with GIT_TEST_PROTOCOL_VERSION=[12]
+		sed 's/ssh: -o SendEnv=GIT_PROTOCOL /ssh: /' <ssh-output >ssh-output.munged &&
+		mv ssh-output.munged ssh-output &&
+		test_cmp ssh-expect ssh-output
+	)
 }
 
 test_expect_success 'clone myhost:src uses ssh' '

2.20.0.405.gbc1bbc6f85

From: Jonathan Tan <[email protected]>

Currently, if support for running Git's entire test suite with protocol v2 were added, some tests would fail because the fetch-pack CLI command dies if it encounters protocol v2. To avoid this, teach fetch-pack support for protocol v2.

---
 builtin/fetch-pack.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

index 63e69a5801..f6a513495e 100644
@@ -55,6 +55,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 	struct oid_array shallow = OID_ARRAY_INIT;
 	struct string_list deepen_not = STRING_LIST_INIT_DUP;
 	struct packet_reader reader;
+	enum protocol_version version;
 
 	fetch_if_missing = 0;
 

@@ -219,9 +220,11 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 			   PACKET_READ_CHOMP_NEWLINE |
 			   PACKET_READ_GENTLE_ON_EOF);
 
-	switch (discover_version(&reader)) {
+	version = discover_version(&reader);
+	switch (version) {
 	case protocol_v2:
-		die("support for protocol v2 not implemented yet");
+		get_remote_refs(fd[1], &reader, &ref, 0, NULL, NULL);
+		break;
 	case protocol_v1:
 	case protocol_v0:
 		get_remote_heads(&reader, &ref, 0, NULL, &shallow);

@@ -231,7 +234,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 	}
 
 	ref = fetch_pack(&args, fd, conn, ref, dest, sought, nr_sought,
-			 &shallow, pack_lockfile_ptr, protocol_v0);
+			 &shallow, pack_lockfile_ptr, version);
 	if (pack_lockfile) {
 		printf("lock %s\n", pack_lockfile);
 		fflush(stdout);

2.20.0.405.gbc1bbc6f85

I'm definitely on board with this goal.

Mark those tests that have behavior differences or bugs under protocol.version=2.

Whether or not these tests should exhibit different behavior is outside the scope of this change. Some (such as t5500-fetch-pack.sh) are intentionally different, but others (such as t7406-submodule-update.sh and t5515-fetch-merge-logic.sh) might indicate bugs in the protocol v2 code.

Tracking down which is which is outside the scope of this change. Let's first exhaustively annotate where the differences are, so that we can spot future behavior differences or regressions.

---
 t/t5500-fetch-pack.sh                |  9 ++++++---
 t/t5503-tagfollow.sh                 |  8 ++++----
 t/t5512-ls-remote.sh                 |  8 ++++----
 t/t5515-fetch-merge-logic.sh         |  2 +-
 t/t5516-fetch-push.sh                | 17 +++++++++++------
 t/t5537-fetch-shallow.sh             |  3 ++-
 t/t5539-fetch-http-shallow.sh        |  9 +++++----
 t/t5541-http-push-smart.sh           |  9 +++++++--
 t/t5551-http-fetch-smart.sh          | 19 +++++++++++--------
 t/t5552-skipping-fetch-negotiator.sh |  4 ++--
 t/t5570-git-daemon.sh                |  2 +-
 t/t7406-submodule-update.sh          |  3 ++-
 12 files changed, 56 insertions(+), 37 deletions(-)

index 086f2c40f6..8b1217ea26 100755
@@ -628,7 +628,10 @@ test_expect_success 'fetch-pack cannot fetch a raw sha1 that is not advertised a
 	test_commit -C server 6 &&
 
 	git init client &&
-	test_must_fail git -C client fetch-pack ../server \
+
+	# Other protocol versions (e.g. 2) allow fetching an unadvertised
+	# object, so run this test with the default protocol version (0).
+	test_must_fail env GIT_TEST_PROTOCOL_VERSION= git -C client fetch-pack ../server \
 		$(git -C server rev-parse refs/heads/master^) 2>err &&
 	test_i18ngrep "Server does not allow request for unadvertised object" err
 '

@@ -788,7 +791,7 @@ test_expect_success 'shallow clone exclude tag two' '
 '
 
 test_expect_success 'fetch exclude tag one' '
-	git -C shallow12 fetch --shallow-exclude one origin &&
+	GIT_TEST_PROTOCOL_VERSION= git -C shallow12 fetch --shallow-exclude one origin &&
 	git -C shallow12 log --pretty=tformat:%s origin/master >actual &&
 	test_write_lines three two >expected &&
 	test_cmp expected actual

@@ -806,7 +809,7 @@ test_expect_success 'fetching deepen' '
 	git -C deepen log --pretty=tformat:%s master >actual &&
 	echo three >expected &&
 	test_cmp expected actual &&
-	git -C deepen fetch --deepen=1 &&
+	GIT_TEST_PROTOCOL_VERSION= git -C deepen fetch --deepen=1 &&
 	git -C deepen log --pretty=tformat:%s origin/master >actual &&
 	cat >expected <<-\EOF &&
 	four

index 4ca48f0276..0e90a90294 100755
@@ -56,7 +56,7 @@ test_expect_success 'fetch A (new commit : 1 connection)' '
 	rm -f $U &&
 	(
 		cd cloned &&
-		GIT_TRACE_PACKET=$UPATH git fetch &&
+		GIT_TRACE_PACKET=$UPATH GIT_TEST_PROTOCOL_VERSION= git fetch &&
 		test $A = $(git rev-parse --verify origin/master)
 	) &&
 	get_needs $U >actual &&

@@ -86,7 +86,7 @@ test_expect_success 'fetch C, T (new branch, tag : 1 connection)' '
 	rm -f $U &&
 	(
 		cd cloned &&
-		GIT_TRACE_PACKET=$UPATH git fetch &&
+		GIT_TRACE_PACKET=$UPATH GIT_TEST_PROTOCOL_VERSION= git fetch &&
 		test $C = $(git rev-parse --verify origin/cat) &&
 		test $T = $(git rev-parse --verify tag1) &&
 		test $A = $(git rev-parse --verify tag1^0)

@@ -122,7 +122,7 @@ test_expect_success 'fetch B, S (commit and tag : 1 connection)' '
 	rm -f $U &&
 	(
 		cd cloned &&
-		GIT_TRACE_PACKET=$UPATH git fetch &&
+		GIT_TRACE_PACKET=$UPATH GIT_TEST_PROTOCOL_VERSION= git fetch &&
 		test $B = $(git rev-parse --verify origin/master) &&
 		test $B = $(git rev-parse --verify tag2^0) &&
 		test $S = $(git rev-parse --verify tag2)

@@ -146,7 +146,7 @@ test_expect_success 'new clone fetch master and tags' '
 		cd clone2 &&
 		git init &&
 		git remote add origin .. &&
-		GIT_TRACE_PACKET=$UPATH git fetch &&
+		GIT_TRACE_PACKET=$UPATH GIT_TEST_PROTOCOL_VERSION= git fetch &&
 		test $B = $(git rev-parse --verify origin/master) &&
 		test $S = $(git rev-parse --verify tag2) &&
 		test $B = $(git rev-parse --verify tag2^0) &&

index ca69636fd5..7b480587c9 100755
@@ -223,7 +223,7 @@ test_expect_success 'ls-remote --symref' '
 	$(git rev-parse refs/tags/mark1.10)	refs/tags/mark1.10
 	$(git rev-parse refs/tags/mark1.2)	refs/tags/mark1.2
 	EOF
-	git ls-remote --symref >actual &&
+	GIT_TEST_PROTOCOL_VERSION= git ls-remote --symref >actual &&
 	test_cmp expect actual
 '
 

@@ -243,7 +243,7 @@ test_expect_failure 'ls-remote with filtered symref (--heads)' '
 	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/heads/foo
 	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/heads/master
 	EOF
-	git ls-remote --symref --heads . >actual &&
+	GIT_TEST_PROTOCOL_VERSION= git ls-remote --symref --heads . >actual &&
 	test_cmp expect actual
 '
 

@@ -252,9 +252,9 @@ test_expect_success 'ls-remote --symref omits filtered-out matches' '
 	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/heads/foo
 	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/heads/master
 	EOF
-	git ls-remote --symref --heads . >actual &&
+	GIT_TEST_PROTOCOL_VERSION=  git ls-remote --symref --heads . >actual &&
 	test_cmp expect actual &&
-	git ls-remote --symref . "refs/heads/*" >actual &&
+	GIT_TEST_PROTOCOL_VERSION= git ls-remote --symref . "refs/heads/*" >actual &&
 	test_cmp expect actual
 '
 

index 36b0dbc01c..f095555c3e 100755
@@ -147,7 +147,7 @@ do
 			do
 				git update-ref -d "$refname" "$val"
 			done
-			git fetch "$@" >/dev/null
+			GIT_TEST_PROTOCOL_VERSION= git fetch "$@" >/dev/null
 			cat .git/FETCH_HEAD
 		} >"$actual_f" &&
 		git show-ref >"$actual_r" &&

index 08cdee0b95..1d1b717cd5 100755
@@ -1129,7 +1129,7 @@ do
 	'
 done
 
-test_expect_success 'fetch exact SHA1' '
+test_expect_success 'fetch exact SHA1 in protocol v0' '
 	mk_test testrepo heads/master hidden/one &&
 	git push testrepo master:refs/hidden/one &&
 	(

@@ -1148,7 +1148,8 @@ test_expect_success 'fetch exact SHA1' '
 		test_must_fail git cat-file -t $the_commit &&
 
 		# fetching the hidden object should fail by default
-		test_must_fail git fetch -v ../testrepo $the_commit:refs/heads/copy 2>err &&
+		test_must_fail env GIT_TEST_PROTOCOL_VERSION= \
+			git fetch -v ../testrepo $the_commit:refs/heads/copy 2>err &&
 		test_i18ngrep "Server does not allow request for unadvertised object" err &&
 		test_must_fail git rev-parse --verify refs/heads/copy &&
 

@@ -1205,7 +1206,8 @@ do
 		mk_empty shallow &&
 		(
 			cd shallow &&
-			test_must_fail git fetch --depth=1 ../testrepo/.git $SHA1 &&
+			test_must_fail env GIT_TEST_PROTOCOL_VERSION= \
+				git fetch --depth=1 ../testrepo/.git $SHA1 &&
 			git --git-dir=../testrepo/.git config uploadpack.allowreachablesha1inwant true &&
 			git fetch --depth=1 ../testrepo/.git $SHA1 &&
 			git cat-file commit $SHA1

@@ -1233,15 +1235,18 @@ do
 		mk_empty shallow &&
 		(
 			cd shallow &&
-			test_must_fail ok=sigpipe git fetch ../testrepo/.git $SHA1_3 &&
-			test_must_fail ok=sigpipe git fetch ../testrepo/.git $SHA1_1 &&
+			test_must_fail ok=sigpipe env GIT_TEST_PROTOCOL_VERSION= \
+				git fetch ../testrepo/.git $SHA1_3 &&
+			test_must_fail ok=sigpipe env GIT_TEST_PROTOCOL_VERSION= \
+				git fetch ../testrepo/.git $SHA1_1 &&
 			git --git-dir=../testrepo/.git config uploadpack.allowreachablesha1inwant true &&
 			git fetch ../testrepo/.git $SHA1_1 &&
 			git cat-file commit $SHA1_1 &&
 			test_must_fail git cat-file commit $SHA1_2 &&
 			git fetch ../testrepo/.git $SHA1_2 &&
 			git cat-file commit $SHA1_2 &&
-			test_must_fail ok=sigpipe git fetch ../testrepo/.git $SHA1_3
+			test_must_fail ok=sigpipe env GIT_TEST_PROTOCOL_VERSION= \
+				git fetch ../testrepo/.git $SHA1_3
 		)
 	'
 done

index 6faf17e17a..85b3022ce6 100755
@@ -127,7 +127,8 @@ test_expect_success 'fetch that requires changes in .git/shallow is filtered' '
 	git init notshallow &&
 	(
 	cd notshallow &&
-	git fetch ../shallow/.git refs/heads/*:refs/remotes/shallow/*&&
+	GIT_TEST_PROTOCOL_VERSION= \
+		git fetch ../shallow/.git refs/heads/*:refs/remotes/shallow/* &&
 	git for-each-ref --format="%(refname)" >actual.refs &&
 	cat <<EOF >expect.refs &&
 refs/remotes/shallow/no-shallow

index 5fbf67c446..a121e19bc7 100755
@@ -67,7 +67,8 @@ test_expect_success 'no shallow lines after receiving ACK ready' '
 		cd clone &&
 		git checkout --orphan newnew &&
 		test_commit new-too &&
-		GIT_TRACE_PACKET="$TRASH_DIRECTORY/trace" git fetch --depth=2 &&
+		GIT_TRACE_PACKET="$TRASH_DIRECTORY/trace" GIT_TEST_PROTOCOL_VERSION= \
+			git fetch --depth=2 &&
 		grep "fetch-pack< ACK .* ready" ../trace &&
 		! grep "fetch-pack> done" ../trace
 	)

@@ -114,7 +115,7 @@ test_expect_success 'shallow clone exclude tag two' '
 '
 
 test_expect_success 'fetch exclude tag one' '
-	git -C shallow12 fetch --shallow-exclude one origin &&
+	GIT_TEST_PROTOCOL_VERSION= git -C shallow12 fetch --shallow-exclude one origin &&
 	git -C shallow12 log --pretty=tformat:%s origin/master >actual &&
 	test_write_lines three two >expected &&
 	test_cmp expected actual

@@ -128,14 +129,14 @@ test_expect_success 'fetching deepen' '
 	test_commit two &&
 	test_commit three &&
 	mv .git "$HTTPD_DOCUMENT_ROOT_PATH/shallow-deepen.git" &&
-	git clone --depth 1 $HTTPD_URL/smart/shallow-deepen.git deepen &&
+	GIT_TEST_PROTOCOL_VERSION= git clone --depth 1 $HTTPD_URL/smart/shallow-deepen.git deepen &&
 	mv "$HTTPD_DOCUMENT_ROOT_PATH/shallow-deepen.git" .git &&
 	test_commit four &&
 	git -C deepen log --pretty=tformat:%s master >actual &&
 	echo three >expected &&
 	test_cmp expected actual &&
 	mv .git "$HTTPD_DOCUMENT_ROOT_PATH/shallow-deepen.git" &&
-	git -C deepen fetch --deepen=1 &&
+	GIT_TEST_PROTOCOL_VERSION= git -C deepen fetch --deepen=1 &&
 	git -C deepen log --pretty=tformat:%s origin/master >actual &&
 	cat >expected <<-\EOF &&
 	four

index 5475afc052..180c9005b7 100755
@@ -45,14 +45,19 @@ test_expect_success 'no empty path components' '
 	# In the URL, add a trailing slash, and see if git appends yet another
 	# slash.
 	cd "$ROOT_PATH" &&
-	git clone $HTTPD_URL/smart/test_repo.git/ test_repo_clone &&
+	# Other protocol versions may make different requests, so perform this
+	# clone with the default protocol.
+	GIT_TEST_PROTOCOL_VERSION= git clone $HTTPD_URL/smart/test_repo.git/ test_repo_clone &&
 
 	check_access_log exp
 '
 
 test_expect_success 'clone remote repository' '
 	rm -rf test_repo_clone &&
-	git clone $HTTPD_URL/smart/test_repo.git test_repo_clone &&
+	# Other protocol versions may make different requests, so perform this
+	# clone with the default protocol.
+	GIT_TEST_PROTOCOL_VERSION= git clone $HTTPD_URL/smart/test_repo.git test_repo_clone &&
+
 	(
 		cd test_repo_clone && git config push.default matching
 	)

index 8630b0cc39..a51993f35a 100755
@@ -43,7 +43,8 @@ test_expect_success 'clone http repository' '
 	< Cache-Control: no-cache, max-age=0, must-revalidate
 	< Content-Type: application/x-git-upload-pack-result
 	EOF
-	GIT_TRACE_CURL=true git clone --quiet $HTTPD_URL/smart/repo.git clone 2>err &&
+	GIT_TRACE_CURL=true GIT_TEST_PROTOCOL_VERSION= \
+		git clone --quiet $HTTPD_URL/smart/repo.git clone 2>err &&
 	test_cmp file clone/file &&
 	tr '\''\015'\'' Q <err |
 	sed -e "

@@ -92,7 +93,7 @@ test_expect_success 'fetch changes via http' '
 	echo content >>file &&
 	git commit -a -m two &&
 	git push public &&
-	(cd clone && git pull) &&
+	(cd clone && GIT_TEST_PROTOCOL_VERSION= git pull) &&
 	test_cmp file clone/file
 '
 

@@ -143,7 +144,7 @@ test_expect_success 'clone from auth-only-for-push repository' '
 test_expect_success 'clone from auth-only-for-objects repository' '
 	echo two >expect &&
 	set_askpass user@host pass@host &&
-	git clone --bare "$HTTPD_URL/auth-fetch/smart/repo.git" half-auth &&
+	GIT_TEST_PROTOCOL_VERSION= git clone --bare "$HTTPD_URL/auth-fetch/smart/repo.git" half-auth &&
 	expect_askpass both user@host &&
 	git --git-dir=half-auth log -1 --format=%s >actual &&
 	test_cmp expect actual

@@ -151,7 +152,7 @@ test_expect_success 'clone from auth-only-for-objects repository' '
 
 test_expect_success 'no-op half-auth fetch does not require a password' '
 	set_askpass wrong &&
-	git --git-dir=half-auth fetch &&
+	GIT_TEST_PROTOCOL_VERSION= git --git-dir=half-auth fetch &&
 	expect_askpass none
 '
 

@@ -187,7 +188,7 @@ test_expect_success 'create namespaced refs' '
 '
 
 test_expect_success 'smart clone respects namespace' '
-	git clone "$HTTPD_URL/smart_namespace/repo.git" ns-smart &&
+	GIT_TEST_PROTOCOL_VERSION= git clone "$HTTPD_URL/smart_namespace/repo.git" ns-smart &&
 	echo namespaced >expect &&
 	git --git-dir=ns-smart/.git log -1 --format=%s >actual &&
 	test_cmp expect actual

@@ -214,7 +215,7 @@ test_expect_success 'cookies stored in http.cookiefile when http.savecookies set
 	EOF
 	git config http.cookiefile cookies.txt &&
 	git config http.savecookies true &&
-	git ls-remote $HTTPD_URL/smart_cookies/repo.git master &&
+	GIT_TEST_PROTOCOL_VERSION= git ls-remote $HTTPD_URL/smart_cookies/repo.git master &&
 	tail -3 cookies.txt | sort >cookies_tail.txt &&
 	test_cmp expect_cookies.txt cookies_tail.txt
 '

@@ -306,7 +307,8 @@ test_expect_success 'test allowreachablesha1inwant with unreachable' '
 
 	git init --bare test_reachable.git &&
 	git -C test_reachable.git remote add origin "$HTTPD_URL/smart/repo.git" &&
-	test_must_fail git -C test_reachable.git fetch origin "$(git rev-parse HEAD)"
+	test_must_fail env GIT_TEST_PROTOCOL_VERSION= \
+		git -C test_reachable.git fetch origin "$(git rev-parse HEAD)"
 '
 
 test_expect_success 'test allowanysha1inwant with unreachable' '

@@ -325,7 +327,8 @@ test_expect_success 'test allowanysha1inwant with unreachable' '
 
 	git init --bare test_reachable.git &&
 	git -C test_reachable.git remote add origin "$HTTPD_URL/smart/repo.git" &&
-	test_must_fail git -C test_reachable.git fetch origin "$(git rev-parse HEAD)" &&
+	test_must_fail env GIT_TEST_PROTOCOL_VERSION= \
+		git -C test_reachable.git fetch origin "$(git rev-parse HEAD)" &&
 
 	git -C "$server" config uploadpack.allowanysha1inwant 1 &&
 	git -C test_reachable.git fetch origin "$(git rev-parse HEAD)"

index 30857b84a8..979e6583d4 100755
@@ -127,7 +127,7 @@ test_expect_success 'use ref advertisement to filter out commits' '
 	# not need to send any ancestors of "c3", but we still need to send "c3"
 	# itself.
 	test_config -C client fetch.negotiationalgorithm skipping &&
-	trace_fetch client origin to_fetch &&
+	GIT_TEST_PROTOCOL_VERSION= trace_fetch client origin to_fetch &&
 	have_sent c5 c4^ c2side &&
 	have_not_sent c4 c4^^ c4^^^
 '

@@ -189,7 +189,7 @@ test_expect_success 'do not send "have" with ancestors of commits that server AC
 	test_commit -C server commit-on-b1 &&
 
 	test_config -C client fetch.negotiationalgorithm skipping &&
-	trace_fetch client "$(pwd)/server" to_fetch &&
+	GIT_TEST_PROTOCOL_VERSION= trace_fetch client "$(pwd)/server" to_fetch &&
 	grep "  fetch" trace &&
 
 	# fetch-pack sends 2 requests each containing 16 "have" lines before

index 7466aad111..c42ee245cd 100755
@@ -190,7 +190,7 @@ test_expect_success 'daemon log records all attributes' '
 	EOF
 	>daemon.log &&
 	GIT_OVERRIDE_VIRTUAL_HOST=localhost \
-		git -c protocol.version=1 \
+		GIT_TEST_PROTOCOL_VERSION= git -c protocol.version=1 \
 			ls-remote "$GIT_DAEMON_URL/interp.git" &&
 	grep -i extended.attribute daemon.log | cut -d" " -f2- >actual &&
 	test_cmp expect actual

index e87164aa8f..a555e38845 100755
@@ -943,7 +943,8 @@ test_expect_success 'submodule update clone shallow submodule outside of depth'
 		cd super3 &&
 		sed -e "s#url = ../#url = file://$pwd/#" <.gitmodules >.gitmodules.tmp &&
 		mv -f .gitmodules.tmp .gitmodules &&
-		test_must_fail git submodule update --init --depth=1 2>actual &&
+		test_must_fail env GIT_TEST_PROTOCOL_VERSION= \
+			git submodule update --init --depth=1 2>actual &&
 		test_i18ngrep "Direct fetching of that commit failed." actual &&
 		git -C ../submodule config uploadpack.allowReachableSHA1InWant true &&
 		git submodule update --init --depth=1 >actual &&

2.20.0.405.gbc1bbc6f85

On Thu, Dec 13 2018, Ævar Arnfjörð Bjarmason wrote:

Now that we have this maybe we should discuss why these tests show different things:

UnfortunatelyYes (see Peff's three-patch series).

Yeah, I actually didn't realize it until working on the earlier series, but this is the documented behavior:

  $ git grep -hC3 'want <oid>' Documentation/technical/protocol-v2.txt

A fetch request can take the following arguments:

want <oid>
Indicates to the server an object which the client wants to retrieve. Wants can be anything and are not limited to advertised objects.

An interesting implication of this at GitHub (and possibly other hosters) is that it exposes objects from shared storage via unexpected repos. If I fork torvalds/linux to peff/linux and push up object X, a v0 fetch will (by default) refuse to serve it to me. But v2 will happily hand it over, which may confuse people into thinking that the object is (or at least at some point was) in Linus's repository.

-Peff

More importantly this bypasses the security guarantee we've had with the default of uploadpack.allowAnySHA1InWant=false.

If I push a id_rsa to a topic branch on $githost and immediately realize my mistake and delete it, someone with access to a log of SHA-1s (e.g. an IRC bot spewing out from/to commits) won't be able to pull it down. This is why we have that as a setting in the first place (f8edeaa05d ("upload-pack: optionally allow fetching any sha1", 2016-11-11)).

Of course this is:

a) Subject to the "SECURITY" section of git-fetch, i.e. maybe this doesn't even work in the face of a determined attacker.

b) Hosting sites like GitHub, GitLab, Gitweb etc. aren't doing reachability checks when you view /commit/<id>. If I delete my topic branch it'll be viewable until the next GC kicks in (which would also be the case with uploadpack.allowAnySHA1InWant=true).

So I wonder how much we need to care about this in practice, but for some configurations protocol v2 definitely breaks a security promise we've been making, now whether that promise was always BS due to "a)" above is another matter.

I'm inclined to say that in the face of that "SECURITY" section we should just:

* Turn on uploadpack.allowReachableSHA1InWant for v0/v1 by default. Make saying uploadpack.allowReachableSHA1InWant=false warn with "this won't work, see SECURITY...".

* The uploadpack.allowTipSHA1InWant setting will also be turned on by default, and will be much faster, since it'll just degrade to uploadpack.allowReachableSHA1InWant=true and we won't need any reachability check. We'll also warn saying that setting it is useless.

Otherwise what's th point of these settings given what we're talking about in that "SECURITY" section? It seems to just lure users into a false sense of security. Better to not make promises we're not confident we can keep, and say that if you push your id_rsa you need to run a gc/prune on the server.

I sent that too fast. What I meant is that there's 3
settings. uploadpack.{allowTipSHA1InWant,allowReachableSHA1InWant,allowAnySHA1InWant}. Those are, respectively, cheap/expensive/cheap to serve up.

We could make them cheap/cheap/cheap by just making the first two a synonym for the 3rd (allowAnySHA1InWant), because as noted above

Also, parts of the v2 code, e.g. this in 685fbd3291 ("fetch-pack:
perform a fetch using v2", 2018-03-15):

	/* v2 supports these by default */
	allow_unadvertised_object_request |= ALLOW_REACHABLE_SHA1;

Seem to have intended to turn on allowReachableSHA1InWant, not allowAnySHA1InWant, but apparently that's what we're doing?

In any case, regardless of what v2 intended there anything except having allowAnySHA1InWant on by default seems irresponsible given x) us documenting in "SECURITY" that it doesn't really work y) even if it did, there being a *tiny* minority of people who'd in some way care about this feature who don't also run some sort of web UI on the git server that'll be bypassing this setting no matter if it worked as intended for the wire protocol.

That I don't know. Maybe Brandon can answer what the intent was.

I agree that turning on the reachability check would make it behavior more or less the same as v0 (at some additional cost to walk the refs again, but v0 smart-http already had to do that).

-Peff

IMHO those security guarantees there are overrated (due to delta guessing attacks, though things are not quite as bad if the attacker can't actually push to the repo).

But I agree that people do assume it's the case. I was certainly surprised by the v2 behavior, and I don't remember that aspect being discussed.

(I suspect it was mostly because in the stateless world, the "fetch" operation requires iterating all of the refs again. That's made even harder by the fact that "ls-refs" takes arguments now, and might have only advertised a subset of the refs. How can "fetch" know which ones were advertised?).

Yeah, this is the part I was thinking about above.

Actually, we don't ever prune unless a user asks us to (and certainly users do ask us to after accidentally pushing secret stuff). In GitHub's case, though, we always tell people to consider anything pushed to a public repo to be non-secret, even if it was exposed for only a few seconds. My understanding is that there are literally clients watching the public feeds and sucking down repo data looking for these kinds of mistakes.

The /commit/<id> thing has been a frequent topic of discussion within GitHub over the years, and we have looked at actually doing reachability checks. They're expensive, though bitmaps help.

No real argument from me. I have always thought those security guarantees were BS.

However, I do think that's all orthogonal to the issue I was mentioning, which is that it looks like repo A wants to serve you object X, even if that repo never saw the object. That's unique to shared storage schemes, though.

IMHO this is also a user education issue (it's a question of trust: refs are the source of trust, and objects don't need to be trusted because they're immutable). But it's pretty subtle for people who don't know the inner workings of Git.

-Peff

Unconditionally turn on uploadpack.allowAnySHA1InWant=true which means that the lesser uploadpack.allow{Tip,Reachable}SHA1InWant settings are implicitly "true" as well.

This is because as mentioned in 235ec24352e ("doc: mention transfer data leaks in more places", 2016-11-14) this has always been leaky, and thus lulls users into a false sense of security. Better to not make promises we're not confident we can keep.

Now setting any of uploadpack.allow{Tip,Reachable,Any}SHA1InWant will warn, to e.g. point users who've explicitly set it to "false" to the documentation. They're probably relying on behavior that was always buggy. We may have users who trusted it to be "false" by default, but warning if it's set seems like a good trade-off.

This change also has the effect of making any operation that needed uploadpack.allowReachableSHA1InWant=true much faster, since we won't need any reachability check.

This change doesn't do any of the hard work of extracting the now-dead ALLOW_* code out of upload-pack.c and fetch-pack.c. Most of that was changed or added in the following commits:

- 9f9fb57063a ("upload-pack: turn on uploadpack.allowAnySHA1InWant=true", 2018-12-17)
- 6e7b66eebd1 ("fetch: fetch objects by their exact SHA-1 object names", 2013-01-29)
- 7199c093ad4 ("upload-pack: prepare to extend allow-tip-sha1-in-want", 2015-05-21)
- 68ee628932c ("upload-pack: optionally allow fetching reachable sha1", 2015-05-21)
- f8edeaa05d8 ("upload-pack: optionally allow fetching any sha1", 2016-11-11)

That cleanup step can be done later. Starting with a change to switch the default (and only) value and tweaking the docs makes it easier to reason about this than fully ripping out the code right away.

See https://public-inbox.org/git/[email protected]/
and related messages for further discussion.

---
 Documentation/config/uploadpack.txt   | 48 +++++++++++++++----------
 Documentation/transfer-data-leaks.txt |  8 +++++
 t/t0410-partial-clone.sh              |  2 --
 t/t1090-sparse-checkout-scope.sh      |  1 -
 t/t5500-fetch-pack.sh                 | 10 +++---
 t/t5512-ls-remote.sh                  | 14 ++++++++
 t/t5516-fetch-push.sh                 | 50 +++++++++++++--------------
 t/t5551-http-fetch-smart.sh           |  4 ---
 t/t5601-clone.sh                      |  3 --
 t/t5616-partial-clone.sh              |  8 +----
 t/t5702-protocol-v2.sh                |  1 -
 t/t7406-submodule-update.sh           |  5 +--
 upload-pack.c                         | 18 +++-------
 13 files changed, 87 insertions(+), 85 deletions(-)

index ed1c835695c..854801bd6cc 100644
@@ -1,31 +1,41 @@
 uploadpack.hideRefs::
 	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`.
++
+This can be used in some setups to reduce the size of the ref
+advertisement, see also `protocol.version` for using a protocol
+version that allows them to be filtered by the client.
++
+In earlier versions of Git attempts to fetch an object pointed to by a
+hidden ref would fail, this is no longer the case. See
+`uploadpack.allowAnySHA1InWant`.
 
 uploadpack.allowTipSHA1InWant::
-	When `uploadpack.hideRefs` is in effect, allow `upload-pack`
-	to accept a fetch request that asks for an object at the tip
-	of a hidden ref (by default, such a request is rejected).
-	See also `uploadpack.hideRefs`.  Even if this is false, a client
-	may be able to steal objects via the techniques described in the
-	"SECURITY" section of the linkgit:gitnamespaces[7] man page; it's
-	best to keep private data in a separate repository.
+	Depreacted. Used to allow `upload-pack` to accept a fetch
+	request that asked for an object at the tip of a hidden
+	ref. Now always `true`. See `uploadpack.allowAnySHA1InWant`
+	below for details.
 
 uploadpack.allowReachableSHA1InWant::
-	Allow `upload-pack` to accept a fetch request that asks for an
-	object that is reachable from any ref tip. However, note that
-	calculating object reachability is computationally expensive.
-	Defaults to `false`.  Even if this is false, a client may be able
-	to steal objects via the techniques described in the "SECURITY"
-	section of the linkgit:gitnamespaces[7] man page; it's best to
-	keep private data in a separate repository.
+	Depreacted. Used to allow `upload-pack` to accept a fetch
+	request that asked for an object that was reachable from any
+	ref tip. Now always `true`. See
+	`uploadpack.allowAnySHA1InWant` below for details.
 
 uploadpack.allowAnySHA1InWant::
-	Allow `upload-pack` to accept a fetch request that asks for any
-	object at all.
-	Defaults to `false`.
+	Deprecated. Before Git version 2.21 the
+	`uploadpack.allow{Tip,Any,Reachable}SHA1InWant` settings were
+	`false` by default. Now they're unconditionally `true`, which
+	means fetch request that ask for any object in the object
+	store will be accepted, regardless of whether such an object
+	is at the tip of a ref, reachable by no ref etc.
++
+The reason for the change in default is as noted in the "SECURITY"
+section of linkgit:git-fetch[1] a determined client can get to any
+previously forbidden objects the server has access to by manipulating
+the client/server dialog. If you host objects that should be kept
+secret from some clients it's best to keep them in a separate
+repository.
 
 uploadpack.keepAlive::
 	When `upload-pack` has started `pack-objects`, there may be a

index 914bacc39e0..f8beb334b84 100644
@@ -28,3 +28,11 @@ The known attack vectors are as follows:
   an object Y that the attacker already has, and the attacker falsely
   claims to have X and not Y, so the victim sends Y as a delta against X.
   The delta reveals regions of X that are similar to Y to the attacker.
+
+Before Git version 2.21 the
+`uploadpack.allow{Tip,Any,Reachable}SHA1InWant` allowed for tweaking
+linkgit:git-upload-pack[1] behavior to refuse to serve some refs. As
+noted above this served to lull users into a false sense of security,
+and the fetch protocol will now allow clients to fetch any object in
+the ref store. See `uploadpack.allowAnySHA1InWant` in
+linkgit:git-config[1].

index ba3887f178b..a9d24a94d9f 100755
@@ -192,7 +192,6 @@ test_expect_success 'fetching of missing blobs works' '
 	git hash-object repo/foo.t >blobhash &&
 	rm -rf repo/.git/objects/* &&
 
-	git -C server config uploadpack.allowanysha1inwant 1 &&
 	git -C server config uploadpack.allowfilter 1 &&
 	git -C repo config core.repositoryformatversion 1 &&
 	git -C repo config extensions.partialclone "origin" &&

@@ -211,7 +210,6 @@ test_expect_success 'fetching of missing trees does not fetch blobs' '
 	git hash-object repo/foo.t >blobhash &&
 	rm -rf repo/.git/objects/* &&
 
-	git -C server config uploadpack.allowanysha1inwant 1 &&
 	git -C server config uploadpack.allowfilter 1 &&
 	git -C repo config core.repositoryformatversion 1 &&
 	git -C repo config extensions.partialclone "origin" &&

index 090b7fc3d35..4de2e2dd1bb 100755
@@ -68,7 +68,6 @@ test_expect_success 'in partial clone, sparse checkout only fetches needed blobs
 	git clone "file://$(pwd)/server" client &&
 
 	test_config -C server uploadpack.allowfilter 1 &&
-	test_config -C server uploadpack.allowanysha1inwant 1 &&
 	echo a >server/a &&
 	echo bb >server/b &&
 	mkdir server/c &&

index 086f2c40f68..b639629863d 100755
@@ -592,8 +592,7 @@ test_expect_success 'fetch-pack can fetch a raw sha1' '
 		test_commit 1 &&
 		test_commit 2 &&
 		git update-ref refs/hidden/one HEAD^ &&
-		git config transfer.hiderefs refs/hidden &&
-		git config uploadpack.allowtipsha1inwant true
+		git config transfer.hiderefs refs/hidden
 	) &&
 	git fetch-pack hidden $(git -C hidden rev-parse refs/hidden/one)
 '

@@ -619,7 +618,7 @@ test_expect_success 'fetch-pack can fetch a raw sha1 overlapping a named ref' '
 		$(git -C server rev-parse refs/tags/1) refs/tags/1
 '
 
-test_expect_success 'fetch-pack cannot fetch a raw sha1 that is not advertised as a ref' '
+test_expect_success 'fetch-pack can fetch a raw sha1 that is not advertised as a ref' '
 	rm -rf server &&
 
 	git init server &&

@@ -628,9 +627,8 @@ test_expect_success 'fetch-pack cannot fetch a raw sha1 that is not advertised a
 	test_commit -C server 6 &&
 
 	git init client &&
-	test_must_fail git -C client fetch-pack ../server \
-		$(git -C server rev-parse refs/heads/master^) 2>err &&
-	test_i18ngrep "Server does not allow request for unadvertised object" err
+	git -C client fetch-pack ../server \
+		$(git -C server rev-parse refs/heads/master^)
 '
 
 check_prot_path () {

index 32e722db2ed..0cd1aad9597 100755
@@ -195,6 +195,20 @@ do
 		! grep refs/tags/magic/one actual
 	'
 
+
+	test_expect_success "fetching a divergent object hidden by $configsection.hiderefs" "
+		test_when_finished 'test_unconfig $configsection.hiderefs' &&
+		git config --add $configsection.hiderefs refs/tags &&
+		test_when_finished 'git tag -d magic/divergent' &&
+		git tag -a -m'a tag' magic/divergent &&
+		SHA1_TAG=\$(git rev-parse magic/divergent) &&
+		echo \$SHA1_TAG >expect &&
+		git init client &&
+		test_when_finished 'rm -r client' &&
+		git -C client fetch ../ \$SHA1_TAG:refs/tags/magic/divergent &&
+		git -C client rev-parse magic/divergent >actual &&
+		test_cmp expect actual
+	"
 done
 
 test_expect_success 'overrides work between mixed transfer/upload-pack hideRefs' '

index 8630b0cc390..b8f7c0b5563 100755
@@ -283,7 +283,6 @@ test_expect_success 'test allowreachablesha1inwant' '
 	test_when_finished "rm -rf test_reachable.git" &&
 	server="$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
 	master_sha=$(git -C "$server" rev-parse refs/heads/master) &&
-	git -C "$server" config uploadpack.allowreachablesha1inwant 1 &&
 
 	git init --bare test_reachable.git &&
 	git -C test_reachable.git remote add origin "$HTTPD_URL/smart/repo.git" &&

@@ -302,7 +301,6 @@ test_expect_success 'test allowreachablesha1inwant with unreachable' '
 
 	server="$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
 	master_sha=$(git -C "$server" rev-parse refs/heads/master) &&
-	git -C "$server" config uploadpack.allowreachablesha1inwant 1 &&
 
 	git init --bare test_reachable.git &&
 	git -C test_reachable.git remote add origin "$HTTPD_URL/smart/repo.git" &&

@@ -321,13 +319,11 @@ test_expect_success 'test allowanysha1inwant with unreachable' '
 
 	server="$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
 	master_sha=$(git -C "$server" rev-parse refs/heads/master) &&
-	git -C "$server" config uploadpack.allowreachablesha1inwant 1 &&
 
 	git init --bare test_reachable.git &&
 	git -C test_reachable.git remote add origin "$HTTPD_URL/smart/repo.git" &&
 	test_must_fail git -C test_reachable.git fetch origin "$(git rev-parse HEAD)" &&
 
-	git -C "$server" config uploadpack.allowanysha1inwant 1 &&
 	git -C test_reachable.git fetch origin "$(git rev-parse HEAD)"
 '
 

index 8bbc7068acb..ec7a3c46528 100755
@@ -646,7 +646,6 @@ partial_clone () {
 	test_commit -C "$SERVER" two &&
 	HASH2=$(git hash-object "$SERVER/two.t") &&
 	test_config -C "$SERVER" uploadpack.allowfilter 1 &&
-	test_config -C "$SERVER" uploadpack.allowanysha1inwant 1 &&
 
 	git clone --filter=blob:limit=0 "$URL" client &&
 

@@ -689,7 +688,6 @@ test_expect_success 'batch missing blob request during checkout' '
 	git -C server commit -m x &&
 
 	test_config -C server uploadpack.allowfilter 1 &&
-	test_config -C server uploadpack.allowanysha1inwant 1 &&
 
 	git clone --filter=blob:limit=0 "file://$(pwd)/server" client &&
 

@@ -720,7 +718,6 @@ test_expect_success 'batch missing blob request does not inadvertently try to fe
 	git -C server commit -m x &&
 
 	test_config -C server uploadpack.allowfilter 1 &&
-	test_config -C server uploadpack.allowanysha1inwant 1 &&
 
 	# Make sure that it succeeds
 	git clone --filter=blob:limit=0 "file://$(pwd)/server" client

index 336f02a41a6..213899a690d 100755
@@ -25,8 +25,7 @@ test_expect_success 'setup normal src repo' '
 # bare clone "src" giving "srv.bare" for use as our server.
 test_expect_success 'setup bare clone for server' '
 	git clone --bare "file://$(pwd)/src" srv.bare &&
-	git -C srv.bare config --local uploadpack.allowfilter 1 &&
-	git -C srv.bare config --local uploadpack.allowanysha1inwant 1
+	git -C srv.bare config --local uploadpack.allowfilter 1
 '
 
 # do basic partial clone from "srv.bare"

@@ -159,7 +158,6 @@ test_expect_success 'partial clone with transfer.fsckobjects=1 uses index-pack -
 	git init src &&
 	test_commit -C src x &&
 	test_config -C src uploadpack.allowfilter 1 &&
-	test_config -C src uploadpack.allowanysha1inwant 1 &&
 
 	GIT_TRACE="$(pwd)/trace" git -c transfer.fsckobjects=1 \
 		clone --filter="blob:none" "file://$(pwd)/src" dst &&

@@ -213,7 +211,6 @@ test_expect_success 'partial clone fetches blobs pointed to by refs even if norm
 	git init src &&
 	test_commit -C src x &&
 	test_config -C src uploadpack.allowfilter 1 &&
-	test_config -C src uploadpack.allowanysha1inwant 1 &&
 
 	# Create a tag pointing to a blob.
 	BLOB=$(echo blob-contents | git -C src hash-object --stdin -w) &&

@@ -229,7 +226,6 @@ test_expect_success 'fetch what is specified on CLI even if already promised' '
 	git init src &&
 	test_commit -C src foo &&
 	test_config -C src uploadpack.allowfilter 1 &&
-	test_config -C src uploadpack.allowanysha1inwant 1 &&
 
 	git hash-object --stdin <src/foo.t >blob &&
 

@@ -257,7 +253,6 @@ test_expect_success 'upon cloning, check that all refs point to objects' '
 	test_create_repo "$SERVER" &&
 	test_commit -C "$SERVER" foo &&
 	test_config -C "$SERVER" uploadpack.allowfilter 1 &&
-	test_config -C "$SERVER" uploadpack.allowanysha1inwant 1 &&
 
 	# Create a tag pointing to a blob.
 	BLOB=$(echo blob-contents | git -C "$SERVER" hash-object --stdin -w) &&

@@ -293,7 +288,6 @@ test_expect_success 'when partial cloning, tolerate server not sending target of
 	test_create_repo "$SERVER" &&
 	test_commit -C "$SERVER" foo &&
 	test_config -C "$SERVER" uploadpack.allowfilter 1 &&
-	test_config -C "$SERVER" uploadpack.allowanysha1inwant 1 &&
 
 	# Create an annotated tag pointing to a blob.
 	BLOB=$(echo blob-contents | git -C "$SERVER" hash-object --stdin -w) &&

index 0f2b09ebb8d..af16f139a4a 100755
@@ -274,7 +274,6 @@ test_expect_success 'setup filter tests' '
 	test_commit -C server message2 a.txt &&
 	git -C server config protocol.version 2 &&
 	git -C server config uploadpack.allowfilter 1 &&
-	git -C server config uploadpack.allowanysha1inwant 1 &&
 	git -C server config protocol.version 2
 '
 

index e87164aa8ff..cf5bda08690 100755
@@ -943,10 +943,7 @@ test_expect_success 'submodule update clone shallow submodule outside of depth'
 		cd super3 &&
 		sed -e "s#url = ../#url = file://$pwd/#" <.gitmodules >.gitmodules.tmp &&
 		mv -f .gitmodules.tmp .gitmodules &&
-		test_must_fail git submodule update --init --depth=1 2>actual &&
-		test_i18ngrep "Direct fetching of that commit failed." actual &&
-		git -C ../submodule config uploadpack.allowReachableSHA1InWant true &&
-		git submodule update --init --depth=1 >actual &&
+		git submodule update --init --depth=1 &&
 		git -C submodule log --oneline >out &&
 		test_line_count = 1 out
 	)

index 5e81f1ff24f..18e4c2d2452 100644
@@ -54,7 +54,8 @@ static int no_progress, daemon_mode;
 #define ALLOW_REACHABLE_SHA1	02
 /* Allow request of any sha1. Implies ALLOW_TIP_SHA1 and ALLOW_REACHABLE_SHA1. */
 #define ALLOW_ANY_SHA1	07
-static unsigned int allow_unadvertised_object_request;
+static unsigned int allow_unadvertised_object_request = (
+	ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1 | ALLOW_ANY_SHA1);
 static int shallow_nr;
 static struct object_array extra_edge_obj;
 static unsigned int timeout;

@@ -1019,20 +1020,11 @@ static int find_symref(const char *refname, const struct object_id *oid,
 static int upload_pack_config(const char *var, const char *value, void *unused)
 {
 	if (!strcmp("uploadpack.allowtipsha1inwant", var)) {
-		if (git_config_bool(var, value))
-			allow_unadvertised_object_request |= ALLOW_TIP_SHA1;
-		else
-			allow_unadvertised_object_request &= ~ALLOW_TIP_SHA1;
+		warning(_("The uploadpack.allowTipSHA1InWant setting is deprecated, and now always 'true'"));
 	} else if (!strcmp("uploadpack.allowreachablesha1inwant", var)) {
-		if (git_config_bool(var, value))
-			allow_unadvertised_object_request |= ALLOW_REACHABLE_SHA1;
-		else
-			allow_unadvertised_object_request &= ~ALLOW_REACHABLE_SHA1;
+		warning(_("The uploadpack.allowReachableSHA1InWant setting is deprecated, and now always 'true'"));
 	} else if (!strcmp("uploadpack.allowanysha1inwant", var)) {
-		if (git_config_bool(var, value))
-			allow_unadvertised_object_request |= ALLOW_ANY_SHA1;
-		else
-			allow_unadvertised_object_request &= ~ALLOW_ANY_SHA1;
+		warning(_("The uploadpack.allowAnySHA1InWant setting is deprecated, and now always 'true'"));
 	} else if (!strcmp("uploadpack.keepalive", var)) {
 		keepalive = git_config_int(var, value);
 		if (!keepalive)

2.20.0.405.gbc1bbc6f85

Overall, I like this. One nit:

Sent from my Android device with K-9 Mail. Please excuse my brevity.

Thanks for the review!

FWIW (and maybe I made the wrong call, and have no preference realy) I opted for this as part of "this change doesn't do any of the hard work of extracting the now-dead ALLOW_[...]*.

I.e. the diff context I had doesn't show all the ALLOW_* values, and with the context given it might be easier for reviewers to look no further than "this is what you'd get before if all uploadpack.allow.*SHA1InWant was true".

The context I quoted said /* Allow request of any sha1. Implies ALLOW_TIP_SHA1 and ALLOW_REACHABLE_SHA1. */

But up to you (or Junio).

Sent from my Android device with K-9 Mail. Please excuse my brevity.

Hi,

Do you have a proof of concept for delta guessing? My understanding was that without using a broken hash (e.g. uncorrected SHA-1), it is not feasible to carry out.

JGit checks delta bases in received thin packs for reachability as well.

IMHO it's a plain bug (either in implementation or documentation).

[...]

This would make per-branch ACLs (as implemented both by Gerrit and gitolite) an essentially useless feature, so please no.

I would be all for changing the default, but making turning off allowReachableSHA1InWant an unsupported deprecated thing is a step too far, in my opinion.

Is there somewhere that we can document these kinds of invariants or goals so that we don't have to keep repeating the same discussions?

Thanks,
Jonathan

Doesn't Gerrit always use JGit?

The discussion upthread is about what we should do about git.git's version of this feature since we document it doesn't do its job from a security / ACL standpoint, but that doesn't preclude other server implementations from having a working version of this.

But if gitolite is relying on this to hide refs, and if our security docs are to be believed, then it's just providing security through obscurity.

Like you I'm curious about a POC. The patch I submitted in <[email protected]> takes the "SECURITY" docs at face value. I.e. I think it's dangerous to expose this as-is given the scary non-promise they make, but perhaps we'll find that this actually works well enough in some cases, or there's other non-security uses for this (e.g. simply discouraging users from cloning certain things, e.g. for cache performance purposes).

Yes, Gerrit uses JGit. JGit shares configuration with Git, so if we make that configuration in Git warn "This is just a placebo", then people will naturally assume that it's just a placebo in JGit, too.

More importantly, if Git upstream stops caring about this use case, then the protocol and other features can evolve in directions that make it even harder to support. I am willing to take on some of the burden of keeping it working, even when I do not run any servers that use plain Git (I do interact with many).

One of the difficulties that security engineers have to deal with is living without absolutes. In other words, their experience is constant risk/benefit tradeoffs: if you want 0% risk, then don't use a computer. ;-)

If someone has thoughts on what would lead people to be comfortable with removing the SECURITY notice, then I'm happy to listen. As already mentioned, I am strongly against abandoning this use case.

Jonathan

Aside from the merits of this change it sounds like a good idea to document the server options JGit shares with us somewhere, or have a test mode similar to what I added in (but didn't follow-up on integrating) in
https://public-inbox.org/git/[email protected]/

For me this would be docs backed up by tests (which can start as a ML reply) showing a case where this actually works to hide data.

I.e. have a repo with "master" and a "root-password" branch, where the "root-password" branch has content that's irresistible to "git repack" for delta purposes, do we end up sending the "root-password" content over on a fetch even when that branch isn't advertised & forbidden?

Or, if that fails are there ways to make it work? E.g. using hidden/* in combination with delta islands?

Delta islands wouldn't generally help here. They only tell us not to store on-disk deltas that fetching clients aren't likely to be able to reuse (i.e, they want X but will generally not have Y, so don't make a delta there).

In the attacks I mentioned in my previous email, the deltas would usually be computed on the fly for each fetch. So the lack of on-disk deltas across "security boundaries" wouldn't help.

You could disable on-the-fly delta compression, but the resulting packs are much larger (and you'd think it would save some server CPU, but experiments I've done show that it helps a lot less than you'd think, since we often end up zlib-deflating more bytes).

-Peff

I think we may be talking about two different things. I mean an attack where you want to know what is in object X, so you ask the server for object Y and tell it that you already have X. If the sender generates a delta against X, that tells you something about what's in X.

For a pure read-only server, you're restricted to the Y's that are already in the repo. So how effective this is depends on what's in X, and what Y's are available.

For a case where X is in a victim repo you cannot make arbitrary writes to, but you _can_ make the victim repo aware of other objects (e.g., by opening a pull request that creates a ref), then you can iteratively provide many Y's, improving your guess about X in each iteration.

For a case where the victim repo has fully shared storage (GitHub, and probably other hosts; I'm not sure if it's available yet, but GitLab is clearly working on shared-storage too), you can probably skip all that and just push a ref pointing to X with an empty pack (Git just cares that it has all of the objects afterwards, not that you pushed them).

None of those care about the quality of the hash (they do assume you know the hash of X already, but then so does fetching by object id).

And no, I've never written a proof-of-concept for that. It would depend largely on the data you're trying to extract. E.g., if you think X contains "root:XXXXXX", then you might hope to ask for "root:AXXXXX", then "root:BXXXXX", etc. You know you've got a hit when the delta gets smaller. So the complexity for guessing N bytes becomes 256*N, rather than 256^N.

Or both. :) The behavior and the documentation seem to agree.

I think Ævar's argument is that those are providing a false sense of security now (at least for read permissions).

Let me clarify my position a little.

I won't claim the existing scheme provides _no_ value (especially the pure read-only case above is less dicey than the others). It's mostly that the protections are very hand-wavy. I don't trust them _now_, and I have little faith that other innocent-looking changes to the object negotiation and the packing code will not significantly weaken them. There's no security boundary expressed within Git's code, so there's a very high chance of information leaking accidentally. A trustable system would have boundaries built in from the ground up.

Enough people seem to believe otherwise (i.e., that the hand-waving arguments are worth _something_) that I've never pushed to actually change the default behavior. But if Ævar wants to try to do so, I'm not going to stand in my way (hence my "no argument from me").

Yes, I agree if we were to go down this road, it probably makes sense to flip the knobs and let them be "unflipped" if the user wants.

It's not clear to me that there's consensus on the invariants or goals. ;)

-Peff

FWIW I don't really care about this, I don't rely on uploadpack.allow{Tip,Reachable,Any}SHA1InWant=false I'm just on the side-quest of:

1. Try protocol v2
2. Discover that v2 implictly has uploadpack.allowAnySHA1InWant=true enabled
3. Some people (including Jonathan) can reasonable read our docs / seem to have understood this to be a security mechanism
4. What are we going to do about that v1 & v2 discrepancy? [You are

     here!]

The genreal ways I see forward from that are:

A) Say that v2 has a security issue and that this is a feature that works in some circumstances, but given Jeff's explanation here we should at least improve our "SECURITY" docs to be less handwaivy.

B) Improve security docs, turn uploadpack.allowAnySHA1InWant=true on by default, allow people to turn it off.

C) Like B) but deprecate uploadpack.allow{Tip,Reachable,Any}SHA1InWant=false. This is my patch upthread

D-Z) ???

I'm not set on C), and yeah it's probably overzelous to just rip the thing out, but then what should we do?

Hmph. The other overzealous thing you could do is to strenthen A and "fix" the security issue in v2? Which letter comes before A in the alphabet? ;-)

Sure, but that being useful is predicated on this supposed security mechanism being useful and not just security-through-obscurity, as noted in side-threads I don't think we have a convincing argument either way (and the one we do have is more on the "it's not secure" side).

Of course we had that with v1 all along, but now that v2 is in released versions and in this insecure mode, we have a reason to closely look at whether we need to be issuing security releases, or doubling down on the "SECURITY" wording in git-fetch and then not carrying the mode forward.

Hi,

Yes, agreed. This is what I was hinting at in [1] with "it's a plain bug".

Just for the record, as I've already said, I would be strongly against removing this feature. I know of multiple populations that make use of it, and removing it would not serve them well.

Changing defaults and documentation is a separate story.

Sincerely,
Jonathan

[1] https://public-inbox.org/git/[email protected]/

A few tests are broken under GIT_TEST_PROTOCOL_VERSION=1, which as protocol.version in git-config(1) notes is just the GIT_TEST_PROTOCOL_VERSION=0 with a version number.

All of these cases look OK to me, and don't seem to show any regressions or other behavior differences that are unexpected. These tests are either testing exact v0 trace output, or trying to test the v2 protocol.

---
 t/t0410-partial-clone.sh | 1 +
 t/t5400-send-pack.sh     | 2 +-
 t/t5516-fetch-push.sh    | 1 +
 t/t5601-clone.sh         | 1 +
 t/t5702-protocol-v2.sh   | 1 +
 5 files changed, 5 insertions(+), 1 deletion(-)

index ba3887f178..786f96c467 100755
@@ -171,6 +171,7 @@ test_expect_success 'fetching of missing objects' '
 '
 
 test_expect_success 'fetching of missing objects works with ref-in-want enabled' '
+	sane_unset GIT_TEST_PROTOCOL_VERSION &&
 	# ref-in-want requires protocol version 2
 	git -C server config protocol.version 2 &&
 	git -C server config uploadpack.allowrefinwant 1 &&

index f1932ea431..b84618c925 100755
@@ -288,7 +288,7 @@ test_expect_success 'receive-pack de-dupes .have lines' '
 	$shared .have
 	EOF
 
-	GIT_TRACE_PACKET=$(pwd)/trace \
+	GIT_TRACE_PACKET=$(pwd)/trace GIT_TEST_PROTOCOL_VERSION=0 \
 	    git push \
 		--receive-pack="unset GIT_TRACE_PACKET; git-receive-pack" \
 		fork HEAD:foo &&

index 37e8e80893..49e5d305e5 100755
@@ -1172,6 +1172,7 @@ test_expect_success 'fetch exact SHA1' '
 '
 
 test_expect_success 'fetch exact SHA1 in protocol v2' '
+	sane_unset GIT_TEST_PROTOCOL_VERSION &&
 	mk_test testrepo heads/master hidden/one &&
 	git push testrepo master:refs/hidden/one &&
 	git -C testrepo config transfer.hiderefs refs/hidden &&

index 8bbc7068ac..a9ce050ee9 100755
@@ -345,6 +345,7 @@ expect_ssh () {
 }
 
 test_expect_success 'clone myhost:src uses ssh' '
+	sane_unset GIT_TEST_PROTOCOL_VERSION &&
 	git clone myhost:src ssh-clone &&
 	expect_ssh myhost src
 '

index 0f2b09ebb8..df7cc2a43a 100755
@@ -5,6 +5,7 @@ test_description='test git wire-protocol version 2'
 TEST_NO_CREATE_REPO=1
 
 . ./test-lib.sh
+sane_unset GIT_TEST_PROTOCOL_VERSION
 
 # Test protocol v2 with 'git://' transport
 #

2.20.0.405.gbc1bbc6f85

Mark those tests that have behavior differences or bugs under protocol.version=0.

Whether or not these tests should exhibit different behavior is outside the scope of this change. Some (such as t5700-protocol-v1.sh) clearly should, but others (such as t7406-submodule-update.sh) might indicate bugs in the protocol v2 code.

Tracking down which is which is outside the scope of this change. Let's first exhaustively annotate where the differences are, so that we can spot future behavior differences or regressions.

---
 t/t5500-fetch-pack.sh                | 4 +++-
 t/t5503-tagfollow.sh                 | 8 ++++----
 t/t5512-ls-remote.sh                 | 8 ++++----
 t/t5515-fetch-merge-logic.sh         | 1 +
 t/t5516-fetch-push.sh                | 3 ++-
 t/t5537-fetch-shallow.sh             | 3 ++-
 t/t5552-skipping-fetch-negotiator.sh | 1 +
 t/t5616-partial-clone.sh             | 3 ++-
 t/t5700-protocol-v1.sh               | 1 +
 t/t7406-submodule-update.sh          | 3 ++-
 10 files changed, 22 insertions(+), 13 deletions(-)

index 086f2c40f6..9c18875c9c 100755
@@ -41,7 +41,8 @@ pull_to_client () {
 	test_expect_success "$number pull" '
 		(
 			cd client &&
-			git fetch-pack -k -v .. $heads &&
+			GIT_TEST_PROTOCOL_VERSION=0 \
+				git fetch-pack -k -v .. $heads &&
 
 			case "$heads" in
 			    *A*)

@@ -440,6 +441,7 @@ test_expect_success 'setup tests for the --stdin parameter' '
 '
 
 test_expect_success 'fetch refs from cmdline' '
+	sane_unset GIT_TEST_PROTOCOL_VERSION &&
 	(
 		cd client &&
 		git fetch-pack --no-progress .. $(cat ../input)

index 4ca48f0276..220c677f24 100755
@@ -56,7 +56,7 @@ test_expect_success 'fetch A (new commit : 1 connection)' '
 	rm -f $U &&
 	(
 		cd cloned &&
-		GIT_TRACE_PACKET=$UPATH git fetch &&
+		GIT_TRACE_PACKET=$UPATH GIT_TEST_PROTOCOL_VERSION=0 git fetch &&
 		test $A = $(git rev-parse --verify origin/master)
 	) &&
 	get_needs $U >actual &&

@@ -86,7 +86,7 @@ test_expect_success 'fetch C, T (new branch, tag : 1 connection)' '
 	rm -f $U &&
 	(
 		cd cloned &&
-		GIT_TRACE_PACKET=$UPATH git fetch &&
+		GIT_TRACE_PACKET=$UPATH GIT_TEST_PROTOCOL_VERSION=0 git fetch &&
 		test $C = $(git rev-parse --verify origin/cat) &&
 		test $T = $(git rev-parse --verify tag1) &&
 		test $A = $(git rev-parse --verify tag1^0)

@@ -122,7 +122,7 @@ test_expect_success 'fetch B, S (commit and tag : 1 connection)' '
 	rm -f $U &&
 	(
 		cd cloned &&
-		GIT_TRACE_PACKET=$UPATH git fetch &&
+		GIT_TRACE_PACKET=$UPATH GIT_TEST_PROTOCOL_VERSION=0 git fetch &&
 		test $B = $(git rev-parse --verify origin/master) &&
 		test $B = $(git rev-parse --verify tag2^0) &&
 		test $S = $(git rev-parse --verify tag2)

@@ -146,7 +146,7 @@ test_expect_success 'new clone fetch master and tags' '
 		cd clone2 &&
 		git init &&
 		git remote add origin .. &&
-		GIT_TRACE_PACKET=$UPATH git fetch &&
+		GIT_TRACE_PACKET=$UPATH GIT_TEST_PROTOCOL_VERSION=0 git fetch &&
 		test $B = $(git rev-parse --verify origin/master) &&
 		test $S = $(git rev-parse --verify tag2) &&
 		test $B = $(git rev-parse --verify tag2^0) &&

index ca69636fd5..28420c4f77 100755
@@ -223,7 +223,7 @@ test_expect_success 'ls-remote --symref' '
 	$(git rev-parse refs/tags/mark1.10)	refs/tags/mark1.10
 	$(git rev-parse refs/tags/mark1.2)	refs/tags/mark1.2
 	EOF
-	git ls-remote --symref >actual &&
+	GIT_TEST_PROTOCOL_VERSION=0 git ls-remote --symref >actual &&
 	test_cmp expect actual
 '
 

@@ -243,7 +243,7 @@ test_expect_failure 'ls-remote with filtered symref (--heads)' '
 	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/heads/foo
 	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/heads/master
 	EOF
-	git ls-remote --symref --heads . >actual &&
+	GIT_TEST_PROTOCOL_VERSION=0 git ls-remote --symref --heads . >actual &&
 	test_cmp expect actual
 '
 

@@ -252,9 +252,9 @@ test_expect_success 'ls-remote --symref omits filtered-out matches' '
 	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/heads/foo
 	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/heads/master
 	EOF
-	git ls-remote --symref --heads . >actual &&
+	GIT_TEST_PROTOCOL_VERSION=0  git ls-remote --symref --heads . >actual &&
 	test_cmp expect actual &&
-	git ls-remote --symref . "refs/heads/*" >actual &&
+	GIT_TEST_PROTOCOL_VERSION=0 git ls-remote --symref . "refs/heads/*" >actual &&
 	test_cmp expect actual
 '
 

index 36b0dbc01c..2a3d1d84d6 100755
@@ -7,6 +7,7 @@
 test_description='Merge logic in fetch'
 
 . ./test-lib.sh
+sane_unset GIT_TEST_PROTOCOL_VERSION
 
 LF='
 '

index 49e5d305e5..0722d288cd 100755
@@ -1129,7 +1129,8 @@ do
 	'
 done
 
-test_expect_success 'fetch exact SHA1' '
+test_expect_success 'fetch exact SHA1 in protocol v0' '
+	sane_unset GIT_TEST_PROTOCOL_VERSION &&
 	mk_test testrepo heads/master hidden/one &&
 	git push testrepo master:refs/hidden/one &&
 	(

index 6faf17e17a..f8f14c0ca2 100755
@@ -127,7 +127,8 @@ test_expect_success 'fetch that requires changes in .git/shallow is filtered' '
 	git init notshallow &&
 	(
 	cd notshallow &&
-	git fetch ../shallow/.git refs/heads/*:refs/remotes/shallow/*&&
+	GIT_TEST_PROTOCOL_VERSION=0 \
+		git fetch ../shallow/.git refs/heads/*:refs/remotes/shallow/* &&
 	git for-each-ref --format="%(refname)" >actual.refs &&
 	cat <<EOF >expect.refs &&
 refs/remotes/shallow/no-shallow

index 30857b84a8..c5b39b8248 100755
@@ -127,6 +127,7 @@ test_expect_success 'use ref advertisement to filter out commits' '
 	# not need to send any ancestors of "c3", but we still need to send "c3"
 	# itself.
 	test_config -C client fetch.negotiationalgorithm skipping &&
+	sane_unset GIT_TEST_PROTOCOL_VERSION &&
 	trace_fetch client origin to_fetch &&
 	have_sent c5 c4^ c2side &&
 	have_not_sent c4 c4^^ c4^^^

index 336f02a41a..feedf84ce1 100755
@@ -144,7 +144,8 @@ test_expect_success 'manual prefetch of missing objects' '
 	sort >observed.oids &&
 
 	test_line_count = 6 observed.oids &&
-	git -C pc1 fetch-pack --stdin "file://$(pwd)/srv.bare" <observed.oids &&
+	GIT_TEST_PROTOCOL_VERSION=0 \
+		git -C pc1 fetch-pack --stdin "file://$(pwd)/srv.bare" <observed.oids &&
 
 	git -C pc1 rev-list --quiet --objects --missing=print \
 		master..origin/master >revs &&

index ba86a44eb1..244ff6879d 100755
@@ -5,6 +5,7 @@ test_description='test git wire-protocol transition'
 TEST_NO_CREATE_REPO=1
 
 . ./test-lib.sh
+sane_unset GIT_TEST_PROTOCOL_VERSION
 
 # Test protocol v1 with 'git://' transport
 #

index e87164aa8f..dd41a96c20 100755
@@ -943,7 +943,8 @@ test_expect_success 'submodule update clone shallow submodule outside of depth'
 		cd super3 &&
 		sed -e "s#url = ../#url = file://$pwd/#" <.gitmodules >.gitmodules.tmp &&
 		mv -f .gitmodules.tmp .gitmodules &&
-		test_must_fail git submodule update --init --depth=1 2>actual &&
+		test_must_fail env GIT_TEST_PROTOCOL_VERSION=0 \
+			git submodule update --init --depth=1 2>actual &&
 		test_i18ngrep "Direct fetching of that commit failed." actual &&
 		git -C ../submodule config uploadpack.allowReachableSHA1InWant true &&
 		git submodule update --init --depth=1 >actual &&

2.20.0.405.gbc1bbc6f85

Just realized that I haven't replied to this yet...

I think Aevar's patches (sent after you wrote this) is a good start, and I have started looking at it too.

Personally, I lean towards removing it, but there are arguments on both sides. In particular, removing "serve" means that both developers and users of Git need not be concerned with a 3rd endpoint, but preserving "serve" (and planning to migrate away from "upload-pack" and "receive-pack") means that we will only have one endpoint, eliminating confusion about which endpoint to use when making certain requests (when we add requests other than "fetch" and "push").

Yeah, I'm excited to see it working with fetch-pack, as the current behavior is to complain if you've tried to enable v2 config:

  $ git config protocol.version 2
  $ git fetch-pack git://github.com/git/git
  fatal: support for protocol v2 not implemented yet

I haven't actually run into it in the real world, but somebody might if they have scripted around fetch-pack and are experimenting with v2. A much friendlier behavior would be falling back to v1, but actually supporting v2 is better still. :)

Yeah, at first glance I like the simplicity of a unified model. But the separate fetch/push endpoints have been useful in the past. Separate uploadpack/receive.hiderefs that I dealt with here are one form. Another is that many people do HTTP access control using the endpoints. For example, if I have a repo which is public-read and private-write, the config we recommend in git-http-backend(1) is to lock down the receive-pack access using webserver config.

If all the webserver sees is "somebody wants to connect to git-serve", it doesn't know if it should be challenging them for authentication or not. It would have to start peeking into the git-serve conversation to see what the client actually wants to do. That's _possible_ to do, but it gets pretty awkward with existing webserver tools (whereas matching the URI endpoint is pretty easy).

Ditto for locked down ssh sessions like git-shell (or custom solutions like gitolite). Right now we can "git-upload-pack is OK on this repo, git-receive-pack is not". But blindly running "git serve" would be dangerous. In this case I think we have a few more options, because the user has always already authenticated. So we can just tell "git serve" via the environment whether the user is authorized for push. It's harder with HTTP because most setups avoid even challenging for auth unless it's necessary.

So I'm a bit worried that the unified endpoint model is going to be a dead end, at which point carrying around git-serve just makes things more complicated.

-Peff

This is from wetware memory of something discussed at a previous Git Merge, so I may have (inadvertently) made it up, but wasn't part of the idea of "git-serve" to have an extensible next-gen protocol where we could add new actions & verbs unrelated to sending or receiving packs?

E.g. client<->server optimistic cooperation like offloading a long-running "git-grep", "git log -G" etc. to a more powerful workhorse server, which would use "git-serve" as a routing layer.

Of course that's not in itself an argument for having a general "serve" command, actually the opposite for the reasons you mention with locking down things. E.g. maybe I want to support server-side git-grep on my server, but not git-log, and if it's one command it becomes a hassle to do that via SSH config or HTTPD config for the reasons you mention.

The upside would be that once a host understands "git serve" I'm more likely to be able to get past whatever middle layer there is between my client and the "git" binary on the other side. E.g. if I have a newly compiled "git" client/server binary, but something like GitLab's "gitaly" sitting between the two of us.

Yes, I think that's a goal (and we already have upload-archive, which is a similar thing).

Right, exactly. It pushes more of the information down into Git's own protocol. Of course we _can_ build mechanisms at that level for configuring which verbs are allowed. But if some context is available at the higher protocol level, then we can use the mechanisms at that higher level.

I think of it as a tradeoff. By including the endpoint in the transport protocol (e.g., in ssh the command name, in HTTP the URL), we get to use the mechanisms in those transports to make policy decisions on the server. But it also means we _have_ to implement those policies twice, once per transport.

IMHO having to deal with both transports is not that big a loss, considering that there are only two, and really not likely to be more.
git:// is already unauthenticated, and IMHO is mostly a dead-end for future protocol work since it provides no advantage over HTTP, and the future is mostly HTTP, with ssh for people who really prefer its authentication mode.

But I think that's what makes it dangerous, too. :)

Gitaly (and we have our own equivalent at GitHub) is responsible for making those policy decisions about who can run what. Opening a pipe between the client and the backend that can issue arbitrary verbs is exactly what they _don't_ want to do.

So they have to intercept the conversation at least at the verb level. It _is_ nice if conversation for each verb is standardized (so once a verb is issued, they can just step out of the way and proxy bytes[1]), and v2 helps with that.

That's not too hard for a Git-aware endpoint to implement. But when that verb interception can be done at the HTTP/ssh level, then it's easy for tools that _aren't_ Git-aware to do handle it (again, like the Apache config we recommend in git-http-backend(1)).

-Peff

[1] Actually, we do much more intimate interception than that at GitHub already. The upload-pack conversation is mostly vanilla, but for receive-pack we handle replication at that layer. So your pack is streamed to 3-6 backend receive-packs simultaneously, and that endpoint layer handles quorum for updating refs, etc.

Yeah I think overall this makes sense. I was just thinking we'd have stuff like this needing to be maintained in all middleware:
https://gitlab.com/gitlab-org/gitlab-shell/blob/v8.4.4/lib/gitlab_shell.rb#L15-28

Which, if and when we have a lot of verbs would be a pain, but of course server operators might want to explicitly whitelist them.

Also for things like "git-grep" I can see e.g. "no POSIX regex" (due to well known DoS issues0 being a configuration we ourselves would want to carry, at that point server operators would need to maintain two whitelists anyway, one in their custom code & another in /etc/gitconfig.

But I think that trade-off is worth it as you note because when you want to filter these it's handy to be able to do it in a dumb ssh/web server.

Another thing to consider is not having a proliferation of things in git-<TAB> completion again. AFAIK these things can't have spaces in them for /etc/passwd & inetd tab completion. So perhaps call them all git-serve-*, or not put them in our bin/ install path as a special case?