[PATCH 0/3] protocol v2 and hidden refs

[PATCH 0/3] protocol v2 and hidden refs

[Jeff King]

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

[PATCH 1/3] serve: pass "config context" through to individual commands

[Jeff King]

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

Signed-off-by: Jeff King <peff@peff.net>
---
 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(-)

diff --git a/builtin/upload-pack.c b/builtin/upload-pack.c
index 42dc4da5a1..67192cfa9e 100644
--- a/builtin/upload-pack.c
+++ b/builtin/upload-pack.c
@@ -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:
diff --git a/ls-refs.c b/ls-refs.c
index a06f12eca8..e8e31475f0 100644
--- a/ls-refs.c
+++ b/ls-refs.c
@@ -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;
diff --git a/ls-refs.h b/ls-refs.h
index b62877e8da..da26fc9824 100644
--- a/ls-refs.h
+++ b/ls-refs.h
@@ -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 */
diff --git a/serve.c b/serve.c
index bda085f09c..70f89cf0d9 100644
--- a/serve.c
+++ b/serve.c
@@ -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;
 	}
 }
diff --git a/serve.h b/serve.h
index fe65ba9f46..d527224cbb 100644
--- a/serve.h
+++ b/serve.h
@@ -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);
diff --git a/upload-pack.c b/upload-pack.c
index 5e81f1ff24..914bbb40bc 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -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;
diff --git a/upload-pack.h b/upload-pack.h
index cab2178796..2a9f51197c 100644
--- a/upload-pack.h
+++ b/upload-pack.h
@@ -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

Re: [PATCH 1/3] serve: pass "config context" through to individual commands

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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

> diff --git a/ls-refs.h b/ls-refs.h
> index b62877e8da..da26fc9824 100644
> --- a/ls-refs.h
> +++ b/ls-refs.h
> @@ -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);

One thing I wonder is if we want to pass the whole *_opt thing,
instead of only one field out of it.

>  #endif /* LS_REFS_H */
> diff --git a/serve.c b/serve.c
> index bda085f09c..70f89cf0d9 100644
> --- a/serve.c
> +++ b/serve.c
> @@ -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,

Likewise here.

>  		       struct argv_array *keys,
>  		       struct packet_reader *request);
>  };

Thanks.

Re: [PATCH 1/3] serve: pass "config context" through to individual commands

[Jeff King]

On Fri, Dec 14, 2018 at 11:09:53AM +0900, Junio C Hamano wrote:

> > diff --git a/ls-refs.h b/ls-refs.h
> > index b62877e8da..da26fc9824 100644
> > --- a/ls-refs.h
> > +++ b/ls-refs.h
> > @@ -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);
> 
> One thing I wonder is if we want to pass the whole *_opt thing,
> instead of only one field out of it.

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

Re: [PATCH 1/3] serve: pass "config context" through to individual commands

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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.

Re: [PATCH 1/3] serve: pass "config context" through to individual commands

[Jeff King]

On Sat, Dec 15, 2018 at 09:31:15AM +0900, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > 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).
> 
> 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".

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

Re: [PATCH 1/3] serve: pass "config context" through to individual commands

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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.

Re: [PATCH 1/3] serve: pass "config context" through to individual commands

[Jeff King]

On Sun, Dec 16, 2018 at 08:12:03PM +0900, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > 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).
> 
> Yes, I'd be 100% happy with that, too.  And it certainly is simpler.

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.

> 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.

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

Signed-off-by: Jeff King <peff@peff.net>
---
 ls-refs.c            | 16 ++++++++++++++++
 t/t5512-ls-remote.sh |  6 ++++++
 2 files changed, 22 insertions(+)

diff --git a/ls-refs.c b/ls-refs.c
index a06f12eca8..9c9a7c647f 100644
--- a/ls-refs.c
+++ b/ls-refs.c
@@ -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;
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 32e722db2e..ca69636fd5 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -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

Re: [PATCH 1/3] serve: pass "config context" through to individual commands

[Jonathan Nieder]

Hi,

Jeff King wrote:

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

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

Re: [PATCH 1/3] serve: pass "config context" through to individual commands

[Jeff King]

On Fri, Dec 14, 2018 at 12:36:21AM -0800, Jonathan Nieder wrote:

> Jeff King wrote:
> 
> > 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).
> 
> 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?

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

> 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.

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

Re: [PATCH 1/3] serve: pass "config context" through to individual commands

[Jonathan Nieder]

Jeff King wrote:
> On Fri, Dec 14, 2018 at 12:36:21AM -0800, Jonathan Nieder wrote:
>> Jeff King wrote:

>>> 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).
>>
>> 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?
>
> I don't know. Why doesn't the current "ls-refs" say "ls-refs for fetch"?

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.

> 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.

Interesting.  The last discussion of push v2 plans was in
https://public-inbox.org/git/20180717210915.139521-1-bmwill@google.com/.
Expect to hear more soon.

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

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?

>> 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.
>
> 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.

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

Thanks for explaining,
Jonathan

Re: [PATCH 1/3] serve: pass "config context" through to individual commands

[Jeff King]

On Fri, Dec 14, 2018 at 01:28:20AM -0800, Jonathan Nieder wrote:

> > 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.
> 
> Interesting.  The last discussion of push v2 plans was in
> https://public-inbox.org/git/20180717210915.139521-1-bmwill@google.com/.
> Expect to hear more soon.

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

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

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.

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

> > 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.
> 
> I think that's a reasonable assumption to make, especially if made
> explicit using a simple comment. :)

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.

Signed-off-by: Jeff King <peff@peff.net>
---
 ls-refs.c            | 16 ++++++++++++++++
 t/t5512-ls-remote.sh |  6 ++++++
 2 files changed, 22 insertions(+)

diff --git a/ls-refs.c b/ls-refs.c
index a06f12eca8..9c9a7c647f 100644
--- a/ls-refs.c
+++ b/ls-refs.c
@@ -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;
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 32e722db2e..ca69636fd5 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -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

[PATCH 2/3] parse_hide_refs_config: handle NULL section

[Jeff King]

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

Signed-off-by: Jeff King <peff@peff.net>
---
 refs.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/refs.c b/refs.c
index f9936355cd..099e91d9cc 100644
--- a/refs.c
+++ b/refs.c
@@ -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

Re: [PATCH 2/3] parse_hide_refs_config: handle NULL section

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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

[PATCH 3/3] upload-pack: support hidden refs with protocol v2

[Jeff King]

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.

Signed-off-by: Jeff King <peff@peff.net>
---
 ls-refs.c            | 12 ++++++++++++
 t/t5512-ls-remote.sh |  6 ++++++
 2 files changed, 18 insertions(+)

diff --git a/ls-refs.c b/ls-refs.c
index e8e31475f0..8a8143338b 100644
--- a/ls-refs.c
+++ b/ls-refs.c
@@ -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;
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 32e722db2e..ca69636fd5 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -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

Re: [PATCH 0/3] protocol v2 and hidden refs

[Ævar Arnfjörð Bjarmason]

On Tue, Dec 11 2018, Jeff King wrote:

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

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

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

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.

>  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

Re: [PATCH 0/3] protocol v2 and hidden refs

[Jeff King]

On Tue, Dec 11, 2018 at 12:45:16PM +0100, Ævar Arnfjörð Bjarmason wrote:

> >     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).
> 
> I think a global test mode for it would be a very good idea.

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

> > 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
> 
> 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.

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.

> 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.

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

-Peff

[PATCH 0/3] Add a GIT_TEST_PROTOCOL_VERSION=X test mode

[Ævar Arnfjörð Bjarmason]

On Tue, Dec 11 2018, Jeff King wrote:

> On Tue, Dec 11, 2018 at 12:45:16PM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>> >     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).
>> 
>> I think a global test mode for it would be a very good idea.
>
> Yeah, but somebody needs to pick through the dozens of false positives
> for it to be useful.

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

Re: [PATCH 0/3] Add a GIT_TEST_PROTOCOL_VERSION=X test mode

[Ævar Arnfjörð Bjarmason]

On Tue, Dec 11 2018, Ævar Arnfjörð Bjarmason wrote:

> On Tue, Dec 11 2018, Jeff King wrote:
>
>> On Tue, Dec 11, 2018 at 12:45:16PM +0100, Ævar Arnfjörð Bjarmason wrote:
>>
>>> >     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).
>>>
>>> I think a global test mode for it would be a very good idea.
>>
>> Yeah, but somebody needs to pick through the dozens of false positives
>> for it to be useful.
>
> 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.

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

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

[PATCH 1/3] tests: add a special setup where for protocol.version

[Ævar Arnfjörð Bjarmason]

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/20181211104236.GA6899@sigill.intra.peff.net/
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.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 protocol.c | 13 ++++++++++++-
 t/README   |  4 ++++
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/protocol.c b/protocol.c
index 5e636785d1..cb58cbb29a 100644
--- a/protocol.c
+++ b/protocol.c
@@ -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)
diff --git a/t/README b/t/README
index 28711cc508..c5762a92bc 100644
--- a/t/README
+++ b/t/README
@@ -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

[PATCH 0/3] Some fixes and improvements

[Jonathan Tan]

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

[PATCH 1/3] squash this into your patch

[Jonathan Tan]

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 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(-)

diff --git a/t/t5539-fetch-http-shallow.sh b/t/t5539-fetch-http-shallow.sh
index 5fbf67c446..996ce060cd 100755
--- a/t/t5539-fetch-http-shallow.sh
+++ b/t/t5539-fetch-http-shallow.sh
@@ -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
diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh
index 5475afc052..20f1308578 100755
--- a/t/t5541-http-push-smart.sh
+++ b/t/t5541-http-push-smart.sh
@@ -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
 	)
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index 8630b0cc39..56fd9351a6 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -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)"
diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
index 7466aad111..4a2a937bb0 100755
--- a/t/t5570-git-daemon.sh
+++ b/t/t5570-git-daemon.sh
@@ -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

[PATCH 2/3] builtin/fetch-pack: support protocol version 2

[Jonathan Tan]

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.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 builtin/fetch-pack.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 63e69a5801..f6a513495e 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -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

[PATCH 3/3] also squash this into your patch

[Jonathan Tan]

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 t/t5500-fetch-pack.sh    | 13 +++++++------
 t/t5616-partial-clone.sh |  3 +--
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 9c18875c9c..a5a8f348a2 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -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
diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
index feedf84ce1..336f02a41a 100755
--- a/t/t5616-partial-clone.sh
+++ b/t/t5616-partial-clone.sh
@@ -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

Re: [PATCH 0/3] Some fixes and improvements

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

> Thanks, Aevar for looking into this. I haven't looked into detail, but:
> ...
> 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. 

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

[PATCH v2 0/8] protocol v2 fixes

[Ævar Arnfjörð Bjarmason]

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

[PATCH v3 0/4] protocol v2 fixes

[Ævar Arnfjörð Bjarmason]

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/87h8ff20ol.fsf@evledraar.gmail.com/

Then my just-sent
https://public-inbox.org/git/20181217221625.1523-1-avarab@gmail.com/
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

Re: [PATCH v3 0/4] protocol v2 fixes

[Jeff King]

On Mon, Dec 17, 2018 at 11:40:50PM +0100, Ævar Arnfjörð Bjarmason wrote:

> 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

These should be dropped in favor of:

  https://public-inbox.org/git/20181218124750.GC30471@sigill.intra.peff.net/

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

-Peff

[PATCH v3 1/4] serve: pass "config context" through to individual commands

[Ævar Arnfjörð Bjarmason]

From: Jeff King <peff@peff.net>

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

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 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(-)

diff --git a/builtin/upload-pack.c b/builtin/upload-pack.c
index 42dc4da5a1f..67192cfa9e9 100644
--- a/builtin/upload-pack.c
+++ b/builtin/upload-pack.c
@@ -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:
diff --git a/ls-refs.c b/ls-refs.c
index a06f12eca8d..e8e31475f06 100644
--- a/ls-refs.c
+++ b/ls-refs.c
@@ -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;
diff --git a/ls-refs.h b/ls-refs.h
index b62877e8dad..da26fc9824d 100644
--- a/ls-refs.h
+++ b/ls-refs.h
@@ -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 */
diff --git a/serve.c b/serve.c
index bda085f09c8..70f89cf0d98 100644
--- a/serve.c
+++ b/serve.c
@@ -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;
 	}
 }
diff --git a/serve.h b/serve.h
index fe65ba9f469..d527224cbb1 100644
--- a/serve.h
+++ b/serve.h
@@ -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);
diff --git a/upload-pack.c b/upload-pack.c
index 5e81f1ff24f..914bbb40bcd 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -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;
diff --git a/upload-pack.h b/upload-pack.h
index cab2178796a..2a9f51197c4 100644
--- a/upload-pack.h
+++ b/upload-pack.h
@@ -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

[PATCH v3 2/4] parse_hide_refs_config: handle NULL section

[Ævar Arnfjörð Bjarmason]

From: Jeff King <peff@peff.net>

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

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 refs.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/refs.c b/refs.c
index f9936355cda..099e91d9cc0 100644
--- a/refs.c
+++ b/refs.c
@@ -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

[PATCH v3 3/4] upload-pack: support hidden refs with protocol v2

[Ævar Arnfjörð Bjarmason]

From: Jeff King <peff@peff.net>

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.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 ls-refs.c            | 12 ++++++++++++
 t/t5512-ls-remote.sh |  6 ++++++
 2 files changed, 18 insertions(+)

diff --git a/ls-refs.c b/ls-refs.c
index e8e31475f06..8a8143338b4 100644
--- a/ls-refs.c
+++ b/ls-refs.c
@@ -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;
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 32e722db2ed..ca69636fd52 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -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

[PATCH v3 4/4] fetch-pack: support protocol version 2

[Ævar Arnfjörð Bjarmason]

From: Jonathan Tan <jonathantanmy@google.com>

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/20181213155817.27666-1-avarab@gmail.com/

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/fetch-pack.c  |  9 ++++++---
 t/t5500-fetch-pack.sh | 22 +++++++++++++++-------
 2 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 63e69a58011..f6a513495ea 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -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);
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 086f2c40f68..49c540b1e1d 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -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

Re: [PATCH v3 4/4] fetch-pack: support protocol version 2

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> From: Jonathan Tan <jonathantanmy@google.com>


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?

> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
> index 086f2c40f68..49c540b1e1d 100755
> --- a/t/t5500-fetch-pack.sh
> +++ b/t/t5500-fetch-pack.sh
> @@ -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 &&

Re: [PATCH v3 4/4] fetch-pack: support protocol version 2

[Jonathan Tan]

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
> 
> > From: Jonathan Tan <jonathantanmy@google.com>
> 
> 
> 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.

Re: [PATCH v3 4/4] fetch-pack: support protocol version 2

[Jeff King]

On Tue, Jan 08, 2019 at 12:38:26PM -0800, Jonathan Tan wrote:

> > Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
> > 
> > > From: Jonathan Tan <jonathantanmy@google.com>
> > 
> > 
> > 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

[PATCH v2 1/8] serve: pass "config context" through to individual commands

[Ævar Arnfjörð Bjarmason]

From: Jeff King <peff@peff.net>

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

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 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(-)

diff --git a/builtin/upload-pack.c b/builtin/upload-pack.c
index 42dc4da5a1..67192cfa9e 100644
--- a/builtin/upload-pack.c
+++ b/builtin/upload-pack.c
@@ -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:
diff --git a/ls-refs.c b/ls-refs.c
index a06f12eca8..e8e31475f0 100644
--- a/ls-refs.c
+++ b/ls-refs.c
@@ -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;
diff --git a/ls-refs.h b/ls-refs.h
index b62877e8da..da26fc9824 100644
--- a/ls-refs.h
+++ b/ls-refs.h
@@ -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 */
diff --git a/serve.c b/serve.c
index bda085f09c..70f89cf0d9 100644
--- a/serve.c
+++ b/serve.c
@@ -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;
 	}
 }
diff --git a/serve.h b/serve.h
index fe65ba9f46..d527224cbb 100644
--- a/serve.h
+++ b/serve.h
@@ -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);
diff --git a/upload-pack.c b/upload-pack.c
index 5e81f1ff24..914bbb40bc 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -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;
diff --git a/upload-pack.h b/upload-pack.h
index cab2178796..2a9f51197c 100644
--- a/upload-pack.h
+++ b/upload-pack.h
@@ -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

[PATCH v2 2/8] parse_hide_refs_config: handle NULL section

[Ævar Arnfjörð Bjarmason]

From: Jeff King <peff@peff.net>

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

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 refs.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/refs.c b/refs.c
index f9936355cd..099e91d9cc 100644
--- a/refs.c
+++ b/refs.c
@@ -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

[PATCH v2 3/8] upload-pack: support hidden refs with protocol v2

[Ævar Arnfjörð Bjarmason]

From: Jeff King <peff@peff.net>

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.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 ls-refs.c            | 12 ++++++++++++
 t/t5512-ls-remote.sh |  6 ++++++
 2 files changed, 18 insertions(+)

diff --git a/ls-refs.c b/ls-refs.c
index e8e31475f0..8a8143338b 100644
--- a/ls-refs.c
+++ b/ls-refs.c
@@ -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;
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 32e722db2e..ca69636fd5 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -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

[PATCH v2 4/8] tests: add a check for unportable env --unset

[Ævar Arnfjörð Bjarmason]

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/cover.1544573604.git.jonathantanmy@google.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/check-non-portable-shell.pl | 1 +
 1 file changed, 1 insertion(+)

diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl
index b45bdac688..1cfb4608ee 100755
--- a/t/check-non-portable-shell.pl
+++ b/t/check-non-portable-shell.pl
@@ -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

[PATCH v2 5/8] tests: add a special setup where for protocol.version

[Ævar Arnfjörð Bjarmason]

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/20181211104236.GA6899@sigill.intra.peff.net/
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.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 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(-)

diff --git a/protocol.c b/protocol.c
index 5e636785d1..aa06c3b5bc 100644
--- a/protocol.c
+++ b/protocol.c
@@ -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)
diff --git a/t/README b/t/README
index 28711cc508..89629c5818 100644
--- a/t/README
+++ b/t/README
@@ -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
diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
index ba3887f178..8ba3d9b5ab 100755
--- a/t/t0410-partial-clone.sh
+++ b/t/t0410-partial-clone.sh
@@ -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
 '
 
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 37e8e80893..08cdee0b95 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -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
diff --git a/t/t5700-protocol-v1.sh b/t/t5700-protocol-v1.sh
index ba86a44eb1..e4d375c462 100755
--- a/t/t5700-protocol-v1.sh
+++ b/t/t5700-protocol-v1.sh
@@ -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
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 0f2b09ebb8..d1549f294e 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -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

Re: [PATCH v2 5/8] tests: add a special setup where for protocol.version

[Jonathan Tan]

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

> +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.

Optional: I prefer the "overrides" language used in
GIT_TEST_COMMIT_GRAPH to make it clear that any existing setting is
ignored. E.g.:

  GIT_TEST_PROTOCOL_VERSION=<'protocol.version' config value>, when set
  to a non-empty string, overrides the 'protocol.version' value. E.g.
  "0", ...

Other than that, this patch and the other patches (1-4, 6, 8) look good
to me. My patch 7 looks good too, but it's probably better if someone
else looks at it too :-)

[PATCH v2 6/8] tests: mark & fix tests broken under GIT_TEST_PROTOCOL_VERSION=1

[Ævar Arnfjörð Bjarmason]

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.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5400-send-pack.sh |  2 +-
 t/t5601-clone.sh     | 11 +++++++++--
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh
index f1932ea431..571d620aed 100755
--- a/t/t5400-send-pack.sh
+++ b/t/t5400-send-pack.sh
@@ -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 &&
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 8bbc7068ac..a7b7ab327c 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -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

[PATCH v2 7/8] builtin/fetch-pack: support protocol version 2

[Ævar Arnfjörð Bjarmason]

From: Jonathan Tan <jonathantanmy@google.com>

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.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/fetch-pack.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 63e69a5801..f6a513495e 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -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

Re: [PATCH v2 7/8] builtin/fetch-pack: support protocol version 2

[Jeff King]

On Thu, Dec 13, 2018 at 04:58:16PM +0100, Ævar Arnfjörð Bjarmason wrote:

> From: Jonathan Tan <jonathantanmy@google.com>
> 
> 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.

I'm definitely on board with this goal.

> @@ -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);

This implementation looks absurdly simple. So simple that it makes me
wonder why we did not just do this in the first place. I.e., is there
some hidden gotcha blocking it, or was it simply that all of the
necessary work happened in the meantime and nobody bothered to flip the
switch?

-Peff

[PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2

[Ævar Arnfjörð Bjarmason]

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.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 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(-)

diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 086f2c40f6..8b1217ea26 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -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
diff --git a/t/t5503-tagfollow.sh b/t/t5503-tagfollow.sh
index 4ca48f0276..0e90a90294 100755
--- a/t/t5503-tagfollow.sh
+++ b/t/t5503-tagfollow.sh
@@ -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) &&
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index ca69636fd5..7b480587c9 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -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
 '
 
diff --git a/t/t5515-fetch-merge-logic.sh b/t/t5515-fetch-merge-logic.sh
index 36b0dbc01c..f095555c3e 100755
--- a/t/t5515-fetch-merge-logic.sh
+++ b/t/t5515-fetch-merge-logic.sh
@@ -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" &&
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 08cdee0b95..1d1b717cd5 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -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
diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
index 6faf17e17a..85b3022ce6 100755
--- a/t/t5537-fetch-shallow.sh
+++ b/t/t5537-fetch-shallow.sh
@@ -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
diff --git a/t/t5539-fetch-http-shallow.sh b/t/t5539-fetch-http-shallow.sh
index 5fbf67c446..a121e19bc7 100755
--- a/t/t5539-fetch-http-shallow.sh
+++ b/t/t5539-fetch-http-shallow.sh
@@ -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
diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh
index 5475afc052..180c9005b7 100755
--- a/t/t5541-http-push-smart.sh
+++ b/t/t5541-http-push-smart.sh
@@ -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
 	)
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index 8630b0cc39..a51993f35a 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -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)"
diff --git a/t/t5552-skipping-fetch-negotiator.sh b/t/t5552-skipping-fetch-negotiator.sh
index 30857b84a8..979e6583d4 100755
--- a/t/t5552-skipping-fetch-negotiator.sh
+++ b/t/t5552-skipping-fetch-negotiator.sh
@@ -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
diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
index 7466aad111..c42ee245cd 100755
--- a/t/t5570-git-daemon.sh
+++ b/t/t5570-git-daemon.sh
@@ -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
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index e87164aa8f..a555e38845 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -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

Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2

[Ævar Arnfjörð Bjarmason]

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:

> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
> index 086f2c40f6..8b1217ea26 100755
> --- a/t/t5500-fetch-pack.sh
> +++ b/t/t5500-fetch-pack.sh
> @@ -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 &&

What? So the equivalent of uploadpack.allowAnySHA1InWant=true is on for
v2 all the time?

>  	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
> diff --git a/t/t5503-tagfollow.sh b/t/t5503-tagfollow.sh
> index 4ca48f0276..0e90a90294 100755
> --- a/t/t5503-tagfollow.sh
> +++ b/t/t5503-tagfollow.sh
> @@ -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) &&
> diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
> index ca69636fd5..7b480587c9 100755
> --- a/t/t5512-ls-remote.sh
> +++ b/t/t5512-ls-remote.sh
> @@ -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
>  '
>
> diff --git a/t/t5515-fetch-merge-logic.sh b/t/t5515-fetch-merge-logic.sh
> index 36b0dbc01c..f095555c3e 100755
> --- a/t/t5515-fetch-merge-logic.sh
> +++ b/t/t5515-fetch-merge-logic.sh

This one should really be looked at by someone more familiar with
v2. Looks scary that we have different merge results with it.

> @@ -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" &&
> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index 08cdee0b95..1d1b717cd5 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -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
>  		)
>  	'

Ditto all this stuff.

> [...]
> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
> index e87164aa8f..a555e38845 100755
> --- a/t/t7406-submodule-update.sh
> +++ b/t/t7406-submodule-update.sh
> @@ -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 &&

And this one and various other shallow things look odd, are shallow
fetches different under v2?

Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

>> +	# 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 &&
>
> What? So the equivalent of uploadpack.allowAnySHA1InWant=true is on for
> v2 all the time?

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

Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2

[Jeff King]

On Thu, Dec 13, 2018 at 05:08:43PM +0100, Ævar Arnfjörð Bjarmason wrote:

> Now that we have this maybe we should discuss why these tests show
> different things:
> 
> > diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
> > index 086f2c40f6..8b1217ea26 100755
> > --- a/t/t5500-fetch-pack.sh
> > +++ b/t/t5500-fetch-pack.sh
> > @@ -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 &&
> 
> What? So the equivalent of uploadpack.allowAnySHA1InWant=true is on for
> v2 all the time?

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

Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2

[Ævar Arnfjörð Bjarmason]

On Fri, Dec 14 2018, Jeff King wrote:

> On Thu, Dec 13, 2018 at 05:08:43PM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>> Now that we have this maybe we should discuss why these tests show
>> different things:
>>
>> > diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
>> > index 086f2c40f6..8b1217ea26 100755
>> > --- a/t/t5500-fetch-pack.sh
>> > +++ b/t/t5500-fetch-pack.sh
>> > @@ -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 &&
>>
>> What? So the equivalent of uploadpack.allowAnySHA1InWant=true is on for
>> v2 all the time?
>
> 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.

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.

Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2

[Ævar Arnfjörð Bjarmason]

On Fri, Dec 14 2018, Ævar Arnfjörð Bjarmason wrote:

> On Fri, Dec 14 2018, Jeff King wrote:
>
>> On Thu, Dec 13, 2018 at 05:08:43PM +0100, Ævar Arnfjörð Bjarmason wrote:
>>
>>> Now that we have this maybe we should discuss why these tests show
>>> different things:
>>>
>>> > diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
>>> > index 086f2c40f6..8b1217ea26 100755
>>> > --- a/t/t5500-fetch-pack.sh
>>> > +++ b/t/t5500-fetch-pack.sh
>>> > @@ -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 &&
>>>
>>> What? So the equivalent of uploadpack.allowAnySHA1InWant=true is on for
>>> v2 all the time?
>>
>> 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.
>
> 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.

Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2

[Jeff King]

On Fri, Dec 14, 2018 at 12:08:02PM +0100, Ævar Arnfjörð Bjarmason wrote:

> 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?

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

Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2

[Jeff King]

On Fri, Dec 14, 2018 at 11:55:30AM +0100, Ævar Arnfjörð Bjarmason wrote:

> > 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.
> 
> More importantly this bypasses the security guarantee we've had with the
> default of uploadpack.allowAnySHA1InWant=false.

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?).

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

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

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

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.

> 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.

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

[PATCH] upload-pack: turn on uploadpack.allowAnySHA1InWant=true

[Ævar Arnfjörð Bjarmason]

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/87pnu51kac.fsf@evledraar.gmail.com/
and related messages for further discussion.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 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(-)

diff --git a/Documentation/config/uploadpack.txt b/Documentation/config/uploadpack.txt
index ed1c835695c..854801bd6cc 100644
--- a/Documentation/config/uploadpack.txt
+++ b/Documentation/config/uploadpack.txt
@@ -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
diff --git a/Documentation/transfer-data-leaks.txt b/Documentation/transfer-data-leaks.txt
index 914bacc39e0..f8beb334b84 100644
--- a/Documentation/transfer-data-leaks.txt
+++ b/Documentation/transfer-data-leaks.txt
@@ -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].
diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
index ba3887f178b..a9d24a94d9f 100755
--- a/t/t0410-partial-clone.sh
+++ b/t/t0410-partial-clone.sh
@@ -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" &&
diff --git a/t/t1090-sparse-checkout-scope.sh b/t/t1090-sparse-checkout-scope.sh
index 090b7fc3d35..4de2e2dd1bb 100755
--- a/t/t1090-sparse-checkout-scope.sh
+++ b/t/t1090-sparse-checkout-scope.sh
@@ -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 &&
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 086f2c40f68..b639629863d 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -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 () {
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 32e722db2ed..0cd1aad9597 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -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' '
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 37e8e80893d..8f2b12c17a5 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1147,26 +1147,14 @@ test_expect_success 'fetch exact SHA1' '
 		git prune &&
 		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_i18ngrep "Server does not allow request for unadvertised object" err &&
-		test_must_fail git rev-parse --verify refs/heads/copy &&
+		# fetching the object should work
+		git fetch -v ../testrepo $the_commit:refs/heads/copy &&
+		git rev-parse --verify refs/heads/copy &&
 
-		# the server side can allow it to succeed
-		(
-			cd ../testrepo &&
-			git config uploadpack.allowtipsha1inwant true
-		) &&
-
-		git fetch -v ../testrepo $the_commit:refs/heads/copy master:refs/heads/extra &&
 		cat >expect <<-EOF &&
 		$the_commit
-		$the_first_commit
 		EOF
-		{
-			git rev-parse --verify refs/heads/copy &&
-			git rev-parse --verify refs/heads/extra
-		} >actual &&
+		git rev-parse --verify refs/heads/copy >actual &&
 		test_cmp expect actual
 	)
 '
@@ -1190,6 +1178,25 @@ test_expect_success 'fetch exact SHA1 in protocol v2' '
 	git -C child fetch -v ../testrepo $the_commit:refs/heads/copy
 '
 
+for state in true false
+do
+	test_expect_success "Deprecated uploadpack.*=$state settings warn" "
+		mk_empty deprecated-warnings &&
+		test_commit -C deprecated-warnings A &&
+		test_config -C deprecated-warnings uploadpack.allowTipSHA1InWant $state &&
+		test_config -C deprecated-warnings uploadpack.allowReachableSHA1InWant $state &&
+		test_config -C deprecated-warnings uploadpack.allowAnySHA1InWant $state &&
+		mk_empty target &&
+		(
+			cd target &&
+			git fetch ../deprecated-warnings/.git refs/tags/A:refs/tags/A 2>stderr &&
+			test_i18ngrep 'warning: .*uploadpack.allowTipSHA1InWant .*deprecated' stderr &&
+			test_i18ngrep 'warning: .*uploadpack.allowReachableSHA1InWant.* deprecated' stderr &&
+			test_i18ngrep 'warning: .*uploadpack.allowAnySHA1InWant.* deprecated' stderr
+		)
+	"
+done
+
 for configallowtipsha1inwant in true false
 do
 	test_expect_success "shallow fetch reachable SHA1 (but not a ref), allowtipsha1inwant=$configallowtipsha1inwant" '
@@ -1204,8 +1211,6 @@ do
 		mk_empty shallow &&
 		(
 			cd shallow &&
-			test_must_fail 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
 		)
@@ -1232,15 +1237,10 @@ 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 &&
-			git --git-dir=../testrepo/.git config uploadpack.allowreachablesha1inwant true &&
+			git fetch ../testrepo/.git $SHA1_3 &&
 			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
+			git cat-file commit $SHA1_2
 		)
 	'
 done
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index 8630b0cc390..b8f7c0b5563 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -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)"
 '
 
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 8bbc7068acb..ec7a3c46528 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -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
diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
index 336f02a41a6..213899a690d 100755
--- a/t/t5616-partial-clone.sh
+++ b/t/t5616-partial-clone.sh
@@ -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) &&
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 0f2b09ebb8d..af16f139a4a 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -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
 '
 
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index e87164aa8ff..cf5bda08690 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -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
 	)
diff --git a/upload-pack.c b/upload-pack.c
index 5e81f1ff24f..18e4c2d2452 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -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

Re: [PATCH] upload-pack: turn on uploadpack.allowAnySHA1InWant=true

[David Turner]

Overall, I like this. One nit:

On December 17, 2018 5:16:25 PM EST, "Ævar Arnfjörð Bjarmason" <avarab@gmail.com> wrote:
>--- a/upload-pack.c
>+++ b/upload-pack.c
>@@ -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);

ALLOW_ANY_SHA1 already includes the other two.

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

Re: [PATCH] upload-pack: turn on uploadpack.allowAnySHA1InWant=true

[Ævar Arnfjörð Bjarmason]

On Mon, Dec 17 2018, David Turner wrote:

> Overall, I like this. One nit:

Thanks for the review!

> On December 17, 2018 5:16:25 PM EST, "Ævar Arnfjörð Bjarmason" <avarab@gmail.com> wrote:
>>--- a/upload-pack.c
>>+++ b/upload-pack.c
>>@@ -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);
>
> ALLOW_ANY_SHA1 already includes the other two.

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

Re: [PATCH] upload-pack: turn on uploadpack.allowAnySHA1InWant=true

[David Turner]

On December 17, 2018 5:57:50 PM EST, "Ævar Arnfjörð Bjarmason" <avarab@gmail.com> wrote:
>
>On Mon, Dec 17 2018, David Turner wrote:
>
>> Overall, I like this. One nit:
>
>Thanks for the review!
>
>> On December 17, 2018 5:16:25 PM EST, "Ævar Arnfjörð Bjarmason"
><avarab@gmail.com> wrote:
>>>--- a/upload-pack.c
>>>+++ b/upload-pack.c
>>>@@ -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);
>>
>> ALLOW_ANY_SHA1 already includes the other two.
>
>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.

Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2

[Jonathan Nieder]

Hi,

Jeff King wrote:
> On Fri, Dec 14, 2018 at 11:55:30AM +0100, Ævar Arnfjörð Bjarmason wrote:

>> More importantly this bypasses the security guarantee we've had with the
>> default of uploadpack.allowAnySHA1InWant=false.
>
> 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).

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.

> 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.

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

[...]
>> 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.
>
> No real argument from me. I have always thought those security
> guarantees were BS.

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

Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2

[Ævar Arnfjörð Bjarmason]

On Mon, Dec 17 2018, Jonathan Nieder wrote:

> Hi,
>
> Jeff King wrote:
>> On Fri, Dec 14, 2018 at 11:55:30AM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>>> More importantly this bypasses the security guarantee we've had with the
>>> default of uploadpack.allowAnySHA1InWant=false.
>>
>> 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).
>
> 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.
>
>> 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.
>
> IMHO it's a plain bug (either in implementation or documentation).
>
> [...]
>>> 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.
>>
>> No real argument from me. I have always thought those security
>> guarantees were BS.
>
> This would make per-branch ACLs (as implemented both by Gerrit and
> gitolite) an essentially useless feature, so please no.

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
<20181217221625.1523-1-avarab@gmail.com> 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).

> 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

Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2

[Jonathan Nieder]

Ævar Arnfjörð Bjarmason wrote:
> On Mon, Dec 17 2018, Jonathan Nieder wrote:

>> This would make per-branch ACLs (as implemented both by Gerrit and
>> gitolite) an essentially useless feature, so please no.
>
> 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.

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

> Like you I'm curious about a POC. The patch I submitted in
> <20181217221625.1523-1-avarab@gmail.com> takes the "SECURITY" docs at
> face value.

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

Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2

[Ævar Arnfjörð Bjarmason]

On Tue, Dec 18 2018, Jonathan Nieder wrote:

> Ævar Arnfjörð Bjarmason wrote:
>> On Mon, Dec 17 2018, Jonathan Nieder wrote:
>
>>> This would make per-branch ACLs (as implemented both by Gerrit and
>>> gitolite) an essentially useless feature, so please no.
>>
>> 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.
>
> 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.

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/20170516203712.15921-1-avarab@gmail.com/

> 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).
>
>> Like you I'm curious about a POC. The patch I submitted in
>> <20181217221625.1523-1-avarab@gmail.com> takes the "SECURITY" docs at
>> face value.
>
> 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.

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?

Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2

[Jeff King]

On Tue, Dec 18, 2018 at 10:28:27AM +0100, Ævar Arnfjörð Bjarmason wrote:

> 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

Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2

[Jeff King]

On Mon, Dec 17, 2018 at 03:14:52PM -0800, Jonathan Nieder wrote:

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

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.

> > 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.
> 
> IMHO it's a plain bug (either in implementation or documentation).

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

> [...]
> >> 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.
> >
> > No real argument from me. I have always thought those security
> > guarantees were BS.
> 
> This would make per-branch ACLs (as implemented both by Gerrit and
> gitolite) an essentially useless feature, so please no.

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

> 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.

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.

> 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?

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

-Peff

Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2

[Ævar Arnfjörð Bjarmason]

On Tue, Dec 18 2018, Jeff King wrote:

> On Mon, Dec 17, 2018 at 03:14:52PM -0800, Jonathan Nieder wrote:
>
>> > 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).
>>
>> 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.
>
> 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.
>
>> > 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.
>>
>> IMHO it's a plain bug (either in implementation or documentation).
>
> Or both. :) The behavior and the documentation seem to agree.
>
>> [...]
>> >> 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.
>> >
>> > No real argument from me. I have always thought those security
>> > guarantees were BS.
>>
>> This would make per-branch ACLs (as implemented both by Gerrit and
>> gitolite) an essentially useless feature, so please no.
>
> 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").

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?

>> 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.
>
> 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.
>
>> 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?
>
> It's not clear to me that there's consensus on the invariants or goals.
> ;)
>
> -Peff

Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> 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? ;-)

Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2

[Ævar Arnfjörð Bjarmason]

On Wed, Dec 26 2018, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> 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.

Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2

[Jonathan Nieder]

Hi,

Ævar Arnfjörð Bjarmason wrote:
> On Wed, Dec 26 2018, Junio C Hamano wrote:

>> 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? ;-)

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

> 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.

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/20181217231452.GA13835@google.com/

[PATCH 2/3] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=1

[Ævar Arnfjörð Bjarmason]

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.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 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(-)

diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
index ba3887f178..786f96c467 100755
--- a/t/t0410-partial-clone.sh
+++ b/t/t0410-partial-clone.sh
@@ -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 &&
diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh
index f1932ea431..b84618c925 100755
--- a/t/t5400-send-pack.sh
+++ b/t/t5400-send-pack.sh
@@ -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 &&
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 37e8e80893..49e5d305e5 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -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 &&
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 8bbc7068ac..a9ce050ee9 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -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
 '
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 0f2b09ebb8..df7cc2a43a 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -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

[PATCH 3/3] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2

[Ævar Arnfjörð Bjarmason]

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.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 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(-)

diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 086f2c40f6..9c18875c9c 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -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)
diff --git a/t/t5503-tagfollow.sh b/t/t5503-tagfollow.sh
index 4ca48f0276..220c677f24 100755
--- a/t/t5503-tagfollow.sh
+++ b/t/t5503-tagfollow.sh
@@ -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) &&
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index ca69636fd5..28420c4f77 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -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
 '
 
diff --git a/t/t5515-fetch-merge-logic.sh b/t/t5515-fetch-merge-logic.sh
index 36b0dbc01c..2a3d1d84d6 100755
--- a/t/t5515-fetch-merge-logic.sh
+++ b/t/t5515-fetch-merge-logic.sh
@@ -7,6 +7,7 @@
 test_description='Merge logic in fetch'
 
 . ./test-lib.sh
+sane_unset GIT_TEST_PROTOCOL_VERSION
 
 LF='
 '
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 49e5d305e5..0722d288cd 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -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 &&
 	(
diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
index 6faf17e17a..f8f14c0ca2 100755
--- a/t/t5537-fetch-shallow.sh
+++ b/t/t5537-fetch-shallow.sh
@@ -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
diff --git a/t/t5552-skipping-fetch-negotiator.sh b/t/t5552-skipping-fetch-negotiator.sh
index 30857b84a8..c5b39b8248 100755
--- a/t/t5552-skipping-fetch-negotiator.sh
+++ b/t/t5552-skipping-fetch-negotiator.sh
@@ -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^^^
diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
index 336f02a41a..feedf84ce1 100755
--- a/t/t5616-partial-clone.sh
+++ b/t/t5616-partial-clone.sh
@@ -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 &&
diff --git a/t/t5700-protocol-v1.sh b/t/t5700-protocol-v1.sh
index ba86a44eb1..244ff6879d 100755
--- a/t/t5700-protocol-v1.sh
+++ b/t/t5700-protocol-v1.sh
@@ -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
 #
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index e87164aa8f..dd41a96c20 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -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

Re: [PATCH 0/3] protocol v2 and hidden refs

[Jonathan Tan]

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

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

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

>   - 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.

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

Re: [PATCH 0/3] protocol v2 and hidden refs

[Jeff King]

On Thu, Dec 13, 2018 at 11:53:05AM -0800, Jonathan Tan wrote:

> >     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).
> 
> I think Aevar's patches (sent after you wrote this) is a good start, and
> I have started looking at it too.

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. :)

> >   - 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.
> 
> 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, 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

Re: [PATCH 0/3] protocol v2 and hidden refs

[Ævar Arnfjörð Bjarmason]

On Fri, Dec 14 2018, Jeff King wrote:

> On Thu, Dec 13, 2018 at 11:53:05AM -0800, Jonathan Tan wrote:
>
>> >     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).
>>
>> I think Aevar's patches (sent after you wrote this) is a good start, and
>> I have started looking at it too.
>
> 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. :)
>
>> >   - 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.
>>
>> 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, 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.

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.

Re: [PATCH 0/3] protocol v2 and hidden refs

[Jeff King]

On Sat, Dec 15, 2018 at 08:53:35PM +0100, Ævar Arnfjörð Bjarmason wrote:

> > 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.
> 
> 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?

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

> 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.

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.

> 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.

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.

Re: [PATCH 0/3] protocol v2 and hidden refs

[Ævar Arnfjörð Bjarmason]

On Sun, Dec 16 2018, Jeff King wrote:

> On Sat, Dec 15, 2018 at 08:53:35PM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>> > 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.
>>
>> 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?
>
> Yes, I think that's a goal (and we already have upload-archive, which is
> a similar thing).
>
>> 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.
>
> 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.
>
>> 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.
>
> 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?