| Message ID | 20181211104342.GA7233@sigill.intra.peff.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | protocol v2 and hidden refs | expand |
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.
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
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
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
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
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 &&
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.
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
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.
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 &&
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,
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(-)