[1/3] serve: pass "config context" through to individual commands
diff mbox series

Message ID 20181211104342.GA7233@sigill.intra.peff.net
State New
Headers show
Series
  • protocol v2 and hidden refs
Related show

Commit Message

Jeff King Dec. 11, 2018, 10:43 a.m. UTC
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(-)

Comments

Junio C Hamano Dec. 14, 2018, 2:09 a.m. UTC | #1
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.
Jeff King Dec. 14, 2018, 8:20 a.m. UTC | #2
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
Jonathan Nieder Dec. 14, 2018, 8:36 a.m. UTC | #3
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
Jeff King Dec. 14, 2018, 8:55 a.m. UTC | #4
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
Jonathan Nieder Dec. 14, 2018, 9:28 a.m. UTC | #5
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
Jeff King Dec. 14, 2018, 9:55 a.m. UTC | #6
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 &&
Junio C Hamano Dec. 15, 2018, 12:31 a.m. UTC | #7
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.
Jeff King Dec. 16, 2018, 10:25 a.m. UTC | #8
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
Junio C Hamano Dec. 16, 2018, 11:12 a.m. UTC | #9
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.
Jeff King Dec. 18, 2018, 12:47 p.m. UTC | #10
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 &&

Patch
diff mbox series

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,