{"thread":{"id":"50006","subject":"[PATCH 0/3] protocol v2 and hidden refs","startedAt":"2018-12-11T10:43:02Z","lastAt":"2019-01-08T21:14:06Z","messageCount":73,"participants":["Jeff King","Ævar Arnfjörð Bjarmason","Jonathan Tan","Junio C Hamano","Jonathan Nieder","David Turner"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"365057","messageId":"20181211104236.GA6899@sigill.intra.peff.net","threadId":"50006","inReplyTo":null,"subject":"[PATCH 0/3] protocol v2 and hidden refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-12-11T10:42:36Z","receivedAt":"2018-12-11T10:43:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When using the v2 protocol, hidden-ref config is not respected at all:\n\n $ git config transfer.hiderefs refs/tags/\n $ git -c protocol.version=0 ls-remote . | grep -c refs/tags\n 0\n $ git -c protocol.version=2 ls-remote . | grep -c refs/tags\n 1424\n\nThe fix in patch 3 is pretty straightforward, but note:\n\n - I'm a little worried this may happen again with future features. The\n root cause is that \"ls-refs\" follows a different code path than the\n ref advertisement for \"upload-pack\". So if we add any new config,\n it needs to go both places (non ref-advertisement config is OK, as\n the v2 \"fetch\" command is a lot closer to a v0 upload-pack).\n\n I think this is just an issue for any future features. I looked for\n other existing features which might be missing in v2, but couldn't\n find any.\n\n I don't know if there's a good solution. I tried running the whole\n test suite with v2 as the default. It does find this bug, but it has\n a bunch of other problems (notably fetch-pack won't run as v2, but\n some other tests I think also depend on v0's reachability rules,\n which v2 is documented not to enforce).\n\n - The \"serve\" command is funky, because it has no concept of whether\n the \"ls-refs\" is for fetching or pushing. Is git-serve even a thing\n that we want to support going forward? I know part of the original\n v2 conception was that one would be able to just connect to\n \"git-serve\" and do a number of operations. But in practice the v2\n probing requires saying \"I'd like to git-upload-pack, and v2 if you\n please\". So no client ever calls git-serve.\n\n Is this something we plan to eventually move to? Or can it be\n considered a funny vestige of the development? In the latter case, I\n think we should consider removing it.\n\n I've worked around it here with patch 2, but note that \"git serve\"\n would not ever respect uploadpack.hiderefs nor receive.hiderefs\n (since it has no idea which operation it's doing).\n\nThe patches are:\n\n [1/3]: serve: pass \"config context\" through to individual commands\n [2/3]: parse_hide_refs_config: handle NULL section\n [3/3]: upload-pack: support hidden refs with protocol v2\n\n builtin/upload-pack.c | 1 +\n ls-refs.c | 16 +++++++++++++++-\n ls-refs.h | 3 ++-\n refs.c | 3 ++-\n serve.c | 9 +++++----\n serve.h | 7 +++++++\n t/t5512-ls-remote.sh | 6 ++++++\n upload-pack.c | 4 ++--\n upload-pack.h | 4 ++--\n 9 files changed, 42 insertions(+), 11 deletions(-)\n\n-Peff\n"},{"id":"365058","messageId":"20181211104342.GA7233@sigill.intra.peff.net","threadId":"50006","inReplyTo":"20181211104236.GA6899@sigill.intra.peff.net","subject":"[PATCH 1/3] serve: pass \"config context\" through to individual commands","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-12-11T10:43:42Z","receivedAt":"2018-12-11T10:44:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In protocol v2, instead of just running \"upload-pack\", we have a generic\n\"serve\" loop which runs command requests from the client. What used to\nbe \"upload-pack\" is now generally split into two operations: \"ls-refs\"\nand \"fetch\". The latter knows it must respect uploadpack.* config, but\nthe former is not actually specific to a fetch operation (we do not yet\ndo v2 receive-pack, but eventually we may, and ls-refs would support\nboth operations).\n\nHowever, ls-refs does need to know which operation we're performing, in\norder to read the correct config (e.g., uploadpack.hiderefs versus\nreceive.hiderefs; we don't read _either_ right now, but this is the\nfirst step to fixing that).\n\nIn the generic \"git-serve\" program, we don't have that information. But\nin the protocol as it is actually used by clients, the client still asks\nto run \"git-upload-pack\", and then issues an \"ls-refs\" from there. So we\n_do_ know at that point that \"uploadpack\" is the right config context.\nThis patch teaches the protocol v2 \"serve\" code to pass that context\nthrough to the individual commands (a future patch will teach ls-refs to\nactually use it).\n\nSigned-off-by: Jeff King \n---\n builtin/upload-pack.c | 1 +\n ls-refs.c | 4 +++-\n ls-refs.h | 3 ++-\n serve.c | 9 +++++----\n serve.h | 7 +++++++\n upload-pack.c | 4 ++--\n upload-pack.h | 4 ++--\n 7 files changed, 22 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/upload-pack.c b/builtin/upload-pack.c\nindex 42dc4da5a1..67192cfa9e 100644\n--- a/builtin/upload-pack.c\n+++ b/builtin/upload-pack.c\n@@ -52,6 +52,7 @@ int cmd_upload_pack(int argc, const char **argv, const char *prefix)\n \tcase protocol_v2:\n \t\tserve_opts.advertise_capabilities = opts.advertise_refs;\n \t\tserve_opts.stateless_rpc = opts.stateless_rpc;\n+\t\tserve_opts.config_context = \"uploadpack\";\n \t\tserve(&serve_opts);\n \t\tbreak;\n \tcase protocol_v1:\ndiff --git a/ls-refs.c b/ls-refs.c\nindex a06f12eca8..e8e31475f0 100644\n--- a/ls-refs.c\n+++ b/ls-refs.c\n@@ -69,7 +69,9 @@ static int send_ref(const char *refname, const struct object_id *oid,\n \treturn 0;\n }\n \n-int ls_refs(struct repository *r, struct argv_array *keys,\n+int ls_refs(struct repository *r,\n+\t const char *config_section,\n+\t struct argv_array *keys,\n \t struct packet_reader *request)\n {\n \tstruct ls_refs_data data;\ndiff --git a/ls-refs.h b/ls-refs.h\nindex b62877e8da..da26fc9824 100644\n--- a/ls-refs.h\n+++ b/ls-refs.h\n@@ -4,7 +4,8 @@\n struct repository;\n struct argv_array;\n struct packet_reader;\n-extern int ls_refs(struct repository *r, struct argv_array *keys,\n+extern int ls_refs(struct repository *r, const char *config_context,\n+\t\t struct argv_array *keys,\n \t\t struct packet_reader *request);\n \n #endif /* LS_REFS_H */\ndiff --git a/serve.c b/serve.c\nindex bda085f09c..70f89cf0d9 100644\n--- a/serve.c\n+++ b/serve.c\n@@ -48,6 +48,7 @@ struct protocol_capability {\n \t * This field should be NULL for capabilities which are not commands.\n \t */\n \tint (*command)(struct repository *r,\n+\t\t const char *config_context,\n \t\t struct argv_array *keys,\n \t\t struct packet_reader *request);\n };\n@@ -158,7 +159,7 @@ enum request_state {\n \tPROCESS_REQUEST_DONE,\n };\n \n-static int process_request(void)\n+static int process_request(struct serve_options *opts)\n {\n \tenum request_state state = PROCESS_REQUEST_KEYS;\n \tstruct packet_reader reader;\n@@ -222,7 +223,7 @@ static int process_request(void)\n \tif (!command)\n \t\tdie(\"no command requested\");\n \n-\tcommand->command(the_repository, &keys, &reader);\n+\tcommand->command(the_repository, opts->config_context, &keys, &reader);\n \n \targv_array_clear(&keys);\n \treturn 0;\n@@ -249,10 +250,10 @@ void serve(struct serve_options *options)\n \t * a single request/response exchange\n \t */\n \tif (options->stateless_rpc) {\n-\t\tprocess_request();\n+\t\tprocess_request(options);\n \t} else {\n \t\tfor (;;)\n-\t\t\tif (process_request())\n+\t\t\tif (process_request(options))\n \t\t\t\tbreak;\n \t}\n }\ndiff --git a/serve.h b/serve.h\nindex fe65ba9f46..d527224cbb 100644\n--- a/serve.h\n+++ b/serve.h\n@@ -8,6 +8,13 @@ extern int has_capability(const struct argv_array *keys, const char *capability,\n struct serve_options {\n \tunsigned advertise_capabilities;\n \tunsigned stateless_rpc;\n+\n+\t/*\n+\t * Some operations may need to know the context when looking up config;\n+\t * e.g., set this to \"uploadpack\" to respect \"uploadpack.hiderefs\" (as\n+\t * opposed to \"receive.hiderefs\").\n+\t */\n+\tconst char *config_context;\n };\n #define SERVE_OPTIONS_INIT { 0 }\n extern void serve(struct serve_options *options);\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 5e81f1ff24..914bbb40bc 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -1413,8 +1413,8 @@ enum fetch_state {\n \tFETCH_DONE,\n };\n \n-int upload_pack_v2(struct repository *r, struct argv_array *keys,\n-\t\t struct packet_reader *request)\n+int upload_pack_v2(struct repository *r, const char *config_context,\n+\t\t struct argv_array *keys, struct packet_reader *request)\n {\n \tenum fetch_state state = FETCH_PROCESS_ARGS;\n \tstruct upload_pack_data data;\ndiff --git a/upload-pack.h b/upload-pack.h\nindex cab2178796..2a9f51197c 100644\n--- a/upload-pack.h\n+++ b/upload-pack.h\n@@ -13,8 +13,8 @@ void upload_pack(struct upload_pack_options *options);\n struct repository;\n struct argv_array;\n struct packet_reader;\n-extern int upload_pack_v2(struct repository *r, struct argv_array *keys,\n-\t\t\t struct packet_reader *request);\n+extern int upload_pack_v2(struct repository *r, const char *config_context,\n+\t\t\t struct argv_array *keys, struct packet_reader *request);\n \n struct strbuf;\n extern int upload_pack_advertise(struct repository *r,\n-- \n2.20.0.734.gb4f2c0d2be\n\n"},{"id":"365059","messageId":"20181211104358.GB7233@sigill.intra.peff.net","threadId":"50006","inReplyTo":"20181211104236.GA6899@sigill.intra.peff.net","subject":"[PATCH 2/3] parse_hide_refs_config: handle NULL section","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-12-11T10:43:58Z","receivedAt":"2018-12-11T10:44:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This helper function looks for config in two places: transfer.hiderefs,\nor $section.hiderefs, where $section is passed in by the caller (and is\n\"uploadpack\" or \"receive\", depending on the context).\n\nIn preparation for callers which do not even have that context (namely\nthe \"git-serve\" command), let's handle a NULL by looking only at\ntransfer.hiderefs (as opposed to segfaulting).\n\nSigned-off-by: Jeff King \n---\n refs.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/refs.c b/refs.c\nindex f9936355cd..099e91d9cc 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1267,7 +1267,8 @@ int parse_hide_refs_config(const char *var, const char *value, const char *secti\n {\n \tconst char *key;\n \tif (!strcmp(\"transfer.hiderefs\", var) ||\n-\t (!parse_config_key(var, section, NULL, NULL, &key) &&\n+\t (section &&\n+\t !parse_config_key(var, section, NULL, NULL, &key) &&\n \t !strcmp(key, \"hiderefs\"))) {\n \t\tchar *ref;\n \t\tint len;\n-- \n2.20.0.734.gb4f2c0d2be\n\n"},{"id":"365060","messageId":"20181211104439.GC7233@sigill.intra.peff.net","threadId":"50006","inReplyTo":"20181211104236.GA6899@sigill.intra.peff.net","subject":"[PATCH 3/3] upload-pack: support hidden refs with protocol v2","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-12-11T10:44:39Z","receivedAt":"2018-12-11T10:45:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In the v2 protocol, upload-pack's advertisement has been moved to the\n\"ls-refs\" command. That command does not respect hidden-ref config (like\ntransfer.hiderefs) at all, and advertises everything.\n\nWhile there are some features that are not supported in v2 (e.g., v2\nalways allows fetching any sha1 without respect to advertisements), the\nlack of this feature is not documented and is likely just a bug. Let's\nmake it work, as otherwise upgrading a server to a v2-capable git will\nstart exposing these refs that the repository admin has asked to remain\nhidden.\n\nNote that we depend on the config_context from the caller here to\nrealize that we should respect uploadpack.hiderefs. When run from the\n\"git-serve\" tool, we won't have that context and will only respect\ntransfer.hiderefs. This should be OK, as git-serve is not actually used\nfor normal protocol operations.\n\nSigned-off-by: Jeff King \n---\n ls-refs.c | 12 ++++++++++++\n t/t5512-ls-remote.sh | 6 ++++++\n 2 files changed, 18 insertions(+)\n\ndiff --git a/ls-refs.c b/ls-refs.c\nindex e8e31475f0..8a8143338b 100644\n--- a/ls-refs.c\n+++ b/ls-refs.c\n@@ -5,6 +5,7 @@\n #include \"argv-array.h\"\n #include \"ls-refs.h\"\n #include \"pkt-line.h\"\n+#include \"config.h\"\n \n /*\n * Check if one of the prefixes is a prefix of the ref.\n@@ -40,6 +41,9 @@ static int send_ref(const char *refname, const struct object_id *oid,\n \tconst char *refname_nons = strip_namespace(refname);\n \tstruct strbuf refline = STRBUF_INIT;\n \n+\tif (ref_is_hidden(refname_nons, refname))\n+\t\treturn 0;\n+\n \tif (!ref_match(&data->prefixes, refname))\n \t\treturn 0;\n \n@@ -69,6 +73,12 @@ static int send_ref(const char *refname, const struct object_id *oid,\n \treturn 0;\n }\n \n+static int ls_refs_config(const char *var, const char *value,\n+\t\t\t void *config_context)\n+{\n+\treturn parse_hide_refs_config(var, value, config_context);\n+}\n+\n int ls_refs(struct repository *r,\n \t const char *config_section,\n \t struct argv_array *keys,\n@@ -78,6 +88,8 @@ int ls_refs(struct repository *r,\n \n \tmemset(&data, 0, sizeof(data));\n \n+\tgit_config(ls_refs_config, (void *)config_section);\n+\n \twhile (packet_reader_read(request) != PACKET_READ_FLUSH) {\n \t\tconst char *arg = request->line;\n \t\tconst char *out;\ndiff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh\nindex 32e722db2e..ca69636fd5 100755\n--- a/t/t5512-ls-remote.sh\n+++ b/t/t5512-ls-remote.sh\n@@ -204,6 +204,12 @@ test_expect_success 'overrides work between mixed transfer/upload-pack hideRefs'\n \tgrep refs/tags/magic actual\n '\n \n+test_expect_success 'protocol v2 supports hiderefs' '\n+\ttest_config uploadpack.hiderefs refs/tags &&\n+\tgit -c protocol.version=2 ls-remote . >actual &&\n+\t! grep refs/tags actual\n+'\n+\n test_expect_success 'ls-remote --symref' '\n \tgit fetch origin &&\n \tcat >expect <<-EOF &&\n-- \n2.20.0.734.gb4f2c0d2be\n"},{"id":"365067","messageId":"875zw0nv77.fsf@evledraar.gmail.com","threadId":"50006","inReplyTo":"20181211104236.GA6899@sigill.intra.peff.net","subject":"Re: [PATCH 0/3] protocol v2 and hidden refs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-12-11T11:45:16Z","receivedAt":"2018-12-11T11:45:21Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Dec 11 2018, Jeff King wrote:\n\n> When using the v2 protocol, hidden-ref config is not respected at all:\n>\n> $ git config transfer.hiderefs refs/tags/\n> $ git -c protocol.version=0 ls-remote . | grep -c refs/tags\n> 0\n> $ git -c protocol.version=2 ls-remote . | grep -c refs/tags\n> 1424\n>\n> The fix in patch 3 is pretty straightforward, but note:\n>\n> - I'm a little worried this may happen again with future features. The\n> root cause is that \"ls-refs\" follows a different code path than the\n> ref advertisement for \"upload-pack\". So if we add any new config,\n> it needs to go both places (non ref-advertisement config is OK, as\n> the v2 \"fetch\" command is a lot closer to a v0 upload-pack).\n>\n> I think this is just an issue for any future features. I looked for\n> other existing features which might be missing in v2, but couldn't\n> find any.\n>\n> I don't know if there's a good solution. I tried running the whole\n> test suite with v2 as the default. It does find this bug, but it has\n> a bunch of other problems (notably fetch-pack won't run as v2, but\n> some other tests I think also depend on v0's reachability rules,\n> which v2 is documented not to enforce).\n\nI think a global test mode for it would be a very good idea.\n\n> - The \"serve\" command is funky, because it has no concept of whether\n> the \"ls-refs\" is for fetching or pushing. Is git-serve even a thing\n> that we want to support going forward? I know part of the original\n> v2 conception was that one would be able to just connect to\n> \"git-serve\" and do a number of operations. But in practice the v2\n> probing requires saying \"I'd like to git-upload-pack, and v2 if you\n> please\". So no client ever calls git-serve.\n>\n> Is this something we plan to eventually move to? Or can it be\n> considered a funny vestige of the development? In the latter case, I\n> think we should consider removing it.\n>\n> I've worked around it here with patch 2, but note that \"git serve\"\n> would not ever respect uploadpack.hiderefs nor receive.hiderefs\n> (since it has no idea which operation it's doing).\n>\n> The patches are:\n>\n> [1/3]: serve: pass \"config context\" through to individual commands\n> [2/3]: parse_hide_refs_config: handle NULL section\n> [3/3]: upload-pack: support hidden refs with protocol v2\n\nDoes this issue rise to the level of needing a security point-release\n(which I'm discussing here as the details are already public). The\ntransfer.hideRefs docs have said:\n\n Even if you hide refs, a client may still be able to steal the\n target objects via the techniques described in the \"SECURITY\"\n section of the gitnamespaces(7) man page; it’s best to keep private\n data in a separate repository.\n\nSo we never promised to hide the objects, but definitely promised to\nhide the ref names. I don't know if anyone uses this in practice for\nsecret ref names, but if they do they have a data leak if they enable\nprotocol v2.\n\nMore importantly, the docs for receive.hideRefs say. \"An attempt to\nupdate or delete a hidden ref by git push is rejected.\". It seems this\nbit was enforced, i.e. this passes before and after your 3/3, but I have\nnot dug enough to be 100% satisfied with that.\n\n diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh\n index ca69636fd5..20059c3308 100755\n --- a/t/t5512-ls-remote.sh\n +++ b/t/t5512-ls-remote.sh\n @@ -210,6 +210,13 @@ test_expect_success 'protocol v2 supports hiderefs' '\n \t! grep refs/tags actual\n '\n\n +test_expect_success 'protocol v2 respects hiderefs when pushing' '\n +\tgit init --bare server.git &&\n +\tgit -C server.git config transfer.hideRefs refs/tags &&\n +\ttest_must_fail git -c protocol.version=0 push \"file://$PWD/server.git\" mark &&\n +\ttest_must_fail git -c protocol.version=2 push \"file://$PWD/server.git\" mark\n +'\n +\n test_expect_success 'ls-remote --symref' '\n \tgit fetch origin &&\n \tcat >expect <<-EOF &&\n\nIf there's some bug where you can bypass this push protection that would\nbe much worse. E.g. GitLab uses \"keep-around\" refs to track its own\ninternal state, and it would be bad if users could manipulate it.\n\n> builtin/upload-pack.c | 1 +\n> ls-refs.c | 16 +++++++++++++++-\n> ls-refs.h | 3 ++-\n> refs.c | 3 ++-\n> serve.c | 9 +++++----\n> serve.h | 7 +++++++\n> t/t5512-ls-remote.sh | 6 ++++++\n> upload-pack.c | 4 ++--\n> upload-pack.h | 4 ++--\n> 9 files changed, 42 insertions(+), 11 deletions(-)\n>\n> -Peff\n"},{"id":"365078","messageId":"20181211135501.GA13731@sigill.intra.peff.net","threadId":"50006","inReplyTo":"875zw0nv77.fsf@evledraar.gmail.com","subject":"Re: [PATCH 0/3] protocol v2 and hidden refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-12-11T13:55:01Z","receivedAt":"2018-12-11T13:55:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 11, 2018 at 12:45:16PM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> > I don't know if there's a good solution. I tried running the whole\n> > test suite with v2 as the default. It does find this bug, but it has\n> > a bunch of other problems (notably fetch-pack won't run as v2, but\n> > some other tests I think also depend on v0's reachability rules,\n> > which v2 is documented not to enforce).\n> \n> I think a global test mode for it would be a very good idea.\n\nYeah, but somebody needs to pick through the dozens of false positives\nfor it to be useful.\n\n> > The patches are:\n> >\n> > [1/3]: serve: pass \"config context\" through to individual commands\n> > [2/3]: parse_hide_refs_config: handle NULL section\n> > [3/3]: upload-pack: support hidden refs with protocol v2\n> \n> Does this issue rise to the level of needing a security point-release\n> (which I'm discussing here as the details are already public). The\n> transfer.hideRefs docs have said:\n> \n> Even if you hide refs, a client may still be able to steal the\n> target objects via the techniques described in the \"SECURITY\"\n> section of the gitnamespaces(7) man page; it’s best to keep private\n> data in a separate repository.\n> \n> So we never promised to hide the objects, but definitely promised to\n> hide the ref names. I don't know if anyone uses this in practice for\n> secret ref names, but if they do they have a data leak if they enable\n> protocol v2.\n\nYes, that was my line of thinking. You can't really consider such items\nsecure, though it is unfortunate that this leak makes it way easier to\naccess them (you can just fetch them, rather than playing\noracle-guessing games with deltas).\n\nAt GitHub we keep some internal book-keeping refs, but exposing them to\nthe user is mostly an annoyance.\n\nOne thing to note is that there's no \"enable protocol v2\". If you're\nrunning a recent enough Git (v2.18+?) on the server, anybody can ask for\nthese refs.\n\n> More importantly, the docs for receive.hideRefs say. \"An attempt to\n> update or delete a hidden ref by git push is rejected.\". It seems this\n> bit was enforced, i.e. this passes before and after your 3/3, but I have\n> not dug enough to be 100% satisfied with that.\n\nThis part is OK. There is no v2 push protocol yet, so you end up running\nthe regular v0 receive-pack.\n\n-Peff\n"},{"id":"365119","messageId":"20181211212135.21126-1-avarab@gmail.com","threadId":"50006","inReplyTo":"20181211135501.GA13731@sigill.intra.peff.net","subject":"[PATCH 0/3] Add a GIT_TEST_PROTOCOL_VERSION=X test mode","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-12-11T21:21:32Z","receivedAt":"2018-12-11T21:21:49Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Tue, Dec 11 2018, Jeff King wrote:\n\n> On Tue, Dec 11, 2018 at 12:45:16PM +0100, Ævar Arnfjörð Bjarmason wrote:\n>\n>> > I don't know if there's a good solution. I tried running the whole\n>> > test suite with v2 as the default. It does find this bug, but it has\n>> > a bunch of other problems (notably fetch-pack won't run as v2, but\n>> > some other tests I think also depend on v0's reachability rules,\n>> > which v2 is documented not to enforce).\n>> \n>> I think a global test mode for it would be a very good idea.\n>\n> Yeah, but somebody needs to pick through the dozens of false positives\n> for it to be useful.\n\nHere's that test mode. As noted in 3/3 there may be more bugs revealed\nby this, but let's first start by marking where the behavior differs.\n\nÆvar Arnfjörð Bjarmason (3):\n tests: add a special setup where for protocol.version\n tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=1\n tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2\n\n protocol.c | 13 ++++++++++++-\n t/README | 4 ++++\n t/t0410-partial-clone.sh | 1 +\n t/t5400-send-pack.sh | 2 +-\n t/t5500-fetch-pack.sh | 4 +++-\n t/t5503-tagfollow.sh | 8 ++++----\n t/t5512-ls-remote.sh | 8 ++++----\n t/t5515-fetch-merge-logic.sh | 1 +\n t/t5516-fetch-push.sh | 4 +++-\n t/t5537-fetch-shallow.sh | 3 ++-\n t/t5552-skipping-fetch-negotiator.sh | 1 +\n t/t5601-clone.sh | 1 +\n t/t5616-partial-clone.sh | 3 ++-\n t/t5700-protocol-v1.sh | 1 +\n t/t5702-protocol-v2.sh | 1 +\n t/t7406-submodule-update.sh | 3 ++-\n 16 files changed, 43 insertions(+), 15 deletions(-)\n\n-- \n2.20.0.405.gbc1bbc6f85\n\n"},{"id":"365120","messageId":"20181211212135.21126-2-avarab@gmail.com","threadId":"50006","inReplyTo":"20181211135501.GA13731@sigill.intra.peff.net","subject":"[PATCH 1/3] tests: add a special setup where for protocol.version","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-12-11T21:21:33Z","receivedAt":"2018-12-11T21:21:52Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a GIT_TEST_PROTOCOL_VERSION=X test mode which is equivalent to\nrunning with protocol.version=X. This is needed to spot regressions\nand differences such as \"ls-refs\" behaving differently with\ntransfer.hideRefs. See\nhttps://public-inbox.org/git/20181211104236.GA6899@sigill.intra.peff.net/\nfor a fix for that regression.\n\nWith this all tests pass with GIT_TEST_PROTOCOL_VERSION=0, but fail\nwith GIT_TEST_PROTOCOL_VERSION=[1|2]. That's OK since this is a new\ntest mode, subsequent patches will fix up these test failures.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n protocol.c | 13 ++++++++++++-\n t/README | 4 ++++\n 2 files changed, 16 insertions(+), 1 deletion(-)\n\ndiff --git a/protocol.c b/protocol.c\nindex 5e636785d1..cb58cbb29a 100644\n--- a/protocol.c\n+++ b/protocol.c\n@@ -17,7 +17,18 @@ static enum protocol_version parse_protocol_version(const char *value)\n enum protocol_version get_protocol_version_config(void)\n {\n \tconst char *value;\n-\tif (!git_config_get_string_const(\"protocol.version\", &value)) {\n+\tconst char *git_test_k = \"GIT_TEST_PROTOCOL_VERSION\";\n+\tconst char *git_test_v = getenv(git_test_k);\n+\n+\tif (git_test_v) {\n+\t\tenum protocol_version version = parse_protocol_version(git_test_v);\n+\n+\t\tif (version == protocol_unknown_version)\n+\t\t\tdie(\"unknown value for %s: %s\", git_test_k,\n+\t\t\t git_test_v);\n+\n+\t\treturn version;\n+\t} else if (!git_config_get_string_const(\"protocol.version\", &value)) {\n \t\tenum protocol_version version = parse_protocol_version(value);\n \n \t\tif (version == protocol_unknown_version)\ndiff --git a/t/README b/t/README\nindex 28711cc508..c5762a92bc 100644\n--- a/t/README\n+++ b/t/README\n@@ -358,6 +358,10 @@ GIT_TEST_MULTI_PACK_INDEX=, when true, forces the multi-pack-\n index to be written after every 'git repack' command, and overrides the\n 'core.multiPackIndex' setting to true.\n \n+GIT_TEST_PROTOCOL_VERSION=<'protocol.version' config value>, when set,\n+runs the test suite with the given protocol.version. E.g. \"0\", \"1\" or\n+\"2\".\n+\n Naming Tests\n ------------\n \n-- \n2.20.0.405.gbc1bbc6f85\n\n"},{"id":"365121","messageId":"20181211212135.21126-3-avarab@gmail.com","threadId":"50006","inReplyTo":"20181211135501.GA13731@sigill.intra.peff.net","subject":"[PATCH 2/3] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=1","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-12-11T21:21:34Z","receivedAt":"2018-12-11T21:21:55Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"A few tests are broken under GIT_TEST_PROTOCOL_VERSION=1, which as\nprotocol.version in git-config(1) notes is just the\nGIT_TEST_PROTOCOL_VERSION=0 with a version number.\n\nAll of these cases look OK to me, and don't seem to show any\nregressions or other behavior differences that are unexpected. These\ntests are either testing exact v0 trace output, or trying to test the\nv2 protocol.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t0410-partial-clone.sh | 1 +\n t/t5400-send-pack.sh | 2 +-\n t/t5516-fetch-push.sh | 1 +\n t/t5601-clone.sh | 1 +\n t/t5702-protocol-v2.sh | 1 +\n 5 files changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex ba3887f178..786f96c467 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -171,6 +171,7 @@ test_expect_success 'fetching of missing objects' '\n '\n \n test_expect_success 'fetching of missing objects works with ref-in-want enabled' '\n+\tsane_unset GIT_TEST_PROTOCOL_VERSION &&\n \t# ref-in-want requires protocol version 2\n \tgit -C server config protocol.version 2 &&\n \tgit -C server config uploadpack.allowrefinwant 1 &&\ndiff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh\nindex f1932ea431..b84618c925 100755\n--- a/t/t5400-send-pack.sh\n+++ b/t/t5400-send-pack.sh\n@@ -288,7 +288,7 @@ test_expect_success 'receive-pack de-dupes .have lines' '\n \t$shared .have\n \tEOF\n \n-\tGIT_TRACE_PACKET=$(pwd)/trace \\\n+\tGIT_TRACE_PACKET=$(pwd)/trace GIT_TEST_PROTOCOL_VERSION=0 \\\n \t git push \\\n \t\t--receive-pack=\"unset GIT_TRACE_PACKET; git-receive-pack\" \\\n \t\tfork HEAD:foo &&\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 37e8e80893..49e5d305e5 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -1172,6 +1172,7 @@ test_expect_success 'fetch exact SHA1' '\n '\n \n test_expect_success 'fetch exact SHA1 in protocol v2' '\n+\tsane_unset GIT_TEST_PROTOCOL_VERSION &&\n \tmk_test testrepo heads/master hidden/one &&\n \tgit push testrepo master:refs/hidden/one &&\n \tgit -C testrepo config transfer.hiderefs refs/hidden &&\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex 8bbc7068ac..a9ce050ee9 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -345,6 +345,7 @@ expect_ssh () {\n }\n \n test_expect_success 'clone myhost:src uses ssh' '\n+\tsane_unset GIT_TEST_PROTOCOL_VERSION &&\n \tgit clone myhost:src ssh-clone &&\n \texpect_ssh myhost src\n '\ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex 0f2b09ebb8..df7cc2a43a 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -5,6 +5,7 @@ test_description='test git wire-protocol version 2'\n TEST_NO_CREATE_REPO=1\n \n . ./test-lib.sh\n+sane_unset GIT_TEST_PROTOCOL_VERSION\n \n # Test protocol v2 with 'git://' transport\n #\n-- \n2.20.0.405.gbc1bbc6f85\n\n"},{"id":"365122","messageId":"20181211212135.21126-4-avarab@gmail.com","threadId":"50006","inReplyTo":"20181211135501.GA13731@sigill.intra.peff.net","subject":"[PATCH 3/3] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-12-11T21:21:35Z","receivedAt":"2018-12-11T21:21:57Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Mark those tests that have behavior differences or bugs under\nprotocol.version=0.\n\nWhether or not these tests should exhibit different behavior is\noutside the scope of this change. Some (such as t5700-protocol-v1.sh)\nclearly should, but others (such as t7406-submodule-update.sh) might\nindicate bugs in the protocol v2 code.\n\nTracking down which is which is outside the scope of this\nchange. Let's first exhaustively annotate where the differences are,\nso that we can spot future behavior differences or regressions.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t5500-fetch-pack.sh | 4 +++-\n t/t5503-tagfollow.sh | 8 ++++----\n t/t5512-ls-remote.sh | 8 ++++----\n t/t5515-fetch-merge-logic.sh | 1 +\n t/t5516-fetch-push.sh | 3 ++-\n t/t5537-fetch-shallow.sh | 3 ++-\n t/t5552-skipping-fetch-negotiator.sh | 1 +\n t/t5616-partial-clone.sh | 3 ++-\n t/t5700-protocol-v1.sh | 1 +\n t/t7406-submodule-update.sh | 3 ++-\n 10 files changed, 22 insertions(+), 13 deletions(-)\n\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex 086f2c40f6..9c18875c9c 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -41,7 +41,8 @@ pull_to_client () {\n \ttest_expect_success \"$number pull\" '\n \t\t(\n \t\t\tcd client &&\n-\t\t\tgit fetch-pack -k -v .. $heads &&\n+\t\t\tGIT_TEST_PROTOCOL_VERSION=0 \\\n+\t\t\t\tgit fetch-pack -k -v .. $heads &&\n \n \t\t\tcase \"$heads\" in\n \t\t\t *A*)\n@@ -440,6 +441,7 @@ test_expect_success 'setup tests for the --stdin parameter' '\n '\n \n test_expect_success 'fetch refs from cmdline' '\n+\tsane_unset GIT_TEST_PROTOCOL_VERSION &&\n \t(\n \t\tcd client &&\n \t\tgit fetch-pack --no-progress .. $(cat ../input)\ndiff --git a/t/t5503-tagfollow.sh b/t/t5503-tagfollow.sh\nindex 4ca48f0276..220c677f24 100755\n--- a/t/t5503-tagfollow.sh\n+++ b/t/t5503-tagfollow.sh\n@@ -56,7 +56,7 @@ test_expect_success 'fetch A (new commit : 1 connection)' '\n \trm -f $U &&\n \t(\n \t\tcd cloned &&\n-\t\tGIT_TRACE_PACKET=$UPATH git fetch &&\n+\t\tGIT_TRACE_PACKET=$UPATH GIT_TEST_PROTOCOL_VERSION=0 git fetch &&\n \t\ttest $A = $(git rev-parse --verify origin/master)\n \t) &&\n \tget_needs $U >actual &&\n@@ -86,7 +86,7 @@ test_expect_success 'fetch C, T (new branch, tag : 1 connection)' '\n \trm -f $U &&\n \t(\n \t\tcd cloned &&\n-\t\tGIT_TRACE_PACKET=$UPATH git fetch &&\n+\t\tGIT_TRACE_PACKET=$UPATH GIT_TEST_PROTOCOL_VERSION=0 git fetch &&\n \t\ttest $C = $(git rev-parse --verify origin/cat) &&\n \t\ttest $T = $(git rev-parse --verify tag1) &&\n \t\ttest $A = $(git rev-parse --verify tag1^0)\n@@ -122,7 +122,7 @@ test_expect_success 'fetch B, S (commit and tag : 1 connection)' '\n \trm -f $U &&\n \t(\n \t\tcd cloned &&\n-\t\tGIT_TRACE_PACKET=$UPATH git fetch &&\n+\t\tGIT_TRACE_PACKET=$UPATH GIT_TEST_PROTOCOL_VERSION=0 git fetch &&\n \t\ttest $B = $(git rev-parse --verify origin/master) &&\n \t\ttest $B = $(git rev-parse --verify tag2^0) &&\n \t\ttest $S = $(git rev-parse --verify tag2)\n@@ -146,7 +146,7 @@ test_expect_success 'new clone fetch master and tags' '\n \t\tcd clone2 &&\n \t\tgit init &&\n \t\tgit remote add origin .. &&\n-\t\tGIT_TRACE_PACKET=$UPATH git fetch &&\n+\t\tGIT_TRACE_PACKET=$UPATH GIT_TEST_PROTOCOL_VERSION=0 git fetch &&\n \t\ttest $B = $(git rev-parse --verify origin/master) &&\n \t\ttest $S = $(git rev-parse --verify tag2) &&\n \t\ttest $B = $(git rev-parse --verify tag2^0) &&\ndiff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh\nindex ca69636fd5..28420c4f77 100755\n--- a/t/t5512-ls-remote.sh\n+++ b/t/t5512-ls-remote.sh\n@@ -223,7 +223,7 @@ test_expect_success 'ls-remote --symref' '\n \t$(git rev-parse refs/tags/mark1.10)\trefs/tags/mark1.10\n \t$(git rev-parse refs/tags/mark1.2)\trefs/tags/mark1.2\n \tEOF\n-\tgit ls-remote --symref >actual &&\n+\tGIT_TEST_PROTOCOL_VERSION=0 git ls-remote --symref >actual &&\n \ttest_cmp expect actual\n '\n \n@@ -243,7 +243,7 @@ test_expect_failure 'ls-remote with filtered symref (--heads)' '\n \t1bd44cb9d13204b0fe1958db0082f5028a16eb3a\trefs/heads/foo\n \t1bd44cb9d13204b0fe1958db0082f5028a16eb3a\trefs/heads/master\n \tEOF\n-\tgit ls-remote --symref --heads . >actual &&\n+\tGIT_TEST_PROTOCOL_VERSION=0 git ls-remote --symref --heads . >actual &&\n \ttest_cmp expect actual\n '\n \n@@ -252,9 +252,9 @@ test_expect_success 'ls-remote --symref omits filtered-out matches' '\n \t1bd44cb9d13204b0fe1958db0082f5028a16eb3a\trefs/heads/foo\n \t1bd44cb9d13204b0fe1958db0082f5028a16eb3a\trefs/heads/master\n \tEOF\n-\tgit ls-remote --symref --heads . >actual &&\n+\tGIT_TEST_PROTOCOL_VERSION=0 git ls-remote --symref --heads . >actual &&\n \ttest_cmp expect actual &&\n-\tgit ls-remote --symref . \"refs/heads/*\" >actual &&\n+\tGIT_TEST_PROTOCOL_VERSION=0 git ls-remote --symref . \"refs/heads/*\" >actual &&\n \ttest_cmp expect actual\n '\n \ndiff --git a/t/t5515-fetch-merge-logic.sh b/t/t5515-fetch-merge-logic.sh\nindex 36b0dbc01c..2a3d1d84d6 100755\n--- a/t/t5515-fetch-merge-logic.sh\n+++ b/t/t5515-fetch-merge-logic.sh\n@@ -7,6 +7,7 @@\n test_description='Merge logic in fetch'\n \n . ./test-lib.sh\n+sane_unset GIT_TEST_PROTOCOL_VERSION\n \n LF='\n '\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 49e5d305e5..0722d288cd 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -1129,7 +1129,8 @@ do\n \t'\n done\n \n-test_expect_success 'fetch exact SHA1' '\n+test_expect_success 'fetch exact SHA1 in protocol v0' '\n+\tsane_unset GIT_TEST_PROTOCOL_VERSION &&\n \tmk_test testrepo heads/master hidden/one &&\n \tgit push testrepo master:refs/hidden/one &&\n \t(\ndiff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh\nindex 6faf17e17a..f8f14c0ca2 100755\n--- a/t/t5537-fetch-shallow.sh\n+++ b/t/t5537-fetch-shallow.sh\n@@ -127,7 +127,8 @@ test_expect_success 'fetch that requires changes in .git/shallow is filtered' '\n \tgit init notshallow &&\n \t(\n \tcd notshallow &&\n-\tgit fetch ../shallow/.git refs/heads/*:refs/remotes/shallow/*&&\n+\tGIT_TEST_PROTOCOL_VERSION=0 \\\n+\t\tgit fetch ../shallow/.git refs/heads/*:refs/remotes/shallow/* &&\n \tgit for-each-ref --format=\"%(refname)\" >actual.refs &&\n \tcat <expect.refs &&\n refs/remotes/shallow/no-shallow\ndiff --git a/t/t5552-skipping-fetch-negotiator.sh b/t/t5552-skipping-fetch-negotiator.sh\nindex 30857b84a8..c5b39b8248 100755\n--- a/t/t5552-skipping-fetch-negotiator.sh\n+++ b/t/t5552-skipping-fetch-negotiator.sh\n@@ -127,6 +127,7 @@ test_expect_success 'use ref advertisement to filter out commits' '\n \t# not need to send any ancestors of \"c3\", but we still need to send \"c3\"\n \t# itself.\n \ttest_config -C client fetch.negotiationalgorithm skipping &&\n+\tsane_unset GIT_TEST_PROTOCOL_VERSION &&\n \ttrace_fetch client origin to_fetch &&\n \thave_sent c5 c4^ c2side &&\n \thave_not_sent c4 c4^^ c4^^^\ndiff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\nindex 336f02a41a..feedf84ce1 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -144,7 +144,8 @@ test_expect_success 'manual prefetch of missing objects' '\n \tsort >observed.oids &&\n \n \ttest_line_count = 6 observed.oids &&\n-\tgit -C pc1 fetch-pack --stdin \"file://$(pwd)/srv.bare\" revs &&\ndiff --git a/t/t5700-protocol-v1.sh b/t/t5700-protocol-v1.sh\nindex ba86a44eb1..244ff6879d 100755\n--- a/t/t5700-protocol-v1.sh\n+++ b/t/t5700-protocol-v1.sh\n@@ -5,6 +5,7 @@ test_description='test git wire-protocol transition'\n TEST_NO_CREATE_REPO=1\n \n . ./test-lib.sh\n+sane_unset GIT_TEST_PROTOCOL_VERSION\n \n # Test protocol v1 with 'git://' transport\n #\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex e87164aa8f..dd41a96c20 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -943,7 +943,8 @@ test_expect_success 'submodule update clone shallow submodule outside of depth'\n \t\tcd super3 &&\n \t\tsed -e \"s#url = ../#url = file://$pwd/#\" <.gitmodules >.gitmodules.tmp &&\n \t\tmv -f .gitmodules.tmp .gitmodules &&\n-\t\ttest_must_fail git submodule update --init --depth=1 2>actual &&\n+\t\ttest_must_fail env GIT_TEST_PROTOCOL_VERSION=0 \\\n+\t\t\tgit submodule update --init --depth=1 2>actual &&\n \t\ttest_i18ngrep \"Direct fetching of that commit failed.\" actual &&\n \t\tgit -C ../submodule config uploadpack.allowReachableSHA1InWant true &&\n \t\tgit submodule update --init --depth=1 >actual &&\n-- \n2.20.0.405.gbc1bbc6f85\n\n"},{"id":"365123","messageId":"87y38vn4dp.fsf@evledraar.gmail.com","threadId":"50006","inReplyTo":"20181211212135.21126-1-avarab@gmail.com","subject":"Re: [PATCH 0/3] Add a GIT_TEST_PROTOCOL_VERSION=X test mode","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-12-11T21:24:34Z","receivedAt":"2018-12-11T21:24:39Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Dec 11 2018, Ævar Arnfjörð Bjarmason wrote:\n\n> On Tue, Dec 11 2018, Jeff King wrote:\n>\n>> On Tue, Dec 11, 2018 at 12:45:16PM +0100, Ævar Arnfjörð Bjarmason wrote:\n>>\n>>> > I don't know if there's a good solution. I tried running the whole\n>>> > test suite with v2 as the default. It does find this bug, but it has\n>>> > a bunch of other problems (notably fetch-pack won't run as v2, but\n>>> > some other tests I think also depend on v0's reachability rules,\n>>> > which v2 is documented not to enforce).\n>>>\n>>> I think a global test mode for it would be a very good idea.\n>>\n>> Yeah, but somebody needs to pick through the dozens of false positives\n>> for it to be useful.\n>\n> Here's that test mode. As noted in 3/3 there may be more bugs revealed\n> by this, but let's first start by marking where the behavior differs.\n\n...and I forgot to mention. This goes on top of Jeff's series here to\nfix this \"hidden refs\" case.\n\n> Ævar Arnfjörð Bjarmason (3):\n> tests: add a special setup where for protocol.version\n> tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=1\n> tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2\n>\n> protocol.c | 13 ++++++++++++-\n> t/README | 4 ++++\n> t/t0410-partial-clone.sh | 1 +\n> t/t5400-send-pack.sh | 2 +-\n> t/t5500-fetch-pack.sh | 4 +++-\n> t/t5503-tagfollow.sh | 8 ++++----\n> t/t5512-ls-remote.sh | 8 ++++----\n> t/t5515-fetch-merge-logic.sh | 1 +\n> t/t5516-fetch-push.sh | 4 +++-\n> t/t5537-fetch-shallow.sh | 3 ++-\n> t/t5552-skipping-fetch-negotiator.sh | 1 +\n> t/t5601-clone.sh | 1 +\n> t/t5616-partial-clone.sh | 3 ++-\n> t/t5700-protocol-v1.sh | 1 +\n> t/t5702-protocol-v2.sh | 1 +\n> t/t7406-submodule-update.sh | 3 ++-\n> 16 files changed, 43 insertions(+), 15 deletions(-)\n"},{"id":"365144","messageId":"cover.1544573604.git.jonathantanmy@google.com","threadId":"50006","inReplyTo":"20181211212135.21126-2-avarab@gmail.com","subject":"[PATCH 0/3] Some fixes and improvements","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-12-12T00:27:55Z","receivedAt":"2018-12-12T00:28:06Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Thanks, Aevar for looking into this. I haven't looked into detail, but:\n\n - s/where/var/ in your patch 1\n - I think that variables should be unset with env --unset instead of\n sane_unset, because (as far as I can tell) the effects of sane_unset\n \"leak\" from test block to test block.\n - On my computer, some tests still fail. I have included a patch (patch\n 1) that makes these tests succeed.\n - I noticed that some of the tests fail because fetch-pack doesn't\n support protocol v2. It turns out that adding support for protocol v2\n in fetch-pack wasn't too difficult, so I have done it. This is patch\n 2.\n - With that support added, some of the other tests no longer need the\n unsetting of the environment variable. This is patch 3.\n\nIf you agree with the general direction I'm going in, when you send out\nanother version, I would add patch 2 somewhere near the beginning of the\nset, and then squash both patch 1 and patch 3 in the G_T_P_V=2 patch. \n\nJonathan Tan (3):\n squash this into your patch\n builtin/fetch-pack: support protocol version 2\n also squash this into your patch\n\n builtin/fetch-pack.c | 9 ++++++---\n t/t5500-fetch-pack.sh | 13 +++++++------\n t/t5539-fetch-http-shallow.sh | 12 ++++++++----\n t/t5541-http-push-smart.sh | 10 ++++++++--\n t/t5551-http-fetch-smart.sh | 23 +++++++++++++++--------\n t/t5570-git-daemon.sh | 2 +-\n t/t5616-partial-clone.sh | 3 +--\n 7 files changed, 46 insertions(+), 26 deletions(-)\n\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"365145","messageId":"33bae6445998e4928aa45c15726667590947c422.1544573604.git.jonathantanmy@google.com","threadId":"50006","inReplyTo":"cover.1544573604.git.jonathantanmy@google.com","subject":"[PATCH 1/3] squash this into your patch","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-12-12T00:27:56Z","receivedAt":"2018-12-12T00:28:10Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Signed-off-by: Jonathan Tan \n---\n t/t5539-fetch-http-shallow.sh | 12 ++++++++----\n t/t5541-http-push-smart.sh | 10 ++++++++--\n t/t5551-http-fetch-smart.sh | 23 +++++++++++++++--------\n t/t5570-git-daemon.sh | 2 +-\n 4 files changed, 32 insertions(+), 15 deletions(-)\n\ndiff --git a/t/t5539-fetch-http-shallow.sh b/t/t5539-fetch-http-shallow.sh\nindex 5fbf67c446..996ce060cd 100755\n--- a/t/t5539-fetch-http-shallow.sh\n+++ b/t/t5539-fetch-http-shallow.sh\n@@ -67,7 +67,8 @@ test_expect_success 'no shallow lines after receiving ACK ready' '\n \t\tcd clone &&\n \t\tgit checkout --orphan newnew &&\n \t\ttest_commit new-too &&\n-\t\tGIT_TRACE_PACKET=\"$TRASH_DIRECTORY/trace\" git fetch --depth=2 &&\n+\t\tGIT_TRACE_PACKET=\"$TRASH_DIRECTORY/trace\" env --unset=GIT_TEST_PROTOCOL_VERSION \\\n+\t\t\tgit fetch --depth=2 &&\n \t\tgrep \"fetch-pack< ACK .* ready\" ../trace &&\n \t\t! grep \"fetch-pack> done\" ../trace\n \t)\n@@ -114,7 +115,8 @@ test_expect_success 'shallow clone exclude tag two' '\n '\n \n test_expect_success 'fetch exclude tag one' '\n-\tgit -C shallow12 fetch --shallow-exclude one origin &&\n+\tenv --unset=GIT_TEST_PROTOCOL_VERSION \\\n+\t\tgit -C shallow12 fetch --shallow-exclude one origin &&\n \tgit -C shallow12 log --pretty=tformat:%s origin/master >actual &&\n \ttest_write_lines three two >expected &&\n \ttest_cmp expected actual\n@@ -128,14 +130,16 @@ test_expect_success 'fetching deepen' '\n \ttest_commit two &&\n \ttest_commit three &&\n \tmv .git \"$HTTPD_DOCUMENT_ROOT_PATH/shallow-deepen.git\" &&\n-\tgit clone --depth 1 $HTTPD_URL/smart/shallow-deepen.git deepen &&\n+\tenv --unset=GIT_TEST_PROTOCOL_VERSION \\\n+\t\tgit clone --depth 1 $HTTPD_URL/smart/shallow-deepen.git deepen &&\n \tmv \"$HTTPD_DOCUMENT_ROOT_PATH/shallow-deepen.git\" .git &&\n \ttest_commit four &&\n \tgit -C deepen log --pretty=tformat:%s master >actual &&\n \techo three >expected &&\n \ttest_cmp expected actual &&\n \tmv .git \"$HTTPD_DOCUMENT_ROOT_PATH/shallow-deepen.git\" &&\n-\tgit -C deepen fetch --deepen=1 &&\n+\tenv --unset=GIT_TEST_PROTOCOL_VERSION \\\n+\t\tgit -C deepen fetch --deepen=1 &&\n \tgit -C deepen log --pretty=tformat:%s origin/master >actual &&\n \tcat >expected <<-\\EOF &&\n \tfour\ndiff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh\nindex 5475afc052..20f1308578 100755\n--- a/t/t5541-http-push-smart.sh\n+++ b/t/t5541-http-push-smart.sh\n@@ -45,14 +45,20 @@ test_expect_success 'no empty path components' '\n \t# In the URL, add a trailing slash, and see if git appends yet another\n \t# slash.\n \tcd \"$ROOT_PATH\" &&\n-\tgit clone $HTTPD_URL/smart/test_repo.git/ test_repo_clone &&\n+\t# Other protocol versions may make different requests, so perform this\n+\t# clone with protocol v0.\n+\tenv --unset=GIT_TEST_PROTOCOL_VERSION \\\n+\t\tgit clone $HTTPD_URL/smart/test_repo.git/ test_repo_clone &&\n \n \tcheck_access_log exp\n '\n \n test_expect_success 'clone remote repository' '\n \trm -rf test_repo_clone &&\n-\tgit clone $HTTPD_URL/smart/test_repo.git test_repo_clone &&\n+\t# Other protocol versions may make different requests, so perform this\n+\t# clone with protocol v0.\n+\tenv --unset=GIT_TEST_PROTOCOL_VERSION \\\n+\t\tgit clone $HTTPD_URL/smart/test_repo.git test_repo_clone &&\n \t(\n \t\tcd test_repo_clone && git config push.default matching\n \t)\ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex 8630b0cc39..56fd9351a6 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -43,7 +43,8 @@ test_expect_success 'clone http repository' '\n \t< Cache-Control: no-cache, max-age=0, must-revalidate\n \t< Content-Type: application/x-git-upload-pack-result\n \tEOF\n-\tGIT_TRACE_CURL=true git clone --quiet $HTTPD_URL/smart/repo.git clone 2>err &&\n+\tGIT_TRACE_CURL=true env --unset=GIT_TEST_PROTOCOL_VERSION \\\n+\t\tgit clone --quiet $HTTPD_URL/smart/repo.git clone 2>err &&\n \ttest_cmp file clone/file &&\n \ttr '\\''\\015'\\'' Q >file &&\n \tgit commit -a -m two &&\n \tgit push public &&\n-\t(cd clone && git pull) &&\n+\t(cd clone && env --unset=GIT_TEST_PROTOCOL_VERSION git pull) &&\n \ttest_cmp file clone/file\n '\n \n@@ -143,7 +144,8 @@ test_expect_success 'clone from auth-only-for-push repository' '\n test_expect_success 'clone from auth-only-for-objects repository' '\n \techo two >expect &&\n \tset_askpass user@host pass@host &&\n-\tgit clone --bare \"$HTTPD_URL/auth-fetch/smart/repo.git\" half-auth &&\n+\tenv --unset=GIT_TEST_PROTOCOL_VERSION \\\n+\t\tgit clone --bare \"$HTTPD_URL/auth-fetch/smart/repo.git\" half-auth &&\n \texpect_askpass both user@host &&\n \tgit --git-dir=half-auth log -1 --format=%s >actual &&\n \ttest_cmp expect actual\n@@ -151,7 +153,8 @@ test_expect_success 'clone from auth-only-for-objects repository' '\n \n test_expect_success 'no-op half-auth fetch does not require a password' '\n \tset_askpass wrong &&\n-\tgit --git-dir=half-auth fetch &&\n+\tenv --unset=GIT_TEST_PROTOCOL_VERSION \\\n+\t\tgit --git-dir=half-auth fetch &&\n \texpect_askpass none\n '\n \n@@ -187,7 +190,8 @@ test_expect_success 'create namespaced refs' '\n '\n \n test_expect_success 'smart clone respects namespace' '\n-\tgit clone \"$HTTPD_URL/smart_namespace/repo.git\" ns-smart &&\n+\tenv --unset=GIT_TEST_PROTOCOL_VERSION \\\n+\t\tgit clone \"$HTTPD_URL/smart_namespace/repo.git\" ns-smart &&\n \techo namespaced >expect &&\n \tgit --git-dir=ns-smart/.git log -1 --format=%s >actual &&\n \ttest_cmp expect actual\n@@ -214,7 +218,8 @@ test_expect_success 'cookies stored in http.cookiefile when http.savecookies set\n \tEOF\n \tgit config http.cookiefile cookies.txt &&\n \tgit config http.savecookies true &&\n-\tgit ls-remote $HTTPD_URL/smart_cookies/repo.git master &&\n+\tenv --unset=GIT_TEST_PROTOCOL_VERSION \\\n+\t\tgit ls-remote $HTTPD_URL/smart_cookies/repo.git master &&\n \ttail -3 cookies.txt | sort >cookies_tail.txt &&\n \ttest_cmp expect_cookies.txt cookies_tail.txt\n '\n@@ -306,7 +311,8 @@ test_expect_success 'test allowreachablesha1inwant with unreachable' '\n \n \tgit init --bare test_reachable.git &&\n \tgit -C test_reachable.git remote add origin \"$HTTPD_URL/smart/repo.git\" &&\n-\ttest_must_fail git -C test_reachable.git fetch origin \"$(git rev-parse HEAD)\"\n+\ttest_must_fail env --unset=GIT_TEST_PROTOCOL_VERSION \\\n+\t\tgit -C test_reachable.git fetch origin \"$(git rev-parse HEAD)\"\n '\n \n test_expect_success 'test allowanysha1inwant with unreachable' '\n@@ -325,7 +331,8 @@ test_expect_success 'test allowanysha1inwant with unreachable' '\n \n \tgit init --bare test_reachable.git &&\n \tgit -C test_reachable.git remote add origin \"$HTTPD_URL/smart/repo.git\" &&\n-\ttest_must_fail git -C test_reachable.git fetch origin \"$(git rev-parse HEAD)\" &&\n+\ttest_must_fail env --unset=GIT_TEST_PROTOCOL_VERSION \\\n+\t\tgit -C test_reachable.git fetch origin \"$(git rev-parse HEAD)\" &&\n \n \tgit -C \"$server\" config uploadpack.allowanysha1inwant 1 &&\n \tgit -C test_reachable.git fetch origin \"$(git rev-parse HEAD)\"\ndiff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh\nindex 7466aad111..4a2a937bb0 100755\n--- a/t/t5570-git-daemon.sh\n+++ b/t/t5570-git-daemon.sh\n@@ -190,7 +190,7 @@ test_expect_success 'daemon log records all attributes' '\n \tEOF\n \t>daemon.log &&\n \tGIT_OVERRIDE_VIRTUAL_HOST=localhost \\\n-\t\tgit -c protocol.version=1 \\\n+\t\tenv --unset=GIT_TEST_PROTOCOL_VERSION git -c protocol.version=1 \\\n \t\t\tls-remote \"$GIT_DAEMON_URL/interp.git\" &&\n \tgrep -i extended.attribute daemon.log | cut -d\" \" -f2- >actual &&\n \ttest_cmp expect actual\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"365146","messageId":"378905b2ce5790cb406b950a3be530859368b1fe.1544573604.git.jonathantanmy@google.com","threadId":"50006","inReplyTo":"cover.1544573604.git.jonathantanmy@google.com","subject":"[PATCH 3/3] also squash this into your patch","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-12-12T00:27:58Z","receivedAt":"2018-12-12T00:28:13Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Signed-off-by: Jonathan Tan \n---\n t/t5500-fetch-pack.sh | 13 +++++++------\n t/t5616-partial-clone.sh | 3 +--\n 2 files changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex 9c18875c9c..a5a8f348a2 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -41,8 +41,7 @@ pull_to_client () {\n \ttest_expect_success \"$number pull\" '\n \t\t(\n \t\t\tcd client &&\n-\t\t\tGIT_TEST_PROTOCOL_VERSION=0 \\\n-\t\t\t\tgit fetch-pack -k -v .. $heads &&\n+\t\t\tgit fetch-pack -k -v .. $heads &&\n \n \t\t\tcase \"$heads\" in\n \t\t\t *A*)\n@@ -441,7 +440,6 @@ test_expect_success 'setup tests for the --stdin parameter' '\n '\n \n test_expect_success 'fetch refs from cmdline' '\n-\tsane_unset GIT_TEST_PROTOCOL_VERSION &&\n \t(\n \t\tcd client &&\n \t\tgit fetch-pack --no-progress .. $(cat ../input)\n@@ -630,7 +628,10 @@ test_expect_success 'fetch-pack cannot fetch a raw sha1 that is not advertised a\n \ttest_commit -C server 6 &&\n \n \tgit init client &&\n-\ttest_must_fail git -C client fetch-pack ../server \\\n+\n+\t# Other protocol versions (e.g. 2) allow fetching an unadvertised\n+\t# object, so run this test with the default protocol version (0).\n+\ttest_must_fail env --unset=GIT_TEST_PROTOCOL_VERSION git -C client fetch-pack ../server \\\n \t\t$(git -C server rev-parse refs/heads/master^) 2>err &&\n \ttest_i18ngrep \"Server does not allow request for unadvertised object\" err\n '\n@@ -790,7 +791,7 @@ test_expect_success 'shallow clone exclude tag two' '\n '\n \n test_expect_success 'fetch exclude tag one' '\n-\tgit -C shallow12 fetch --shallow-exclude one origin &&\n+\tenv --unset=GIT_TEST_PROTOCOL_VERSION git -C shallow12 fetch --shallow-exclude one origin &&\n \tgit -C shallow12 log --pretty=tformat:%s origin/master >actual &&\n \ttest_write_lines three two >expected &&\n \ttest_cmp expected actual\n@@ -808,7 +809,7 @@ test_expect_success 'fetching deepen' '\n \tgit -C deepen log --pretty=tformat:%s master >actual &&\n \techo three >expected &&\n \ttest_cmp expected actual &&\n-\tgit -C deepen fetch --deepen=1 &&\n+\tenv --unset=GIT_TEST_PROTOCOL_VERSION git -C deepen fetch --deepen=1 &&\n \tgit -C deepen log --pretty=tformat:%s origin/master >actual &&\n \tcat >expected <<-\\EOF &&\n \tfour\ndiff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\nindex feedf84ce1..336f02a41a 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -144,8 +144,7 @@ test_expect_success 'manual prefetch of missing objects' '\n \tsort >observed.oids &&\n \n \ttest_line_count = 6 observed.oids &&\n-\tGIT_TEST_PROTOCOL_VERSION=0 \\\n-\t\tgit -C pc1 fetch-pack --stdin \"file://$(pwd)/srv.bare\" revs &&\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"365147","messageId":"a09a0ac1218930eab01ed6ae92a20d84ab12a03e.1544573604.git.jonathantanmy@google.com","threadId":"50006","inReplyTo":"cover.1544573604.git.jonathantanmy@google.com","subject":"[PATCH 2/3] builtin/fetch-pack: support protocol version 2","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-12-12T00:27:57Z","receivedAt":"2018-12-12T00:28:15Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Currently, if support for running Git's entire test suite with protocol\nv2 were added, some tests would fail because the fetch-pack CLI command\ndies if it encounters protocol v2. To avoid this, teach fetch-pack\nsupport for protocol v2.\n\nSigned-off-by: Jonathan Tan \n---\n builtin/fetch-pack.c | 9 ++++++---\n 1 file changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex 63e69a5801..f6a513495e 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -55,6 +55,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \tstruct oid_array shallow = OID_ARRAY_INIT;\n \tstruct string_list deepen_not = STRING_LIST_INIT_DUP;\n \tstruct packet_reader reader;\n+\tenum protocol_version version;\n \n \tfetch_if_missing = 0;\n \n@@ -219,9 +220,11 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \t\t\t PACKET_READ_CHOMP_NEWLINE |\n \t\t\t PACKET_READ_GENTLE_ON_EOF);\n \n-\tswitch (discover_version(&reader)) {\n+\tversion = discover_version(&reader);\n+\tswitch (version) {\n \tcase protocol_v2:\n-\t\tdie(\"support for protocol v2 not implemented yet\");\n+\t\tget_remote_refs(fd[1], &reader, &ref, 0, NULL, NULL);\n+\t\tbreak;\n \tcase protocol_v1:\n \tcase protocol_v0:\n \t\tget_remote_heads(&reader, &ref, 0, NULL, &shallow);\n@@ -231,7 +234,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \t}\n \n \tref = fetch_pack(&args, fd, conn, ref, dest, sought, nr_sought,\n-\t\t\t &shallow, pack_lockfile_ptr, protocol_v0);\n+\t\t\t &shallow, pack_lockfile_ptr, version);\n \tif (pack_lockfile) {\n \t\tprintf(\"lock %s\\n\", pack_lockfile);\n \t\tfflush(stdout);\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"365216","messageId":"xmqqimzygmz6.fsf@gitster-ct.c.googlers.com","threadId":"50006","inReplyTo":"cover.1544573604.git.jonathantanmy@google.com","subject":"Re: [PATCH 0/3] Some fixes and improvements","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-13T02:49:17Z","receivedAt":"2018-12-13T02:49:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> Thanks, Aevar for looking into this. I haven't looked into detail, but:\n> ...\n> If you agree with the general direction I'm going in, when you send out\n> another version, I would add patch 2 somewhere near the beginning of the\n> set, and then squash both patch 1 and patch 3 in the G_T_P_V=2 patch. \n\nThanks, both. I'll let these sit in the list archive and expect an\nupdated series ready to be picked up.\n"},{"id":"365260","messageId":"20181213155817.27666-1-avarab@gmail.com","threadId":"50006","inReplyTo":"xmqqimzygmz6.fsf@gitster-ct.c.googlers.com","subject":"[PATCH v2 0/8] protocol v2 fixes","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-12-13T15:58:09Z","receivedAt":"2018-12-13T15:58:31Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"I figured it would be easier for everyone if I rolled this all into\none series instead of Junio & us needing to keep track of what's based\non what.\n\nThe only change I made to Jeff's patches is my SOB and adding a\nparagraph to the end of his 3/3 saying that the v2 push protocol\ndoesn't have the same issue (because it doesn't exist yet). I had that\nquestion in this thread, and thought it was useful to clarify it.\n\nNo changes to Jonathan's one patch, except my SOB.\n\nFor the rest I incorporated Jonathan's suggestions / fixes with some\namendments. The suggestion to use env --unset isn't portable (and\nthere's now a check for that while we're at it), so instead we support\n\"GIT_TEST_PROTOCOL_VERSION=\" which'll ignore the environment value.\n\nOther changes in my patches are more narrowly skipping tests, i.e. no\n\"unset\" anymore except for those tests where we're only doing v1 and\nv2 tests. I also removed the \"env\" use in those cases that don't need\nit (where we use e.g. test_must_fail), instead we just set the env\nvariable ourselves with native shell syntax.\n\nJeff King (3):\n serve: pass \"config context\" through to individual commands\n parse_hide_refs_config: handle NULL section\n upload-pack: support hidden refs with protocol v2\n\nJonathan Tan (1):\n builtin/fetch-pack: support protocol version 2\n\nÆvar Arnfjörð Bjarmason (4):\n tests: add a check for unportable env --unset\n tests: add a special setup where for protocol.version\n tests: mark & fix tests broken under GIT_TEST_PROTOCOL_VERSION=1\n tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2\n\n builtin/fetch-pack.c | 9 ++++++---\n builtin/upload-pack.c | 1 +\n ls-refs.c | 16 +++++++++++++++-\n ls-refs.h | 3 ++-\n protocol.c | 13 ++++++++++++-\n refs.c | 3 ++-\n serve.c | 9 +++++----\n serve.h | 7 +++++++\n t/README | 6 ++++++\n t/check-non-portable-shell.pl | 1 +\n t/t0410-partial-clone.sh | 3 ++-\n t/t5400-send-pack.sh | 2 +-\n t/t5500-fetch-pack.sh | 9 ++++++---\n t/t5503-tagfollow.sh | 8 ++++----\n t/t5512-ls-remote.sh | 14 ++++++++++----\n t/t5515-fetch-merge-logic.sh | 2 +-\n t/t5516-fetch-push.sh | 20 +++++++++++++-------\n t/t5537-fetch-shallow.sh | 3 ++-\n t/t5539-fetch-http-shallow.sh | 9 +++++----\n t/t5541-http-push-smart.sh | 9 +++++++--\n t/t5551-http-fetch-smart.sh | 19 +++++++++++--------\n t/t5552-skipping-fetch-negotiator.sh | 4 ++--\n t/t5570-git-daemon.sh | 2 +-\n t/t5601-clone.sh | 11 +++++++++--\n t/t5700-protocol-v1.sh | 1 +\n t/t5702-protocol-v2.sh | 1 +\n t/t7406-submodule-update.sh | 3 ++-\n upload-pack.c | 4 ++--\n upload-pack.h | 4 ++--\n 29 files changed, 139 insertions(+), 57 deletions(-)\n\n-- \n2.20.0.405.gbc1bbc6f85\n\n"},{"id":"365261","messageId":"20181213155817.27666-2-avarab@gmail.com","threadId":"50006","inReplyTo":"xmqqimzygmz6.fsf@gitster-ct.c.googlers.com","subject":"[PATCH v2 1/8] serve: pass \"config context\" through to individual commands","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-12-13T15:58:10Z","receivedAt":"2018-12-13T15:58:32Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"From: Jeff King \n\nIn protocol v2, instead of just running \"upload-pack\", we have a generic\n\"serve\" loop which runs command requests from the client. What used to\nbe \"upload-pack\" is now generally split into two operations: \"ls-refs\"\nand \"fetch\". The latter knows it must respect uploadpack.* config, but\nthe former is not actually specific to a fetch operation (we do not yet\ndo v2 receive-pack, but eventually we may, and ls-refs would support\nboth operations).\n\nHowever, ls-refs does need to know which operation we're performing, in\norder to read the correct config (e.g., uploadpack.hiderefs versus\nreceive.hiderefs; we don't read _either_ right now, but this is the\nfirst step to fixing that).\n\nIn the generic \"git-serve\" program, we don't have that information. But\nin the protocol as it is actually used by clients, the client still asks\nto run \"git-upload-pack\", and then issues an \"ls-refs\" from there. So we\n_do_ know at that point that \"uploadpack\" is the right config context.\nThis patch teaches the protocol v2 \"serve\" code to pass that context\nthrough to the individual commands (a future patch will teach ls-refs to\nactually use it).\n\nSigned-off-by: Jeff King \nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/upload-pack.c | 1 +\n ls-refs.c | 4 +++-\n ls-refs.h | 3 ++-\n serve.c | 9 +++++----\n serve.h | 7 +++++++\n upload-pack.c | 4 ++--\n upload-pack.h | 4 ++--\n 7 files changed, 22 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/upload-pack.c b/builtin/upload-pack.c\nindex 42dc4da5a1..67192cfa9e 100644\n--- a/builtin/upload-pack.c\n+++ b/builtin/upload-pack.c\n@@ -52,6 +52,7 @@ int cmd_upload_pack(int argc, const char **argv, const char *prefix)\n \tcase protocol_v2:\n \t\tserve_opts.advertise_capabilities = opts.advertise_refs;\n \t\tserve_opts.stateless_rpc = opts.stateless_rpc;\n+\t\tserve_opts.config_context = \"uploadpack\";\n \t\tserve(&serve_opts);\n \t\tbreak;\n \tcase protocol_v1:\ndiff --git a/ls-refs.c b/ls-refs.c\nindex a06f12eca8..e8e31475f0 100644\n--- a/ls-refs.c\n+++ b/ls-refs.c\n@@ -69,7 +69,9 @@ static int send_ref(const char *refname, const struct object_id *oid,\n \treturn 0;\n }\n \n-int ls_refs(struct repository *r, struct argv_array *keys,\n+int ls_refs(struct repository *r,\n+\t const char *config_section,\n+\t struct argv_array *keys,\n \t struct packet_reader *request)\n {\n \tstruct ls_refs_data data;\ndiff --git a/ls-refs.h b/ls-refs.h\nindex b62877e8da..da26fc9824 100644\n--- a/ls-refs.h\n+++ b/ls-refs.h\n@@ -4,7 +4,8 @@\n struct repository;\n struct argv_array;\n struct packet_reader;\n-extern int ls_refs(struct repository *r, struct argv_array *keys,\n+extern int ls_refs(struct repository *r, const char *config_context,\n+\t\t struct argv_array *keys,\n \t\t struct packet_reader *request);\n \n #endif /* LS_REFS_H */\ndiff --git a/serve.c b/serve.c\nindex bda085f09c..70f89cf0d9 100644\n--- a/serve.c\n+++ b/serve.c\n@@ -48,6 +48,7 @@ struct protocol_capability {\n \t * This field should be NULL for capabilities which are not commands.\n \t */\n \tint (*command)(struct repository *r,\n+\t\t const char *config_context,\n \t\t struct argv_array *keys,\n \t\t struct packet_reader *request);\n };\n@@ -158,7 +159,7 @@ enum request_state {\n \tPROCESS_REQUEST_DONE,\n };\n \n-static int process_request(void)\n+static int process_request(struct serve_options *opts)\n {\n \tenum request_state state = PROCESS_REQUEST_KEYS;\n \tstruct packet_reader reader;\n@@ -222,7 +223,7 @@ static int process_request(void)\n \tif (!command)\n \t\tdie(\"no command requested\");\n \n-\tcommand->command(the_repository, &keys, &reader);\n+\tcommand->command(the_repository, opts->config_context, &keys, &reader);\n \n \targv_array_clear(&keys);\n \treturn 0;\n@@ -249,10 +250,10 @@ void serve(struct serve_options *options)\n \t * a single request/response exchange\n \t */\n \tif (options->stateless_rpc) {\n-\t\tprocess_request();\n+\t\tprocess_request(options);\n \t} else {\n \t\tfor (;;)\n-\t\t\tif (process_request())\n+\t\t\tif (process_request(options))\n \t\t\t\tbreak;\n \t}\n }\ndiff --git a/serve.h b/serve.h\nindex fe65ba9f46..d527224cbb 100644\n--- a/serve.h\n+++ b/serve.h\n@@ -8,6 +8,13 @@ extern int has_capability(const struct argv_array *keys, const char *capability,\n struct serve_options {\n \tunsigned advertise_capabilities;\n \tunsigned stateless_rpc;\n+\n+\t/*\n+\t * Some operations may need to know the context when looking up config;\n+\t * e.g., set this to \"uploadpack\" to respect \"uploadpack.hiderefs\" (as\n+\t * opposed to \"receive.hiderefs\").\n+\t */\n+\tconst char *config_context;\n };\n #define SERVE_OPTIONS_INIT { 0 }\n extern void serve(struct serve_options *options);\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 5e81f1ff24..914bbb40bc 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -1413,8 +1413,8 @@ enum fetch_state {\n \tFETCH_DONE,\n };\n \n-int upload_pack_v2(struct repository *r, struct argv_array *keys,\n-\t\t struct packet_reader *request)\n+int upload_pack_v2(struct repository *r, const char *config_context,\n+\t\t struct argv_array *keys, struct packet_reader *request)\n {\n \tenum fetch_state state = FETCH_PROCESS_ARGS;\n \tstruct upload_pack_data data;\ndiff --git a/upload-pack.h b/upload-pack.h\nindex cab2178796..2a9f51197c 100644\n--- a/upload-pack.h\n+++ b/upload-pack.h\n@@ -13,8 +13,8 @@ void upload_pack(struct upload_pack_options *options);\n struct repository;\n struct argv_array;\n struct packet_reader;\n-extern int upload_pack_v2(struct repository *r, struct argv_array *keys,\n-\t\t\t struct packet_reader *request);\n+extern int upload_pack_v2(struct repository *r, const char *config_context,\n+\t\t\t struct argv_array *keys, struct packet_reader *request);\n \n struct strbuf;\n extern int upload_pack_advertise(struct repository *r,\n-- \n2.20.0.405.gbc1bbc6f85\n\n"},{"id":"365262","messageId":"20181213155817.27666-3-avarab@gmail.com","threadId":"50006","inReplyTo":"xmqqimzygmz6.fsf@gitster-ct.c.googlers.com","subject":"[PATCH v2 2/8] parse_hide_refs_config: handle NULL section","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-12-13T15:58:11Z","receivedAt":"2018-12-13T15:58:34Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"From: Jeff King \n\nThis helper function looks for config in two places: transfer.hiderefs,\nor $section.hiderefs, where $section is passed in by the caller (and is\n\"uploadpack\" or \"receive\", depending on the context).\n\nIn preparation for callers which do not even have that context (namely\nthe \"git-serve\" command), let's handle a NULL by looking only at\ntransfer.hiderefs (as opposed to segfaulting).\n\nSigned-off-by: Jeff King \nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n refs.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/refs.c b/refs.c\nindex f9936355cd..099e91d9cc 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1267,7 +1267,8 @@ int parse_hide_refs_config(const char *var, const char *value, const char *secti\n {\n \tconst char *key;\n \tif (!strcmp(\"transfer.hiderefs\", var) ||\n-\t (!parse_config_key(var, section, NULL, NULL, &key) &&\n+\t (section &&\n+\t !parse_config_key(var, section, NULL, NULL, &key) &&\n \t !strcmp(key, \"hiderefs\"))) {\n \t\tchar *ref;\n \t\tint len;\n-- \n2.20.0.405.gbc1bbc6f85\n\n"},{"id":"365263","messageId":"20181213155817.27666-4-avarab@gmail.com","threadId":"50006","inReplyTo":"xmqqimzygmz6.fsf@gitster-ct.c.googlers.com","subject":"[PATCH v2 3/8] upload-pack: support hidden refs with protocol v2","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-12-13T15:58:12Z","receivedAt":"2018-12-13T15:58:36Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"From: Jeff King \n\nIn the v2 protocol, upload-pack's advertisement has been moved to the\n\"ls-refs\" command. That command does not respect hidden-ref config (like\ntransfer.hiderefs) at all, and advertises everything.\n\nWhile there are some features that are not supported in v2 (e.g., v2\nalways allows fetching any sha1 without respect to advertisements), the\nlack of this feature is not documented and is likely just a bug. Let's\nmake it work, as otherwise upgrading a server to a v2-capable git will\nstart exposing these refs that the repository admin has asked to remain\nhidden.\n\nNote that we depend on the config_context from the caller here to\nrealize that we should respect uploadpack.hiderefs. When run from the\n\"git-serve\" tool, we won't have that context and will only respect\ntransfer.hiderefs. This should be OK, as git-serve is not actually used\nfor normal protocol operations.\n\nNote also that we don't need to worry about the \"An attempt to update\nor delete a hidden ref by git push is rejected\" feature of\nreceive.hideRefs, see git-config(1). This is because there's no v2\npush protocol yet.\n\nSigned-off-by: Jeff King \nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n ls-refs.c | 12 ++++++++++++\n t/t5512-ls-remote.sh | 6 ++++++\n 2 files changed, 18 insertions(+)\n\ndiff --git a/ls-refs.c b/ls-refs.c\nindex e8e31475f0..8a8143338b 100644\n--- a/ls-refs.c\n+++ b/ls-refs.c\n@@ -5,6 +5,7 @@\n #include \"argv-array.h\"\n #include \"ls-refs.h\"\n #include \"pkt-line.h\"\n+#include \"config.h\"\n \n /*\n * Check if one of the prefixes is a prefix of the ref.\n@@ -40,6 +41,9 @@ static int send_ref(const char *refname, const struct object_id *oid,\n \tconst char *refname_nons = strip_namespace(refname);\n \tstruct strbuf refline = STRBUF_INIT;\n \n+\tif (ref_is_hidden(refname_nons, refname))\n+\t\treturn 0;\n+\n \tif (!ref_match(&data->prefixes, refname))\n \t\treturn 0;\n \n@@ -69,6 +73,12 @@ static int send_ref(const char *refname, const struct object_id *oid,\n \treturn 0;\n }\n \n+static int ls_refs_config(const char *var, const char *value,\n+\t\t\t void *config_context)\n+{\n+\treturn parse_hide_refs_config(var, value, config_context);\n+}\n+\n int ls_refs(struct repository *r,\n \t const char *config_section,\n \t struct argv_array *keys,\n@@ -78,6 +88,8 @@ int ls_refs(struct repository *r,\n \n \tmemset(&data, 0, sizeof(data));\n \n+\tgit_config(ls_refs_config, (void *)config_section);\n+\n \twhile (packet_reader_read(request) != PACKET_READ_FLUSH) {\n \t\tconst char *arg = request->line;\n \t\tconst char *out;\ndiff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh\nindex 32e722db2e..ca69636fd5 100755\n--- a/t/t5512-ls-remote.sh\n+++ b/t/t5512-ls-remote.sh\n@@ -204,6 +204,12 @@ test_expect_success 'overrides work between mixed transfer/upload-pack hideRefs'\n \tgrep refs/tags/magic actual\n '\n \n+test_expect_success 'protocol v2 supports hiderefs' '\n+\ttest_config uploadpack.hiderefs refs/tags &&\n+\tgit -c protocol.version=2 ls-remote . >actual &&\n+\t! grep refs/tags actual\n+'\n+\n test_expect_success 'ls-remote --symref' '\n \tgit fetch origin &&\n \tcat >expect <<-EOF &&\n-- \n2.20.0.405.gbc1bbc6f85\n\n"},{"id":"365264","messageId":"20181213155817.27666-5-avarab@gmail.com","threadId":"50006","inReplyTo":"xmqqimzygmz6.fsf@gitster-ct.c.googlers.com","subject":"[PATCH v2 4/8] tests: add a check for unportable env --unset","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-12-13T15:58:13Z","receivedAt":"2018-12-13T15:58:38Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The \"env --unset=*\" argument isn't portable. Neither Solaris or AIX\nhave it, and probably not a bunch of other POSIX-like OS's.\n\nUsing this was suggested on-list[1]. Let's add a check for it so it\ndoesn't sneak into the codebase in the future.\n\n1. https://public-inbox.org/git/cover.1544573604.git.jonathantanmy@google.com/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/check-non-portable-shell.pl | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl\nindex b45bdac688..1cfb4608ee 100755\n--- a/t/check-non-portable-shell.pl\n+++ b/t/check-non-portable-shell.pl\n@@ -35,6 +35,7 @@ sub err {\n \t\tchomp;\n \t}\n \n+\t/\\benv (?:-u|--unset)\\b/ and err 'env [-u|--unset] is not portable';\n \t/\\bsed\\s+-i/ and err 'sed -i is not portable';\n \t/\\becho\\s+-[neE]/ and err 'echo with option is not portable (use printf)';\n \t/^\\s*declare\\s+/ and err 'arrays/declare not portable';\n-- \n2.20.0.405.gbc1bbc6f85\n\n"},{"id":"365265","messageId":"20181213155817.27666-6-avarab@gmail.com","threadId":"50006","inReplyTo":"xmqqimzygmz6.fsf@gitster-ct.c.googlers.com","subject":"[PATCH v2 5/8] tests: add a special setup where for protocol.version","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-12-13T15:58:14Z","receivedAt":"2018-12-13T15:58:40Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a GIT_TEST_PROTOCOL_VERSION=X test mode which is equivalent to\nrunning with protocol.version=X. This is needed to spot regressions\nand differences such as \"ls-refs\" behaving differently with\ntransfer.hideRefs. See\nhttps://public-inbox.org/git/20181211104236.GA6899@sigill.intra.peff.net/\nfor a fix for that regression.\n\nWith this all tests pass with GIT_TEST_PROTOCOL_VERSION=0, but fail\nwith GIT_TEST_PROTOCOL_VERSION=[1|2]. That's OK since this is a new\ntest mode, subsequent patches will fix up these test failures.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n protocol.c | 13 ++++++++++++-\n t/README | 6 ++++++\n t/t0410-partial-clone.sh | 3 ++-\n t/t5516-fetch-push.sh | 3 ++-\n t/t5700-protocol-v1.sh | 1 +\n t/t5702-protocol-v2.sh | 1 +\n 6 files changed, 24 insertions(+), 3 deletions(-)\n\ndiff --git a/protocol.c b/protocol.c\nindex 5e636785d1..aa06c3b5bc 100644\n--- a/protocol.c\n+++ b/protocol.c\n@@ -17,7 +17,18 @@ static enum protocol_version parse_protocol_version(const char *value)\n enum protocol_version get_protocol_version_config(void)\n {\n \tconst char *value;\n-\tif (!git_config_get_string_const(\"protocol.version\", &value)) {\n+\tconst char *git_test_k = \"GIT_TEST_PROTOCOL_VERSION\";\n+\tconst char *git_test_v = getenv(git_test_k);\n+\n+\tif (git_test_v && strlen(git_test_v)) {\n+\t\tenum protocol_version version = parse_protocol_version(git_test_v);\n+\n+\t\tif (version == protocol_unknown_version)\n+\t\t\tdie(\"unknown value for %s: %s\", git_test_k,\n+\t\t\t git_test_v);\n+\n+\t\treturn version;\n+\t} else if (!git_config_get_string_const(\"protocol.version\", &value)) {\n \t\tenum protocol_version version = parse_protocol_version(value);\n \n \t\tif (version == protocol_unknown_version)\ndiff --git a/t/README b/t/README\nindex 28711cc508..89629c5818 100644\n--- a/t/README\n+++ b/t/README\n@@ -311,6 +311,12 @@ marked strings\" in po/README for details.\n GIT_TEST_SPLIT_INDEX= forces split-index mode on the whole\n test suite. Accept any boolean values that are accepted by git-config.\n \n+GIT_TEST_PROTOCOL_VERSION=<'protocol.version' config value>, when set,\n+runs the test suite with the given protocol.version. E.g. \"0\", \"1\" or\n+\"2\". Can be set to the empty string within tests themselves (e.g. \"env\n+GIT_TEST_PROTOCOL_VERSION= \") to unset the value in the\n+environment as a workaround for \"env --unset\" not being portable.\n+\n GIT_TEST_FULL_IN_PACK_ARRAY= exercises the uncommon\n pack-objects code path where there are more than 1024 packs even if\n the actual number of packs in repository is below this limit. Accept\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex ba3887f178..8ba3d9b5ab 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -178,7 +178,8 @@ test_expect_success 'fetching of missing objects works with ref-in-want enabled'\n \n \trm -rf repo/.git/objects/* &&\n \trm -f trace &&\n-\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C repo cat-file -p \"$HASH\" &&\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" env GIT_TEST_PROTOCOL_VERSION= \\\n+\t\tgit -C repo cat-file -p \"$HASH\" &&\n \tgrep \"git< fetch=.*ref-in-want\" trace\n '\n \ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 37e8e80893..08cdee0b95 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -1187,7 +1187,8 @@ test_expect_success 'fetch exact SHA1 in protocol v2' '\n \n \t# fetching the hidden object succeeds by default\n \t# NEEDSWORK: should this match the v0 behavior instead?\n-\tgit -C child fetch -v ../testrepo $the_commit:refs/heads/copy\n+\tenv GIT_TEST_PROTOCOL_VERSION= \\\n+\t\tgit -C child fetch -v ../testrepo $the_commit:refs/heads/copy\n '\n \n for configallowtipsha1inwant in true false\ndiff --git a/t/t5700-protocol-v1.sh b/t/t5700-protocol-v1.sh\nindex ba86a44eb1..e4d375c462 100755\n--- a/t/t5700-protocol-v1.sh\n+++ b/t/t5700-protocol-v1.sh\n@@ -8,6 +8,7 @@ TEST_NO_CREATE_REPO=1\n \n # Test protocol v1 with 'git://' transport\n #\n+unset GIT_TEST_PROTOCOL_VERSION\n . \"$TEST_DIRECTORY\"/lib-git-daemon.sh\n start_git_daemon --export-all --enable=receive-pack\n daemon_parent=$GIT_DAEMON_DOCUMENT_ROOT_PATH/parent\ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex 0f2b09ebb8..d1549f294e 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -8,6 +8,7 @@ TEST_NO_CREATE_REPO=1\n \n # Test protocol v2 with 'git://' transport\n #\n+unset GIT_TEST_PROTOCOL_VERSION\n . \"$TEST_DIRECTORY\"/lib-git-daemon.sh\n start_git_daemon --export-all --enable=receive-pack\n daemon_parent=$GIT_DAEMON_DOCUMENT_ROOT_PATH/parent\n-- \n2.20.0.405.gbc1bbc6f85\n\n"},{"id":"365266","messageId":"20181213155817.27666-7-avarab@gmail.com","threadId":"50006","inReplyTo":"xmqqimzygmz6.fsf@gitster-ct.c.googlers.com","subject":"[PATCH v2 6/8] tests: mark & fix tests broken under GIT_TEST_PROTOCOL_VERSION=1","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-12-13T15:58:15Z","receivedAt":"2018-12-13T15:58:43Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"There's one t5400-send-pack.sh test which is broken under\nGIT_TEST_PROTOCOL_VERSION=1. It's easier to just coerce it to run\nunder protocol 1. The difference in the output is that there'll be a\n\"version 1\" line which'll make the subsequent \"test_cmp\". See\nprotocol.version in git-config(1) which notes that \"1\" is just \"0\"\nwith a version number. the trace output will include fail.\n\nSimilarly in t5601-clone.sh we'll be passing an option to ssh, but\nsince so many tests would fail in this file let's go above & beyond\nand make them pass by only testing the relevant part of the output.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t5400-send-pack.sh | 2 +-\n t/t5601-clone.sh | 11 +++++++++--\n 2 files changed, 10 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh\nindex f1932ea431..571d620aed 100755\n--- a/t/t5400-send-pack.sh\n+++ b/t/t5400-send-pack.sh\n@@ -288,7 +288,7 @@ test_expect_success 'receive-pack de-dupes .have lines' '\n \t$shared .have\n \tEOF\n \n-\tGIT_TRACE_PACKET=$(pwd)/trace \\\n+\tGIT_TRACE_PACKET=$(pwd)/trace GIT_TEST_PROTOCOL_VERSION= \\\n \t git push \\\n \t\t--receive-pack=\"unset GIT_TRACE_PACKET; git-receive-pack\" \\\n \t\tfork HEAD:foo &&\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex 8bbc7068ac..a7b7ab327c 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -325,7 +325,7 @@ copy_ssh_wrapper_as () {\n \n expect_ssh () {\n \ttest_when_finished '\n-\t\t(cd \"$TRASH_DIRECTORY\" && rm -f ssh-expect && >ssh-output)\n+\t\t(cd \"$TRASH_DIRECTORY\" && rm -f ssh-expect ssh-output.munged && >ssh-output)\n \t' &&\n \t{\n \t\tcase \"$#\" in\n@@ -341,7 +341,14 @@ expect_ssh () {\n \t\t\techo \"ssh: $1 $2 git-upload-pack '$3' $4\"\n \t\tesac\n \t} >\"$TRASH_DIRECTORY/ssh-expect\" &&\n-\t(cd \"$TRASH_DIRECTORY\" && test_cmp ssh-expect ssh-output)\n+\t(\n+\t\tcd \"$TRASH_DIRECTORY\" &&\n+\t\t# We don't care about this trivial difference in\n+\t\t# output with GIT_TEST_PROTOCOL_VERSION=[12]\n+\t\tsed 's/ssh: -o SendEnv=GIT_PROTOCOL /ssh: /' ssh-output.munged &&\n+\t\tmv ssh-output.munged ssh-output &&\n+\t\ttest_cmp ssh-expect ssh-output\n+\t)\n }\n \n test_expect_success 'clone myhost:src uses ssh' '\n-- \n2.20.0.405.gbc1bbc6f85\n\n"},{"id":"365267","messageId":"20181213155817.27666-8-avarab@gmail.com","threadId":"50006","inReplyTo":"xmqqimzygmz6.fsf@gitster-ct.c.googlers.com","subject":"[PATCH v2 7/8] builtin/fetch-pack: support protocol version 2","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-12-13T15:58:16Z","receivedAt":"2018-12-13T15:58:44Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"From: Jonathan Tan \n\nCurrently, if support for running Git's entire test suite with protocol\nv2 were added, some tests would fail because the fetch-pack CLI command\ndies if it encounters protocol v2. To avoid this, teach fetch-pack\nsupport for protocol v2.\n\nSigned-off-by: Jonathan Tan \nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/fetch-pack.c | 9 ++++++---\n 1 file changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex 63e69a5801..f6a513495e 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -55,6 +55,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \tstruct oid_array shallow = OID_ARRAY_INIT;\n \tstruct string_list deepen_not = STRING_LIST_INIT_DUP;\n \tstruct packet_reader reader;\n+\tenum protocol_version version;\n \n \tfetch_if_missing = 0;\n \n@@ -219,9 +220,11 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \t\t\t PACKET_READ_CHOMP_NEWLINE |\n \t\t\t PACKET_READ_GENTLE_ON_EOF);\n \n-\tswitch (discover_version(&reader)) {\n+\tversion = discover_version(&reader);\n+\tswitch (version) {\n \tcase protocol_v2:\n-\t\tdie(\"support for protocol v2 not implemented yet\");\n+\t\tget_remote_refs(fd[1], &reader, &ref, 0, NULL, NULL);\n+\t\tbreak;\n \tcase protocol_v1:\n \tcase protocol_v0:\n \t\tget_remote_heads(&reader, &ref, 0, NULL, &shallow);\n@@ -231,7 +234,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \t}\n \n \tref = fetch_pack(&args, fd, conn, ref, dest, sought, nr_sought,\n-\t\t\t &shallow, pack_lockfile_ptr, protocol_v0);\n+\t\t\t &shallow, pack_lockfile_ptr, version);\n \tif (pack_lockfile) {\n \t\tprintf(\"lock %s\\n\", pack_lockfile);\n \t\tfflush(stdout);\n-- \n2.20.0.405.gbc1bbc6f85\n\n"},{"id":"365268","messageId":"20181213155817.27666-9-avarab@gmail.com","threadId":"50006","inReplyTo":"xmqqimzygmz6.fsf@gitster-ct.c.googlers.com","subject":"[PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-12-13T15:58:17Z","receivedAt":"2018-12-13T15:58:47Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Mark those tests that have behavior differences or bugs under\nprotocol.version=2.\n\nWhether or not these tests should exhibit different behavior is\noutside the scope of this change. Some (such as t5500-fetch-pack.sh)\nare intentionally different, but others (such as\nt7406-submodule-update.sh and t5515-fetch-merge-logic.sh) might\nindicate bugs in the protocol v2 code.\n\nTracking down which is which is outside the scope of this\nchange. Let's first exhaustively annotate where the differences are,\nso that we can spot future behavior differences or regressions.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \nSigned-off-by: Jonathan Tan \n---\n t/t5500-fetch-pack.sh | 9 ++++++---\n t/t5503-tagfollow.sh | 8 ++++----\n t/t5512-ls-remote.sh | 8 ++++----\n t/t5515-fetch-merge-logic.sh | 2 +-\n t/t5516-fetch-push.sh | 17 +++++++++++------\n t/t5537-fetch-shallow.sh | 3 ++-\n t/t5539-fetch-http-shallow.sh | 9 +++++----\n t/t5541-http-push-smart.sh | 9 +++++++--\n t/t5551-http-fetch-smart.sh | 19 +++++++++++--------\n t/t5552-skipping-fetch-negotiator.sh | 4 ++--\n t/t5570-git-daemon.sh | 2 +-\n t/t7406-submodule-update.sh | 3 ++-\n 12 files changed, 56 insertions(+), 37 deletions(-)\n\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex 086f2c40f6..8b1217ea26 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -628,7 +628,10 @@ test_expect_success 'fetch-pack cannot fetch a raw sha1 that is not advertised a\n \ttest_commit -C server 6 &&\n \n \tgit init client &&\n-\ttest_must_fail git -C client fetch-pack ../server \\\n+\n+\t# Other protocol versions (e.g. 2) allow fetching an unadvertised\n+\t# object, so run this test with the default protocol version (0).\n+\ttest_must_fail env GIT_TEST_PROTOCOL_VERSION= git -C client fetch-pack ../server \\\n \t\t$(git -C server rev-parse refs/heads/master^) 2>err &&\n \ttest_i18ngrep \"Server does not allow request for unadvertised object\" err\n '\n@@ -788,7 +791,7 @@ test_expect_success 'shallow clone exclude tag two' '\n '\n \n test_expect_success 'fetch exclude tag one' '\n-\tgit -C shallow12 fetch --shallow-exclude one origin &&\n+\tGIT_TEST_PROTOCOL_VERSION= git -C shallow12 fetch --shallow-exclude one origin &&\n \tgit -C shallow12 log --pretty=tformat:%s origin/master >actual &&\n \ttest_write_lines three two >expected &&\n \ttest_cmp expected actual\n@@ -806,7 +809,7 @@ test_expect_success 'fetching deepen' '\n \tgit -C deepen log --pretty=tformat:%s master >actual &&\n \techo three >expected &&\n \ttest_cmp expected actual &&\n-\tgit -C deepen fetch --deepen=1 &&\n+\tGIT_TEST_PROTOCOL_VERSION= git -C deepen fetch --deepen=1 &&\n \tgit -C deepen log --pretty=tformat:%s origin/master >actual &&\n \tcat >expected <<-\\EOF &&\n \tfour\ndiff --git a/t/t5503-tagfollow.sh b/t/t5503-tagfollow.sh\nindex 4ca48f0276..0e90a90294 100755\n--- a/t/t5503-tagfollow.sh\n+++ b/t/t5503-tagfollow.sh\n@@ -56,7 +56,7 @@ test_expect_success 'fetch A (new commit : 1 connection)' '\n \trm -f $U &&\n \t(\n \t\tcd cloned &&\n-\t\tGIT_TRACE_PACKET=$UPATH git fetch &&\n+\t\tGIT_TRACE_PACKET=$UPATH GIT_TEST_PROTOCOL_VERSION= git fetch &&\n \t\ttest $A = $(git rev-parse --verify origin/master)\n \t) &&\n \tget_needs $U >actual &&\n@@ -86,7 +86,7 @@ test_expect_success 'fetch C, T (new branch, tag : 1 connection)' '\n \trm -f $U &&\n \t(\n \t\tcd cloned &&\n-\t\tGIT_TRACE_PACKET=$UPATH git fetch &&\n+\t\tGIT_TRACE_PACKET=$UPATH GIT_TEST_PROTOCOL_VERSION= git fetch &&\n \t\ttest $C = $(git rev-parse --verify origin/cat) &&\n \t\ttest $T = $(git rev-parse --verify tag1) &&\n \t\ttest $A = $(git rev-parse --verify tag1^0)\n@@ -122,7 +122,7 @@ test_expect_success 'fetch B, S (commit and tag : 1 connection)' '\n \trm -f $U &&\n \t(\n \t\tcd cloned &&\n-\t\tGIT_TRACE_PACKET=$UPATH git fetch &&\n+\t\tGIT_TRACE_PACKET=$UPATH GIT_TEST_PROTOCOL_VERSION= git fetch &&\n \t\ttest $B = $(git rev-parse --verify origin/master) &&\n \t\ttest $B = $(git rev-parse --verify tag2^0) &&\n \t\ttest $S = $(git rev-parse --verify tag2)\n@@ -146,7 +146,7 @@ test_expect_success 'new clone fetch master and tags' '\n \t\tcd clone2 &&\n \t\tgit init &&\n \t\tgit remote add origin .. &&\n-\t\tGIT_TRACE_PACKET=$UPATH git fetch &&\n+\t\tGIT_TRACE_PACKET=$UPATH GIT_TEST_PROTOCOL_VERSION= git fetch &&\n \t\ttest $B = $(git rev-parse --verify origin/master) &&\n \t\ttest $S = $(git rev-parse --verify tag2) &&\n \t\ttest $B = $(git rev-parse --verify tag2^0) &&\ndiff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh\nindex ca69636fd5..7b480587c9 100755\n--- a/t/t5512-ls-remote.sh\n+++ b/t/t5512-ls-remote.sh\n@@ -223,7 +223,7 @@ test_expect_success 'ls-remote --symref' '\n \t$(git rev-parse refs/tags/mark1.10)\trefs/tags/mark1.10\n \t$(git rev-parse refs/tags/mark1.2)\trefs/tags/mark1.2\n \tEOF\n-\tgit ls-remote --symref >actual &&\n+\tGIT_TEST_PROTOCOL_VERSION= git ls-remote --symref >actual &&\n \ttest_cmp expect actual\n '\n \n@@ -243,7 +243,7 @@ test_expect_failure 'ls-remote with filtered symref (--heads)' '\n \t1bd44cb9d13204b0fe1958db0082f5028a16eb3a\trefs/heads/foo\n \t1bd44cb9d13204b0fe1958db0082f5028a16eb3a\trefs/heads/master\n \tEOF\n-\tgit ls-remote --symref --heads . >actual &&\n+\tGIT_TEST_PROTOCOL_VERSION= git ls-remote --symref --heads . >actual &&\n \ttest_cmp expect actual\n '\n \n@@ -252,9 +252,9 @@ test_expect_success 'ls-remote --symref omits filtered-out matches' '\n \t1bd44cb9d13204b0fe1958db0082f5028a16eb3a\trefs/heads/foo\n \t1bd44cb9d13204b0fe1958db0082f5028a16eb3a\trefs/heads/master\n \tEOF\n-\tgit ls-remote --symref --heads . >actual &&\n+\tGIT_TEST_PROTOCOL_VERSION= git ls-remote --symref --heads . >actual &&\n \ttest_cmp expect actual &&\n-\tgit ls-remote --symref . \"refs/heads/*\" >actual &&\n+\tGIT_TEST_PROTOCOL_VERSION= git ls-remote --symref . \"refs/heads/*\" >actual &&\n \ttest_cmp expect actual\n '\n \ndiff --git a/t/t5515-fetch-merge-logic.sh b/t/t5515-fetch-merge-logic.sh\nindex 36b0dbc01c..f095555c3e 100755\n--- a/t/t5515-fetch-merge-logic.sh\n+++ b/t/t5515-fetch-merge-logic.sh\n@@ -147,7 +147,7 @@ do\n \t\t\tdo\n \t\t\t\tgit update-ref -d \"$refname\" \"$val\"\n \t\t\tdone\n-\t\t\tgit fetch \"$@\" >/dev/null\n+\t\t\tGIT_TEST_PROTOCOL_VERSION= git fetch \"$@\" >/dev/null\n \t\t\tcat .git/FETCH_HEAD\n \t\t} >\"$actual_f\" &&\n \t\tgit show-ref >\"$actual_r\" &&\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 08cdee0b95..1d1b717cd5 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -1129,7 +1129,7 @@ do\n \t'\n done\n \n-test_expect_success 'fetch exact SHA1' '\n+test_expect_success 'fetch exact SHA1 in protocol v0' '\n \tmk_test testrepo heads/master hidden/one &&\n \tgit push testrepo master:refs/hidden/one &&\n \t(\n@@ -1148,7 +1148,8 @@ test_expect_success 'fetch exact SHA1' '\n \t\ttest_must_fail git cat-file -t $the_commit &&\n \n \t\t# fetching the hidden object should fail by default\n-\t\ttest_must_fail git fetch -v ../testrepo $the_commit:refs/heads/copy 2>err &&\n+\t\ttest_must_fail env GIT_TEST_PROTOCOL_VERSION= \\\n+\t\t\tgit fetch -v ../testrepo $the_commit:refs/heads/copy 2>err &&\n \t\ttest_i18ngrep \"Server does not allow request for unadvertised object\" err &&\n \t\ttest_must_fail git rev-parse --verify refs/heads/copy &&\n \n@@ -1205,7 +1206,8 @@ do\n \t\tmk_empty shallow &&\n \t\t(\n \t\t\tcd shallow &&\n-\t\t\ttest_must_fail git fetch --depth=1 ../testrepo/.git $SHA1 &&\n+\t\t\ttest_must_fail env GIT_TEST_PROTOCOL_VERSION= \\\n+\t\t\t\tgit fetch --depth=1 ../testrepo/.git $SHA1 &&\n \t\t\tgit --git-dir=../testrepo/.git config uploadpack.allowreachablesha1inwant true &&\n \t\t\tgit fetch --depth=1 ../testrepo/.git $SHA1 &&\n \t\t\tgit cat-file commit $SHA1\n@@ -1233,15 +1235,18 @@ do\n \t\tmk_empty shallow &&\n \t\t(\n \t\t\tcd shallow &&\n-\t\t\ttest_must_fail ok=sigpipe git fetch ../testrepo/.git $SHA1_3 &&\n-\t\t\ttest_must_fail ok=sigpipe git fetch ../testrepo/.git $SHA1_1 &&\n+\t\t\ttest_must_fail ok=sigpipe env GIT_TEST_PROTOCOL_VERSION= \\\n+\t\t\t\tgit fetch ../testrepo/.git $SHA1_3 &&\n+\t\t\ttest_must_fail ok=sigpipe env GIT_TEST_PROTOCOL_VERSION= \\\n+\t\t\t\tgit fetch ../testrepo/.git $SHA1_1 &&\n \t\t\tgit --git-dir=../testrepo/.git config uploadpack.allowreachablesha1inwant true &&\n \t\t\tgit fetch ../testrepo/.git $SHA1_1 &&\n \t\t\tgit cat-file commit $SHA1_1 &&\n \t\t\ttest_must_fail git cat-file commit $SHA1_2 &&\n \t\t\tgit fetch ../testrepo/.git $SHA1_2 &&\n \t\t\tgit cat-file commit $SHA1_2 &&\n-\t\t\ttest_must_fail ok=sigpipe git fetch ../testrepo/.git $SHA1_3\n+\t\t\ttest_must_fail ok=sigpipe env GIT_TEST_PROTOCOL_VERSION= \\\n+\t\t\t\tgit fetch ../testrepo/.git $SHA1_3\n \t\t)\n \t'\n done\ndiff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh\nindex 6faf17e17a..85b3022ce6 100755\n--- a/t/t5537-fetch-shallow.sh\n+++ b/t/t5537-fetch-shallow.sh\n@@ -127,7 +127,8 @@ test_expect_success 'fetch that requires changes in .git/shallow is filtered' '\n \tgit init notshallow &&\n \t(\n \tcd notshallow &&\n-\tgit fetch ../shallow/.git refs/heads/*:refs/remotes/shallow/*&&\n+\tGIT_TEST_PROTOCOL_VERSION= \\\n+\t\tgit fetch ../shallow/.git refs/heads/*:refs/remotes/shallow/* &&\n \tgit for-each-ref --format=\"%(refname)\" >actual.refs &&\n \tcat <expect.refs &&\n refs/remotes/shallow/no-shallow\ndiff --git a/t/t5539-fetch-http-shallow.sh b/t/t5539-fetch-http-shallow.sh\nindex 5fbf67c446..a121e19bc7 100755\n--- a/t/t5539-fetch-http-shallow.sh\n+++ b/t/t5539-fetch-http-shallow.sh\n@@ -67,7 +67,8 @@ test_expect_success 'no shallow lines after receiving ACK ready' '\n \t\tcd clone &&\n \t\tgit checkout --orphan newnew &&\n \t\ttest_commit new-too &&\n-\t\tGIT_TRACE_PACKET=\"$TRASH_DIRECTORY/trace\" git fetch --depth=2 &&\n+\t\tGIT_TRACE_PACKET=\"$TRASH_DIRECTORY/trace\" GIT_TEST_PROTOCOL_VERSION= \\\n+\t\t\tgit fetch --depth=2 &&\n \t\tgrep \"fetch-pack< ACK .* ready\" ../trace &&\n \t\t! grep \"fetch-pack> done\" ../trace\n \t)\n@@ -114,7 +115,7 @@ test_expect_success 'shallow clone exclude tag two' '\n '\n \n test_expect_success 'fetch exclude tag one' '\n-\tgit -C shallow12 fetch --shallow-exclude one origin &&\n+\tGIT_TEST_PROTOCOL_VERSION= git -C shallow12 fetch --shallow-exclude one origin &&\n \tgit -C shallow12 log --pretty=tformat:%s origin/master >actual &&\n \ttest_write_lines three two >expected &&\n \ttest_cmp expected actual\n@@ -128,14 +129,14 @@ test_expect_success 'fetching deepen' '\n \ttest_commit two &&\n \ttest_commit three &&\n \tmv .git \"$HTTPD_DOCUMENT_ROOT_PATH/shallow-deepen.git\" &&\n-\tgit clone --depth 1 $HTTPD_URL/smart/shallow-deepen.git deepen &&\n+\tGIT_TEST_PROTOCOL_VERSION= git clone --depth 1 $HTTPD_URL/smart/shallow-deepen.git deepen &&\n \tmv \"$HTTPD_DOCUMENT_ROOT_PATH/shallow-deepen.git\" .git &&\n \ttest_commit four &&\n \tgit -C deepen log --pretty=tformat:%s master >actual &&\n \techo three >expected &&\n \ttest_cmp expected actual &&\n \tmv .git \"$HTTPD_DOCUMENT_ROOT_PATH/shallow-deepen.git\" &&\n-\tgit -C deepen fetch --deepen=1 &&\n+\tGIT_TEST_PROTOCOL_VERSION= git -C deepen fetch --deepen=1 &&\n \tgit -C deepen log --pretty=tformat:%s origin/master >actual &&\n \tcat >expected <<-\\EOF &&\n \tfour\ndiff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh\nindex 5475afc052..180c9005b7 100755\n--- a/t/t5541-http-push-smart.sh\n+++ b/t/t5541-http-push-smart.sh\n@@ -45,14 +45,19 @@ test_expect_success 'no empty path components' '\n \t# In the URL, add a trailing slash, and see if git appends yet another\n \t# slash.\n \tcd \"$ROOT_PATH\" &&\n-\tgit clone $HTTPD_URL/smart/test_repo.git/ test_repo_clone &&\n+\t# Other protocol versions may make different requests, so perform this\n+\t# clone with the default protocol.\n+\tGIT_TEST_PROTOCOL_VERSION= git clone $HTTPD_URL/smart/test_repo.git/ test_repo_clone &&\n \n \tcheck_access_log exp\n '\n \n test_expect_success 'clone remote repository' '\n \trm -rf test_repo_clone &&\n-\tgit clone $HTTPD_URL/smart/test_repo.git test_repo_clone &&\n+\t# Other protocol versions may make different requests, so perform this\n+\t# clone with the default protocol.\n+\tGIT_TEST_PROTOCOL_VERSION= git clone $HTTPD_URL/smart/test_repo.git test_repo_clone &&\n+\n \t(\n \t\tcd test_repo_clone && git config push.default matching\n \t)\ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex 8630b0cc39..a51993f35a 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -43,7 +43,8 @@ test_expect_success 'clone http repository' '\n \t< Cache-Control: no-cache, max-age=0, must-revalidate\n \t< Content-Type: application/x-git-upload-pack-result\n \tEOF\n-\tGIT_TRACE_CURL=true git clone --quiet $HTTPD_URL/smart/repo.git clone 2>err &&\n+\tGIT_TRACE_CURL=true GIT_TEST_PROTOCOL_VERSION= \\\n+\t\tgit clone --quiet $HTTPD_URL/smart/repo.git clone 2>err &&\n \ttest_cmp file clone/file &&\n \ttr '\\''\\015'\\'' Q >file &&\n \tgit commit -a -m two &&\n \tgit push public &&\n-\t(cd clone && git pull) &&\n+\t(cd clone && GIT_TEST_PROTOCOL_VERSION= git pull) &&\n \ttest_cmp file clone/file\n '\n \n@@ -143,7 +144,7 @@ test_expect_success 'clone from auth-only-for-push repository' '\n test_expect_success 'clone from auth-only-for-objects repository' '\n \techo two >expect &&\n \tset_askpass user@host pass@host &&\n-\tgit clone --bare \"$HTTPD_URL/auth-fetch/smart/repo.git\" half-auth &&\n+\tGIT_TEST_PROTOCOL_VERSION= git clone --bare \"$HTTPD_URL/auth-fetch/smart/repo.git\" half-auth &&\n \texpect_askpass both user@host &&\n \tgit --git-dir=half-auth log -1 --format=%s >actual &&\n \ttest_cmp expect actual\n@@ -151,7 +152,7 @@ test_expect_success 'clone from auth-only-for-objects repository' '\n \n test_expect_success 'no-op half-auth fetch does not require a password' '\n \tset_askpass wrong &&\n-\tgit --git-dir=half-auth fetch &&\n+\tGIT_TEST_PROTOCOL_VERSION= git --git-dir=half-auth fetch &&\n \texpect_askpass none\n '\n \n@@ -187,7 +188,7 @@ test_expect_success 'create namespaced refs' '\n '\n \n test_expect_success 'smart clone respects namespace' '\n-\tgit clone \"$HTTPD_URL/smart_namespace/repo.git\" ns-smart &&\n+\tGIT_TEST_PROTOCOL_VERSION= git clone \"$HTTPD_URL/smart_namespace/repo.git\" ns-smart &&\n \techo namespaced >expect &&\n \tgit --git-dir=ns-smart/.git log -1 --format=%s >actual &&\n \ttest_cmp expect actual\n@@ -214,7 +215,7 @@ test_expect_success 'cookies stored in http.cookiefile when http.savecookies set\n \tEOF\n \tgit config http.cookiefile cookies.txt &&\n \tgit config http.savecookies true &&\n-\tgit ls-remote $HTTPD_URL/smart_cookies/repo.git master &&\n+\tGIT_TEST_PROTOCOL_VERSION= git ls-remote $HTTPD_URL/smart_cookies/repo.git master &&\n \ttail -3 cookies.txt | sort >cookies_tail.txt &&\n \ttest_cmp expect_cookies.txt cookies_tail.txt\n '\n@@ -306,7 +307,8 @@ test_expect_success 'test allowreachablesha1inwant with unreachable' '\n \n \tgit init --bare test_reachable.git &&\n \tgit -C test_reachable.git remote add origin \"$HTTPD_URL/smart/repo.git\" &&\n-\ttest_must_fail git -C test_reachable.git fetch origin \"$(git rev-parse HEAD)\"\n+\ttest_must_fail env GIT_TEST_PROTOCOL_VERSION= \\\n+\t\tgit -C test_reachable.git fetch origin \"$(git rev-parse HEAD)\"\n '\n \n test_expect_success 'test allowanysha1inwant with unreachable' '\n@@ -325,7 +327,8 @@ test_expect_success 'test allowanysha1inwant with unreachable' '\n \n \tgit init --bare test_reachable.git &&\n \tgit -C test_reachable.git remote add origin \"$HTTPD_URL/smart/repo.git\" &&\n-\ttest_must_fail git -C test_reachable.git fetch origin \"$(git rev-parse HEAD)\" &&\n+\ttest_must_fail env GIT_TEST_PROTOCOL_VERSION= \\\n+\t\tgit -C test_reachable.git fetch origin \"$(git rev-parse HEAD)\" &&\n \n \tgit -C \"$server\" config uploadpack.allowanysha1inwant 1 &&\n \tgit -C test_reachable.git fetch origin \"$(git rev-parse HEAD)\"\ndiff --git a/t/t5552-skipping-fetch-negotiator.sh b/t/t5552-skipping-fetch-negotiator.sh\nindex 30857b84a8..979e6583d4 100755\n--- a/t/t5552-skipping-fetch-negotiator.sh\n+++ b/t/t5552-skipping-fetch-negotiator.sh\n@@ -127,7 +127,7 @@ test_expect_success 'use ref advertisement to filter out commits' '\n \t# not need to send any ancestors of \"c3\", but we still need to send \"c3\"\n \t# itself.\n \ttest_config -C client fetch.negotiationalgorithm skipping &&\n-\ttrace_fetch client origin to_fetch &&\n+\tGIT_TEST_PROTOCOL_VERSION= trace_fetch client origin to_fetch &&\n \thave_sent c5 c4^ c2side &&\n \thave_not_sent c4 c4^^ c4^^^\n '\n@@ -189,7 +189,7 @@ test_expect_success 'do not send \"have\" with ancestors of commits that server AC\n \ttest_commit -C server commit-on-b1 &&\n \n \ttest_config -C client fetch.negotiationalgorithm skipping &&\n-\ttrace_fetch client \"$(pwd)/server\" to_fetch &&\n+\tGIT_TEST_PROTOCOL_VERSION= trace_fetch client \"$(pwd)/server\" to_fetch &&\n \tgrep \" fetch\" trace &&\n \n \t# fetch-pack sends 2 requests each containing 16 \"have\" lines before\ndiff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh\nindex 7466aad111..c42ee245cd 100755\n--- a/t/t5570-git-daemon.sh\n+++ b/t/t5570-git-daemon.sh\n@@ -190,7 +190,7 @@ test_expect_success 'daemon log records all attributes' '\n \tEOF\n \t>daemon.log &&\n \tGIT_OVERRIDE_VIRTUAL_HOST=localhost \\\n-\t\tgit -c protocol.version=1 \\\n+\t\tGIT_TEST_PROTOCOL_VERSION= git -c protocol.version=1 \\\n \t\t\tls-remote \"$GIT_DAEMON_URL/interp.git\" &&\n \tgrep -i extended.attribute daemon.log | cut -d\" \" -f2- >actual &&\n \ttest_cmp expect actual\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex e87164aa8f..a555e38845 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -943,7 +943,8 @@ test_expect_success 'submodule update clone shallow submodule outside of depth'\n \t\tcd super3 &&\n \t\tsed -e \"s#url = ../#url = file://$pwd/#\" <.gitmodules >.gitmodules.tmp &&\n \t\tmv -f .gitmodules.tmp .gitmodules &&\n-\t\ttest_must_fail git submodule update --init --depth=1 2>actual &&\n+\t\ttest_must_fail env GIT_TEST_PROTOCOL_VERSION= \\\n+\t\t\tgit submodule update --init --depth=1 2>actual &&\n \t\ttest_i18ngrep \"Direct fetching of that commit failed.\" actual &&\n \t\tgit -C ../submodule config uploadpack.allowReachableSHA1InWant true &&\n \t\tgit submodule update --init --depth=1 >actual &&\n-- \n2.20.0.405.gbc1bbc6f85\n\n"},{"id":"365269","messageId":"87pnu51kac.fsf@evledraar.gmail.com","threadId":"50006","inReplyTo":"20181213155817.27666-9-avarab@gmail.com","subject":"Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-12-13T16:08:43Z","receivedAt":"2018-12-13T16:08:51Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Dec 13 2018, Ævar Arnfjörð Bjarmason wrote:\n\nNow that we have this maybe we should discuss why these tests show\ndifferent things:\n\n> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\n> index 086f2c40f6..8b1217ea26 100755\n> --- a/t/t5500-fetch-pack.sh\n> +++ b/t/t5500-fetch-pack.sh\n> @@ -628,7 +628,10 @@ test_expect_success 'fetch-pack cannot fetch a raw sha1 that is not advertised a\n> \ttest_commit -C server 6 &&\n>\n> \tgit init client &&\n> -\ttest_must_fail git -C client fetch-pack ../server \\\n> +\n> +\t# Other protocol versions (e.g. 2) allow fetching an unadvertised\n> +\t# object, so run this test with the default protocol version (0).\n> +\ttest_must_fail env GIT_TEST_PROTOCOL_VERSION= git -C client fetch-pack ../server \\\n> \t\t$(git -C server rev-parse refs/heads/master^) 2>err &&\n\nWhat? So the equivalent of uploadpack.allowAnySHA1InWant=true is on for\nv2 all the time?\n\n> \ttest_i18ngrep \"Server does not allow request for unadvertised object\" err\n> '\n> @@ -788,7 +791,7 @@ test_expect_success 'shallow clone exclude tag two' '\n> '\n>\n> test_expect_success 'fetch exclude tag one' '\n> -\tgit -C shallow12 fetch --shallow-exclude one origin &&\n> +\tGIT_TEST_PROTOCOL_VERSION= git -C shallow12 fetch --shallow-exclude one origin &&\n> \tgit -C shallow12 log --pretty=tformat:%s origin/master >actual &&\n> \ttest_write_lines three two >expected &&\n> \ttest_cmp expected actual\n> @@ -806,7 +809,7 @@ test_expect_success 'fetching deepen' '\n> \tgit -C deepen log --pretty=tformat:%s master >actual &&\n> \techo three >expected &&\n> \ttest_cmp expected actual &&\n> -\tgit -C deepen fetch --deepen=1 &&\n> +\tGIT_TEST_PROTOCOL_VERSION= git -C deepen fetch --deepen=1 &&\n> \tgit -C deepen log --pretty=tformat:%s origin/master >actual &&\n> \tcat >expected <<-\\EOF &&\n> \tfour\n> diff --git a/t/t5503-tagfollow.sh b/t/t5503-tagfollow.sh\n> index 4ca48f0276..0e90a90294 100755\n> --- a/t/t5503-tagfollow.sh\n> +++ b/t/t5503-tagfollow.sh\n> @@ -56,7 +56,7 @@ test_expect_success 'fetch A (new commit : 1 connection)' '\n> \trm -f $U &&\n> \t(\n> \t\tcd cloned &&\n> -\t\tGIT_TRACE_PACKET=$UPATH git fetch &&\n> +\t\tGIT_TRACE_PACKET=$UPATH GIT_TEST_PROTOCOL_VERSION= git fetch &&\n> \t\ttest $A = $(git rev-parse --verify origin/master)\n> \t) &&\n> \tget_needs $U >actual &&\n> @@ -86,7 +86,7 @@ test_expect_success 'fetch C, T (new branch, tag : 1 connection)' '\n> \trm -f $U &&\n> \t(\n> \t\tcd cloned &&\n> -\t\tGIT_TRACE_PACKET=$UPATH git fetch &&\n> +\t\tGIT_TRACE_PACKET=$UPATH GIT_TEST_PROTOCOL_VERSION= git fetch &&\n> \t\ttest $C = $(git rev-parse --verify origin/cat) &&\n> \t\ttest $T = $(git rev-parse --verify tag1) &&\n> \t\ttest $A = $(git rev-parse --verify tag1^0)\n> @@ -122,7 +122,7 @@ test_expect_success 'fetch B, S (commit and tag : 1 connection)' '\n> \trm -f $U &&\n> \t(\n> \t\tcd cloned &&\n> -\t\tGIT_TRACE_PACKET=$UPATH git fetch &&\n> +\t\tGIT_TRACE_PACKET=$UPATH GIT_TEST_PROTOCOL_VERSION= git fetch &&\n> \t\ttest $B = $(git rev-parse --verify origin/master) &&\n> \t\ttest $B = $(git rev-parse --verify tag2^0) &&\n> \t\ttest $S = $(git rev-parse --verify tag2)\n> @@ -146,7 +146,7 @@ test_expect_success 'new clone fetch master and tags' '\n> \t\tcd clone2 &&\n> \t\tgit init &&\n> \t\tgit remote add origin .. &&\n> -\t\tGIT_TRACE_PACKET=$UPATH git fetch &&\n> +\t\tGIT_TRACE_PACKET=$UPATH GIT_TEST_PROTOCOL_VERSION= git fetch &&\n> \t\ttest $B = $(git rev-parse --verify origin/master) &&\n> \t\ttest $S = $(git rev-parse --verify tag2) &&\n> \t\ttest $B = $(git rev-parse --verify tag2^0) &&\n> diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh\n> index ca69636fd5..7b480587c9 100755\n> --- a/t/t5512-ls-remote.sh\n> +++ b/t/t5512-ls-remote.sh\n> @@ -223,7 +223,7 @@ test_expect_success 'ls-remote --symref' '\n> \t$(git rev-parse refs/tags/mark1.10)\trefs/tags/mark1.10\n> \t$(git rev-parse refs/tags/mark1.2)\trefs/tags/mark1.2\n> \tEOF\n> -\tgit ls-remote --symref >actual &&\n> +\tGIT_TEST_PROTOCOL_VERSION= git ls-remote --symref >actual &&\n> \ttest_cmp expect actual\n> '\n>\n> @@ -243,7 +243,7 @@ test_expect_failure 'ls-remote with filtered symref (--heads)' '\n> \t1bd44cb9d13204b0fe1958db0082f5028a16eb3a\trefs/heads/foo\n> \t1bd44cb9d13204b0fe1958db0082f5028a16eb3a\trefs/heads/master\n> \tEOF\n> -\tgit ls-remote --symref --heads . >actual &&\n> +\tGIT_TEST_PROTOCOL_VERSION= git ls-remote --symref --heads . >actual &&\n> \ttest_cmp expect actual\n> '\n>\n> @@ -252,9 +252,9 @@ test_expect_success 'ls-remote --symref omits filtered-out matches' '\n> \t1bd44cb9d13204b0fe1958db0082f5028a16eb3a\trefs/heads/foo\n> \t1bd44cb9d13204b0fe1958db0082f5028a16eb3a\trefs/heads/master\n> \tEOF\n> -\tgit ls-remote --symref --heads . >actual &&\n> +\tGIT_TEST_PROTOCOL_VERSION= git ls-remote --symref --heads . >actual &&\n> \ttest_cmp expect actual &&\n> -\tgit ls-remote --symref . \"refs/heads/*\" >actual &&\n> +\tGIT_TEST_PROTOCOL_VERSION= git ls-remote --symref . \"refs/heads/*\" >actual &&\n> \ttest_cmp expect actual\n> '\n>\n> diff --git a/t/t5515-fetch-merge-logic.sh b/t/t5515-fetch-merge-logic.sh\n> index 36b0dbc01c..f095555c3e 100755\n> --- a/t/t5515-fetch-merge-logic.sh\n> +++ b/t/t5515-fetch-merge-logic.sh\n\nThis one should really be looked at by someone more familiar with\nv2. Looks scary that we have different merge results with it.\n\n> @@ -147,7 +147,7 @@ do\n> \t\t\tdo\n> \t\t\t\tgit update-ref -d \"$refname\" \"$val\"\n> \t\t\tdone\n> -\t\t\tgit fetch \"$@\" >/dev/null\n> +\t\t\tGIT_TEST_PROTOCOL_VERSION= git fetch \"$@\" >/dev/null\n> \t\t\tcat .git/FETCH_HEAD\n> \t\t} >\"$actual_f\" &&\n> \t\tgit show-ref >\"$actual_r\" &&\n> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\n> index 08cdee0b95..1d1b717cd5 100755\n> --- a/t/t5516-fetch-push.sh\n> +++ b/t/t5516-fetch-push.sh\n> @@ -1129,7 +1129,7 @@ do\n> \t'\n> done\n>\n> -test_expect_success 'fetch exact SHA1' '\n> +test_expect_success 'fetch exact SHA1 in protocol v0' '\n> \tmk_test testrepo heads/master hidden/one &&\n> \tgit push testrepo master:refs/hidden/one &&\n> \t(\n> @@ -1148,7 +1148,8 @@ test_expect_success 'fetch exact SHA1' '\n> \t\ttest_must_fail git cat-file -t $the_commit &&\n>\n> \t\t# fetching the hidden object should fail by default\n> -\t\ttest_must_fail git fetch -v ../testrepo $the_commit:refs/heads/copy 2>err &&\n> +\t\ttest_must_fail env GIT_TEST_PROTOCOL_VERSION= \\\n> +\t\t\tgit fetch -v ../testrepo $the_commit:refs/heads/copy 2>err &&\n> \t\ttest_i18ngrep \"Server does not allow request for unadvertised object\" err &&\n> \t\ttest_must_fail git rev-parse --verify refs/heads/copy &&\n>\n> @@ -1205,7 +1206,8 @@ do\n> \t\tmk_empty shallow &&\n> \t\t(\n> \t\t\tcd shallow &&\n> -\t\t\ttest_must_fail git fetch --depth=1 ../testrepo/.git $SHA1 &&\n> +\t\t\ttest_must_fail env GIT_TEST_PROTOCOL_VERSION= \\\n> +\t\t\t\tgit fetch --depth=1 ../testrepo/.git $SHA1 &&\n> \t\t\tgit --git-dir=../testrepo/.git config uploadpack.allowreachablesha1inwant true &&\n> \t\t\tgit fetch --depth=1 ../testrepo/.git $SHA1 &&\n> \t\t\tgit cat-file commit $SHA1\n> @@ -1233,15 +1235,18 @@ do\n> \t\tmk_empty shallow &&\n> \t\t(\n> \t\t\tcd shallow &&\n> -\t\t\ttest_must_fail ok=sigpipe git fetch ../testrepo/.git $SHA1_3 &&\n> -\t\t\ttest_must_fail ok=sigpipe git fetch ../testrepo/.git $SHA1_1 &&\n> +\t\t\ttest_must_fail ok=sigpipe env GIT_TEST_PROTOCOL_VERSION= \\\n> +\t\t\t\tgit fetch ../testrepo/.git $SHA1_3 &&\n> +\t\t\ttest_must_fail ok=sigpipe env GIT_TEST_PROTOCOL_VERSION= \\\n> +\t\t\t\tgit fetch ../testrepo/.git $SHA1_1 &&\n> \t\t\tgit --git-dir=../testrepo/.git config uploadpack.allowreachablesha1inwant true &&\n> \t\t\tgit fetch ../testrepo/.git $SHA1_1 &&\n> \t\t\tgit cat-file commit $SHA1_1 &&\n> \t\t\ttest_must_fail git cat-file commit $SHA1_2 &&\n> \t\t\tgit fetch ../testrepo/.git $SHA1_2 &&\n> \t\t\tgit cat-file commit $SHA1_2 &&\n> -\t\t\ttest_must_fail ok=sigpipe git fetch ../testrepo/.git $SHA1_3\n> +\t\t\ttest_must_fail ok=sigpipe env GIT_TEST_PROTOCOL_VERSION= \\\n> +\t\t\t\tgit fetch ../testrepo/.git $SHA1_3\n> \t\t)\n> \t'\n\nDitto all this stuff.\n\n> [...]\n> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\n> index e87164aa8f..a555e38845 100755\n> --- a/t/t7406-submodule-update.sh\n> +++ b/t/t7406-submodule-update.sh\n> @@ -943,7 +943,8 @@ test_expect_success 'submodule update clone shallow submodule outside of depth'\n> \t\tcd super3 &&\n> \t\tsed -e \"s#url = ../#url = file://$pwd/#\" <.gitmodules >.gitmodules.tmp &&\n> \t\tmv -f .gitmodules.tmp .gitmodules &&\n> -\t\ttest_must_fail git submodule update --init --depth=1 2>actual &&\n> +\t\ttest_must_fail env GIT_TEST_PROTOCOL_VERSION= \\\n> +\t\t\tgit submodule update --init --depth=1 2>actual &&\n> \t\ttest_i18ngrep \"Direct fetching of that commit failed.\" actual &&\n> \t\tgit -C ../submodule config uploadpack.allowReachableSHA1InWant true &&\n> \t\tgit submodule update --init --depth=1 >actual &&\n\nAnd this one and various other shallow things look odd, are shallow\nfetches different under v2?\n"},{"id":"365293","messageId":"20181213194848.248308-1-jonathantanmy@google.com","threadId":"50006","inReplyTo":"20181213155817.27666-6-avarab@gmail.com","subject":"Re: [PATCH v2 5/8] tests: add a special setup where for protocol.version","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-12-13T19:48:48Z","receivedAt":"2018-12-13T19:48:54Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Regarding the subject, I think you mean \"add a special setup var\", not\n\"add a special setup where\".\n\n> +GIT_TEST_PROTOCOL_VERSION=<'protocol.version' config value>, when set,\n> +runs the test suite with the given protocol.version. E.g. \"0\", \"1\" or\n> +\"2\". Can be set to the empty string within tests themselves (e.g. \"env\n> +GIT_TEST_PROTOCOL_VERSION= \") to unset the value in the\n> +environment as a workaround for \"env --unset\" not being portable.\n\nOptional: I prefer the \"overrides\" language used in\nGIT_TEST_COMMIT_GRAPH to make it clear that any existing setting is\nignored. E.g.:\n\n GIT_TEST_PROTOCOL_VERSION=<'protocol.version' config value>, when set\n to a non-empty string, overrides the 'protocol.version' value. E.g.\n \"0\", ...\n\nOther than that, this patch and the other patches (1-4, 6, 8) look good\nto me. My patch 7 looks good too, but it's probably better if someone\nelse looks at it too :-)\n"},{"id":"365294","messageId":"20181213195305.249059-1-jonathantanmy@google.com","threadId":"50006","inReplyTo":"20181211104236.GA6899@sigill.intra.peff.net","subject":"Re: [PATCH 0/3] protocol v2 and hidden refs","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-12-13T19:53:05Z","receivedAt":"2018-12-13T19:53:12Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Just realized that I haven't replied to this yet...\n\n> - I'm a little worried this may happen again with future features. The\n> root cause is that \"ls-refs\" follows a different code path than the\n> ref advertisement for \"upload-pack\". So if we add any new config,\n> it needs to go both places (non ref-advertisement config is OK, as\n> the v2 \"fetch\" command is a lot closer to a v0 upload-pack).\n> \n> I think this is just an issue for any future features. I looked for\n> other existing features which might be missing in v2, but couldn't\n> find any.\n> \n> I don't know if there's a good solution. I tried running the whole\n> test suite with v2 as the default. It does find this bug, but it has\n> a bunch of other problems (notably fetch-pack won't run as v2, but\n> some other tests I think also depend on v0's reachability rules,\n> which v2 is documented not to enforce).\n\nI think Aevar's patches (sent after you wrote this) is a good start, and\nI have started looking at it too.\n\n> - The \"serve\" command is funky, because it has no concept of whether\n> the \"ls-refs\" is for fetching or pushing. Is git-serve even a thing\n> that we want to support going forward? I know part of the original\n> v2 conception was that one would be able to just connect to\n> \"git-serve\" and do a number of operations. But in practice the v2\n> probing requires saying \"I'd like to git-upload-pack, and v2 if you\n> please\". So no client ever calls git-serve.\n> \n> Is this something we plan to eventually move to? Or can it be\n> considered a funny vestige of the development? In the latter case, I\n> think we should consider removing it.\n\nPersonally, I lean towards removing it, but there are arguments on both\nsides. In particular, removing \"serve\" means that both developers and\nusers of Git need not be concerned with a 3rd endpoint, but preserving\n\"serve\" (and planning to migrate away from \"upload-pack\" and\n\"receive-pack\") means that we will only have one endpoint, eliminating\nconfusion about which endpoint to use when making certain requests (when\nwe add requests other than \"fetch\" and \"push\").\n"},{"id":"365315","messageId":"xmqqo99oeu4u.fsf@gitster-ct.c.googlers.com","threadId":"50006","inReplyTo":"20181211104342.GA7233@sigill.intra.peff.net","subject":"Re: [PATCH 1/3] serve: pass \"config context\" through to individual commands","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-14T02:09:53Z","receivedAt":"2018-12-14T02:10:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> In protocol v2, instead of just running \"upload-pack\", we have a generic\n> \"serve\" loop which runs command requests from the client. What used to\n> be \"upload-pack\" is now generally split into two operations: \"ls-refs\"\n> and \"fetch\". The latter knows it must respect uploadpack.* config, but\n> the former is not actually specific to a fetch operation (we do not yet\n> do v2 receive-pack, but eventually we may, and ls-refs would support\n> both operations).\n>\n> However, ls-refs does need to know which operation we're performing, in\n> order to read the correct config (e.g., uploadpack.hiderefs versus\n> receive.hiderefs; we don't read _either_ right now, but this is the\n> first step to fixing that).\n>\n> In the generic \"git-serve\" program, we don't have that information. But\n> in the protocol as it is actually used by clients, the client still asks\n> to run \"git-upload-pack\", and then issues an \"ls-refs\" from there. So we\n> _do_ know at that point that \"uploadpack\" is the right config context.\n> This patch teaches the protocol v2 \"serve\" code to pass that context\n> through to the individual commands (a future patch will teach ls-refs to\n> actually use it).\n\nThanks for a clear description of ugly status quo X-<.\n\n> diff --git a/ls-refs.h b/ls-refs.h\n> index b62877e8da..da26fc9824 100644\n> --- a/ls-refs.h\n> +++ b/ls-refs.h\n> @@ -4,7 +4,8 @@\n> struct repository;\n> struct argv_array;\n> struct packet_reader;\n> -extern int ls_refs(struct repository *r, struct argv_array *keys,\n> +extern int ls_refs(struct repository *r, const char *config_context,\n> +\t\t struct argv_array *keys,\n> \t\t struct packet_reader *request);\n\nOne thing I wonder is if we want to pass the whole *_opt thing,\ninstead of only one field out of it.\n\n> #endif /* LS_REFS_H */\n> diff --git a/serve.c b/serve.c\n> index bda085f09c..70f89cf0d9 100644\n> --- a/serve.c\n> +++ b/serve.c\n> @@ -48,6 +48,7 @@ struct protocol_capability {\n> \t * This field should be NULL for capabilities which are not commands.\n> \t */\n> \tint (*command)(struct repository *r,\n> +\t\t const char *config_context,\n\nLikewise here.\n\n> \t\t struct argv_array *keys,\n> \t\t struct packet_reader *request);\n> };\n\nThanks.\n"},{"id":"365316","messageId":"xmqqk1kceu23.fsf@gitster-ct.c.googlers.com","threadId":"50006","inReplyTo":"20181211104358.GB7233@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] parse_hide_refs_config: handle NULL section","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-14T02:11:32Z","receivedAt":"2018-12-14T02:11:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> This helper function looks for config in two places: transfer.hiderefs,\n> or $section.hiderefs, where $section is passed in by the caller (and is\n> \"uploadpack\" or \"receive\", depending on the context).\n>\n> In preparation for callers which do not even have that context (namely\n> the \"git-serve\" command), let's handle a NULL by looking only at\n> transfer.hiderefs (as opposed to segfaulting).\n\nMakes sense, and I too wonder if we want to keep the \"serve\" thing\nin the longer term, at least in the current form.\n\n"},{"id":"365317","messageId":"xmqqftv0etqh.fsf@gitster-ct.c.googlers.com","threadId":"50006","inReplyTo":"87pnu51kac.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-14T02:18:30Z","receivedAt":"2018-12-14T02:18:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n>> +\t# Other protocol versions (e.g. 2) allow fetching an unadvertised\n>> +\t# object, so run this test with the default protocol version (0).\n>> +\ttest_must_fail env GIT_TEST_PROTOCOL_VERSION= git -C client fetch-pack ../server \\\n>> \t\t$(git -C server rev-parse refs/heads/master^) 2>err &&\n>\n> What? So the equivalent of uploadpack.allowAnySHA1InWant=true is on for\n> v2 all the time?\n\nUnfortunatelyYes (see Peff's three-patch series).\n"},{"id":"365329","messageId":"20181214082050.GA11777@sigill.intra.peff.net","threadId":"50006","inReplyTo":"xmqqo99oeu4u.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/3] serve: pass \"config context\" through to individual commands","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-12-14T08:20:50Z","receivedAt":"2018-12-14T08:20:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 14, 2018 at 11:09:53AM +0900, Junio C Hamano wrote:\n\n> > diff --git a/ls-refs.h b/ls-refs.h\n> > index b62877e8da..da26fc9824 100644\n> > --- a/ls-refs.h\n> > +++ b/ls-refs.h\n> > @@ -4,7 +4,8 @@\n> > struct repository;\n> > struct argv_array;\n> > struct packet_reader;\n> > -extern int ls_refs(struct repository *r, struct argv_array *keys,\n> > +extern int ls_refs(struct repository *r, const char *config_context,\n> > +\t\t struct argv_array *keys,\n> > \t\t struct packet_reader *request);\n> \n> One thing I wonder is if we want to pass the whole *_opt thing,\n> instead of only one field out of it.\n\nI actually started by doing that, but \"struct serve_options\" is not\ncurrently known by ls-refs.c, upload-pack.c, etc. So they'd have to\nstart including \"serve.h\". I don't think that's the end of the world,\nbut it felt like a funny mutual circular to me (my mental model now is\nthat ls-refs, upload-pack, etc are low-level commands, tied together by\nthe \"serve\" stuff).\n\n-Peff\n"},{"id":"365330","messageId":"20181214083507.GB11777@sigill.intra.peff.net","threadId":"50006","inReplyTo":"20181213195305.249059-1-jonathantanmy@google.com","subject":"Re: [PATCH 0/3] protocol v2 and hidden refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-12-14T08:35:08Z","receivedAt":"2018-12-14T08:35:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 13, 2018 at 11:53:05AM -0800, Jonathan Tan wrote:\n\n> > I don't know if there's a good solution. I tried running the whole\n> > test suite with v2 as the default. It does find this bug, but it has\n> > a bunch of other problems (notably fetch-pack won't run as v2, but\n> > some other tests I think also depend on v0's reachability rules,\n> > which v2 is documented not to enforce).\n> \n> I think Aevar's patches (sent after you wrote this) is a good start, and\n> I have started looking at it too.\n\nYeah, I'm excited to see it working with fetch-pack, as the current\nbehavior is to complain if you've tried to enable v2 config:\n\n $ git config protocol.version 2\n $ git fetch-pack git://github.com/git/git\n fatal: support for protocol v2 not implemented yet\n\nI haven't actually run into it in the real world, but somebody might if\nthey have scripted around fetch-pack and are experimenting with v2. A\nmuch friendlier behavior would be falling back to v1, but actually\nsupporting v2 is better still. :)\n\n> > - The \"serve\" command is funky, because it has no concept of whether\n> > the \"ls-refs\" is for fetching or pushing. Is git-serve even a thing\n> > that we want to support going forward? I know part of the original\n> > v2 conception was that one would be able to just connect to\n> > \"git-serve\" and do a number of operations. But in practice the v2\n> > probing requires saying \"I'd like to git-upload-pack, and v2 if you\n> > please\". So no client ever calls git-serve.\n> > \n> > Is this something we plan to eventually move to? Or can it be\n> > considered a funny vestige of the development? In the latter case, I\n> > think we should consider removing it.\n> \n> Personally, I lean towards removing it, but there are arguments on both\n> sides. In particular, removing \"serve\" means that both developers and\n> users of Git need not be concerned with a 3rd endpoint, but preserving\n> \"serve\" (and planning to migrate away from \"upload-pack\" and\n> \"receive-pack\") means that we will only have one endpoint, eliminating\n> confusion about which endpoint to use when making certain requests (when\n> we add requests other than \"fetch\" and \"push\").\n\nYeah, at first glance I like the simplicity of a unified model. But the\nseparate fetch/push endpoints have been useful in the past. Separate\nuploadpack/receive.hiderefs that I dealt with here are one form. Another\nis that many people do HTTP access control using the endpoints. For\nexample, if I have a repo which is public-read and private-write, the\nconfig we recommend in git-http-backend(1) is to lock down the\nreceive-pack access using webserver config.\n\nIf all the webserver sees is \"somebody wants to connect to git-serve\",\nit doesn't know if it should be challenging them for authentication or\nnot. It would have to start peeking into the git-serve conversation to\nsee what the client actually wants to do. That's _possible_ to do, but\nit gets pretty awkward with existing webserver tools (whereas matching\nthe URI endpoint is pretty easy).\n\nDitto for locked down ssh sessions like git-shell (or custom solutions\nlike gitolite). Right now we can \"git-upload-pack is OK on this repo,\ngit-receive-pack is not\". But blindly running \"git serve\" would be\ndangerous. In this case I think we have a few more options, because the\nuser has always already authenticated. So we can just tell \"git serve\"\nvia the environment whether the user is authorized for push. It's harder\nwith HTTP because most setups avoid even challenging for auth unless\nit's necessary.\n\nSo I'm a bit worried that the unified endpoint model is going to be a\ndead end, at which point carrying around git-serve just makes things\nmore complicated.\n\n-Peff\n"},{"id":"365331","messageId":"20181214083621.GA7121@google.com","threadId":"50006","inReplyTo":"20181211104342.GA7233@sigill.intra.peff.net","subject":"Re: [PATCH 1/3] serve: pass \"config context\" through to individual commands","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-12-14T08:36:21Z","receivedAt":"2018-12-14T08:36:26Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJeff King wrote:\n\n> In protocol v2, instead of just running \"upload-pack\", we have a generic\n> \"serve\" loop which runs command requests from the client. What used to\n> be \"upload-pack\" is now generally split into two operations: \"ls-refs\"\n> and \"fetch\". The latter knows it must respect uploadpack.* config, but\n> the former is not actually specific to a fetch operation (we do not yet\n> do v2 receive-pack, but eventually we may, and ls-refs would support\n> both operations).\n\nI think I'm missing something. Why wouldn't \"ls-refs for push\" not pass\nthe information that it's for push as part of the *body* of the ls-refs\nrequest?\n\n(That's a separate issue from whether we need to have ls-refs for push\nat all, as opposed to specifying a policy for the requested ref\nupdates and getting a list of \"have\"s without ref names attached. But\nthat's a discussion for another day.)\n\nIs there some other more immediate motivation for this patch? In the\nspirit of YAGNI, I would rather understand that motivation instead of\none that in many possible designs would never materialize.\n\nThanks,\nJonathan\n"},{"id":"365333","messageId":"20181214085507.GD11777@sigill.intra.peff.net","threadId":"50006","inReplyTo":"20181214083621.GA7121@google.com","subject":"Re: [PATCH 1/3] serve: pass \"config context\" through to individual commands","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-12-14T08:55:07Z","receivedAt":"2018-12-14T08:55:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 14, 2018 at 12:36:21AM -0800, Jonathan Nieder wrote:\n\n> Jeff King wrote:\n> \n> > In protocol v2, instead of just running \"upload-pack\", we have a generic\n> > \"serve\" loop which runs command requests from the client. What used to\n> > be \"upload-pack\" is now generally split into two operations: \"ls-refs\"\n> > and \"fetch\". The latter knows it must respect uploadpack.* config, but\n> > the former is not actually specific to a fetch operation (we do not yet\n> > do v2 receive-pack, but eventually we may, and ls-refs would support\n> > both operations).\n> \n> I think I'm missing something. Why wouldn't \"ls-refs for push\" not pass\n> the information that it's for push as part of the *body* of the ls-refs\n> request?\n\nI don't know. Why doesn't the current \"ls-refs\" say \"ls-refs for fetch\"?\n\nCertainly if that information was carried from the client request it\nwould work fine, and ls-refs would have enough to know which config to\nrespect. But I could not find any documentation on this, nor discussion\nof plans for a v2 push. Since that information isn't passed now, we'd\nhave to assume that \"ls-refs\" without \"for-push\" always means \"for\nfetch\".\n\nI'm conceptually OK with that, but if that is the plan for going\nforward, it was not at all obvious to me (and it does feel rather\nimplicit).\n\n> Is there some other more immediate motivation for this patch? In the\n> spirit of YAGNI, I would rather understand that motivation instead of\n> one that in many possible designs would never materialize.\n\nWithout this information, in patch 3 ls-refs cannot know to look at\nuploadpack.hiderefs, unless it makes the implicit assumption that it is\nalways serving a fetch.\n\n-Peff\n"},{"id":"365334","messageId":"20181214092820.GB7121@google.com","threadId":"50006","inReplyTo":"20181214085507.GD11777@sigill.intra.peff.net","subject":"Re: [PATCH 1/3] serve: pass \"config context\" through to individual commands","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-12-14T09:28:20Z","receivedAt":"2018-12-14T09:28:26Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n> On Fri, Dec 14, 2018 at 12:36:21AM -0800, Jonathan Nieder wrote:\n>> Jeff King wrote:\n\n>>> In protocol v2, instead of just running \"upload-pack\", we have a generic\n>>> \"serve\" loop which runs command requests from the client. What used to\n>>> be \"upload-pack\" is now generally split into two operations: \"ls-refs\"\n>>> and \"fetch\". The latter knows it must respect uploadpack.* config, but\n>>> the former is not actually specific to a fetch operation (we do not yet\n>>> do v2 receive-pack, but eventually we may, and ls-refs would support\n>>> both operations).\n>>\n>> I think I'm missing something. Why wouldn't \"ls-refs for push\" not pass\n>> the information that it's for push as part of the *body* of the ls-refs\n>> request?\n>\n> I don't know. Why doesn't the current \"ls-refs\" say \"ls-refs for fetch\"?\n\nAlso YAGNI. ;-)\n\nIn other words, since the design for push isn't set in stone yet, we had\nnothing to be consistent with. And if there's an option to ls-ref to\nindicate whether it's for fetch or for push, then it can default to\nfetch.\n\nAs an aside, my experience from teaching people about Git protocol is\nthat the concept of \"ls-remote for push\" producing a different result\nfrom \"git ls-remote\" is very confusing. Given what it is used for, I am\nmotivated to replace it with something more tailored.\n\n> Certainly if that information was carried from the client request it\n> would work fine, and ls-refs would have enough to know which config to\n> respect. But I could not find any documentation on this, nor discussion\n> of plans for a v2 push.\n\nInteresting. The last discussion of push v2 plans was in\nhttps://public-inbox.org/git/20180717210915.139521-1-bmwill@google.com/.\nExpect to hear more soon.\n\n> Since that information isn't passed now, we'd\n> have to assume that \"ls-refs\" without \"for-push\" always means \"for\n> fetch\".\n>\n> I'm conceptually OK with that, but if that is the plan for going\n> forward, it was not at all obvious to me (and it does feel rather\n> implicit).\n\nDon't get me wrong: I haven't wrapped my head around config context\nand how it fits into the broader picture yet, but it may be a very\ngood thing to have. So please consider this comment to be about the\ncommit message only.\n\nBased on the motivation you're describing here, I think treating it as\nuploadpack and adding a NEEDSWORK comment would be a good way forward.\nIf we're moving toward a world with more protocol commands that don't\nfit in the upload-pack / receive-pack categories, then we need to\nfigure out in more detail what that world looks like:\n\n- do we keep on adding new endpoints, in the same spirit as\n upload-archive? If so, what endpoint should a client use to get\n capabilities before it decides which endpoint to use?\n\n- do we merge everything in \"git serve\" except where a specific\n endpoint is needed for protocol v0 compatibility? That would lose\n the ability to distinguish fetches from pushes without looking at\n the body of requests (which is useful to some people for monitoring,\n blocking, etc) --- do we consider that to be an acceptable loss?\n\n- once we've decided what the future should look like, how does the\n transition to that future look?\n\n>> Is there some other more immediate motivation for this patch? In the\n>> spirit of YAGNI, I would rather understand that motivation instead of\n>> one that in many possible designs would never materialize.\n>\n> Without this information, in patch 3 ls-refs cannot know to look at\n> uploadpack.hiderefs, unless it makes the implicit assumption that it is\n> always serving a fetch.\n\nI think that's a reasonable assumption to make, especially if made\nexplicit using a simple comment. :)\n\nThanks for explaining,\nJonathan\n"},{"id":"365335","messageId":"20181214095512.GA13465@sigill.intra.peff.net","threadId":"50006","inReplyTo":"20181214092820.GB7121@google.com","subject":"Re: [PATCH 1/3] serve: pass \"config context\" through to individual commands","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-12-14T09:55:12Z","receivedAt":"2018-12-14T09:55:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 14, 2018 at 01:28:20AM -0800, Jonathan Nieder wrote:\n\n> > Certainly if that information was carried from the client request it\n> > would work fine, and ls-refs would have enough to know which config to\n> > respect. But I could not find any documentation on this, nor discussion\n> > of plans for a v2 push.\n> \n> Interesting. The last discussion of push v2 plans was in\n> https://public-inbox.org/git/20180717210915.139521-1-bmwill@google.com/.\n> Expect to hear more soon.\n\nThe words \"ls-refs\" and \"advertisement\" are notably absent from that\nthread. ;)\n\n> > I'm conceptually OK with that, but if that is the plan for going\n> > forward, it was not at all obvious to me (and it does feel rather\n> > implicit).\n> \n> Don't get me wrong: I haven't wrapped my head around config context\n> and how it fits into the broader picture yet, but it may be a very\n> good thing to have. So please consider this comment to be about the\n> commit message only.\n\nIf we're OK with accepting that the client will pass along the\nfetch/push context for each individual command, then I don't think we\nwould ever need this. It is literally about relaying the fact of \"the\noriginal request was via upload-pack\". If the commands already know the\ncontext the client is interested in from another method, then I don't\nthink they should ever need to care about that fact.\n\n> Based on the motivation you're describing here, I think treating it as\n> uploadpack and adding a NEEDSWORK comment would be a good way forward.\n> If we're moving toward a world with more protocol commands that don't\n> fit in the upload-pack / receive-pack categories, then we need to\n> figure out in more detail what that world looks like:\n> \n> - do we keep on adding new endpoints, in the same spirit as\n> upload-archive? If so, what endpoint should a client use to get\n> capabilities before it decides which endpoint to use?\n> \n> - do we merge everything in \"git serve\" except where a specific\n> endpoint is needed for protocol v0 compatibility? That would lose\n> the ability to distinguish fetches from pushes without looking at\n> the body of requests (which is useful to some people for monitoring,\n> blocking, etc) --- do we consider that to be an acceptable loss?\n> \n> - once we've decided what the future should look like, how does the\n> transition to that future look?\n\nI agree those are all interesting open questions. I didn't want to solve\nany of them now, but just fix this (IMHO pretty serious) regression. I\nwas mostly trying to do so without making any assumptions about where\nwe'd go in the future (and even NEEDSWORK feels a little funny; it's not\nclear to me whether that work is going to be needed or not).\n\n> > Without this information, in patch 3 ls-refs cannot know to look at\n> > uploadpack.hiderefs, unless it makes the implicit assumption that it is\n> > always serving a fetch.\n> \n> I think that's a reasonable assumption to make, especially if made\n> explicit using a simple comment. :)\n\nThe big danger is that somebody does implement a \"push\" command and\nforgets to touch ls-refs. That would be wrong and buggy for more reasons\nthan this (as you noted earlier, it should handle .have refs somehow).\nBut what worries me is that the failure mode for the bug is to start\nexposing refs which are meant to be hidden. Which to me is a little more\nserious than \"the new functionality doesn't work\".\n\nSo I guess I considered it to mostly be defensive (and I'd be fine if it\nwas later ripped out when a more elegant approach becomes obvious).\n\nThat said, I'm not totally opposed to the implicit thing if that's where\nwe all think the protocol code should be headed. The patch is certainly\nsmaller. The whole series could be replaced with this:\n\n-- >8 --\nSubject: [PATCH] upload-pack: support hidden refs with protocol v2\n\nIn the v2 protocol, upload-pack's advertisement has been moved to the\n\"ls-refs\" command. That command does not respect hidden-ref config (like\ntransfer.hiderefs) at all, and advertises everything.\n\nWhile there are some features that are not supported in v2 (e.g., v2\nalways allows fetching any sha1 without respect to advertisements), the\nlack of this feature is not documented and is likely just a bug. Let's\nmake it work, as otherwise upgrading a server to a v2-capable git will\nstart exposing these refs that the repository admin has asked to remain\nhidden.\n\nNote that we assume we're operating on behalf of a fetch here, since\nthat's the only thing implemented in v2 at this point. See the in-code\ncomment.\n\nSigned-off-by: Jeff King \n---\n ls-refs.c | 16 ++++++++++++++++\n t/t5512-ls-remote.sh | 6 ++++++\n 2 files changed, 22 insertions(+)\n\ndiff --git a/ls-refs.c b/ls-refs.c\nindex a06f12eca8..9c9a7c647f 100644\n--- a/ls-refs.c\n+++ b/ls-refs.c\n@@ -5,6 +5,7 @@\n #include \"argv-array.h\"\n #include \"ls-refs.h\"\n #include \"pkt-line.h\"\n+#include \"config.h\"\n \n /*\n * Check if one of the prefixes is a prefix of the ref.\n@@ -40,6 +41,9 @@ static int send_ref(const char *refname, const struct object_id *oid,\n \tconst char *refname_nons = strip_namespace(refname);\n \tstruct strbuf refline = STRBUF_INIT;\n \n+\tif (ref_is_hidden(refname_nons, refname))\n+\t\treturn 0;\n+\n \tif (!ref_match(&data->prefixes, refname))\n \t\treturn 0;\n \n@@ -69,6 +73,16 @@ static int send_ref(const char *refname, const struct object_id *oid,\n \treturn 0;\n }\n \n+static int ls_refs_config(const char *var, const char *value, void *data)\n+{\n+\t/*\n+\t * We only serve fetches over v2 for now, so respect only \"uploadpack\"\n+\t * config. This may need to eventually be expanded to \"receive\", but we\n+\t * don't yet know how that information will be passed to ls-refs.\n+\t */\n+\treturn parse_hide_refs_config(var, value, \"uploadpack\");\n+}\n+\n int ls_refs(struct repository *r, struct argv_array *keys,\n \t struct packet_reader *request)\n {\n@@ -76,6 +90,8 @@ int ls_refs(struct repository *r, struct argv_array *keys,\n \n \tmemset(&data, 0, sizeof(data));\n \n+\tgit_config(ls_refs_config, NULL);\n+\n \twhile (packet_reader_read(request) != PACKET_READ_FLUSH) {\n \t\tconst char *arg = request->line;\n \t\tconst char *out;\ndiff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh\nindex 32e722db2e..ca69636fd5 100755\n--- a/t/t5512-ls-remote.sh\n+++ b/t/t5512-ls-remote.sh\n@@ -204,6 +204,12 @@ test_expect_success 'overrides work between mixed transfer/upload-pack hideRefs'\n \tgrep refs/tags/magic actual\n '\n \n+test_expect_success 'protocol v2 supports hiderefs' '\n+\ttest_config uploadpack.hiderefs refs/tags &&\n+\tgit -c protocol.version=2 ls-remote . >actual &&\n+\t! grep refs/tags actual\n+'\n+\n test_expect_success 'ls-remote --symref' '\n \tgit fetch origin &&\n \tcat >expect <<-EOF &&\n-- \n2.20.0.738.gdb22cab611\n\n"},{"id":"365337","messageId":"20181214101232.GC13465@sigill.intra.peff.net","threadId":"50006","inReplyTo":"87pnu51kac.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-12-14T10:12:32Z","receivedAt":"2018-12-14T10:12:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 13, 2018 at 05:08:43PM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> Now that we have this maybe we should discuss why these tests show\n> different things:\n> \n> > diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\n> > index 086f2c40f6..8b1217ea26 100755\n> > --- a/t/t5500-fetch-pack.sh\n> > +++ b/t/t5500-fetch-pack.sh\n> > @@ -628,7 +628,10 @@ test_expect_success 'fetch-pack cannot fetch a raw sha1 that is not advertised a\n> > \ttest_commit -C server 6 &&\n> >\n> > \tgit init client &&\n> > -\ttest_must_fail git -C client fetch-pack ../server \\\n> > +\n> > +\t# Other protocol versions (e.g. 2) allow fetching an unadvertised\n> > +\t# object, so run this test with the default protocol version (0).\n> > +\ttest_must_fail env GIT_TEST_PROTOCOL_VERSION= git -C client fetch-pack ../server \\\n> > \t\t$(git -C server rev-parse refs/heads/master^) 2>err &&\n> \n> What? So the equivalent of uploadpack.allowAnySHA1InWant=true is on for\n> v2 all the time?\n\nYeah, I actually didn't realize it until working on the earlier series,\nbut this is the documented behavior:\n\n $ git grep -hC3 'want ' Documentation/technical/protocol-v2.txt\n \n A `fetch` request can take the following arguments:\n \n want \n \tIndicates to the server an object which the client wants to\n \tretrieve. Wants can be anything and are not limited to\n \tadvertised objects.\n\nAn interesting implication of this at GitHub (and possibly other\nhosters) is that it exposes objects from shared storage via unexpected\nrepos. If I fork torvalds/linux to peff/linux and push up object X, a v0\nfetch will (by default) refuse to serve it to me. But v2 will happily\nhand it over, which may confuse people into thinking that the object is\n(or at least at some point was) in Linus's repository.\n\n-Peff\n"},{"id":"365339","messageId":"20181214101708.GD13465@sigill.intra.peff.net","threadId":"50006","inReplyTo":"20181213155817.27666-8-avarab@gmail.com","subject":"Re: [PATCH v2 7/8] builtin/fetch-pack: support protocol version 2","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-12-14T10:17:08Z","receivedAt":"2018-12-14T10:17:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 13, 2018 at 04:58:16PM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> From: Jonathan Tan \n> \n> Currently, if support for running Git's entire test suite with protocol\n> v2 were added, some tests would fail because the fetch-pack CLI command\n> dies if it encounters protocol v2. To avoid this, teach fetch-pack\n> support for protocol v2.\n\nI'm definitely on board with this goal.\n\n> @@ -219,9 +220,11 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n> \t\t\t PACKET_READ_CHOMP_NEWLINE |\n> \t\t\t PACKET_READ_GENTLE_ON_EOF);\n> \n> -\tswitch (discover_version(&reader)) {\n> +\tversion = discover_version(&reader);\n> +\tswitch (version) {\n> \tcase protocol_v2:\n> -\t\tdie(\"support for protocol v2 not implemented yet\");\n> +\t\tget_remote_refs(fd[1], &reader, &ref, 0, NULL, NULL);\n> +\t\tbreak;\n> \tcase protocol_v1:\n> \tcase protocol_v0:\n> \t\tget_remote_heads(&reader, &ref, 0, NULL, &shallow);\n> @@ -231,7 +234,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n> \t}\n> \n> \tref = fetch_pack(&args, fd, conn, ref, dest, sought, nr_sought,\n> -\t\t\t &shallow, pack_lockfile_ptr, protocol_v0);\n> +\t\t\t &shallow, pack_lockfile_ptr, version);\n\nThis implementation looks absurdly simple. So simple that it makes me\nwonder why we did not just do this in the first place. I.e., is there\nsome hidden gotcha blocking it, or was it simply that all of the\nnecessary work happened in the meantime and nobody bothered to flip the\nswitch?\n\n-Peff\n"},{"id":"365342","messageId":"87o99o1iot.fsf@evledraar.gmail.com","threadId":"50006","inReplyTo":"20181214101232.GC13465@sigill.intra.peff.net","subject":"Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-12-14T10:55:30Z","receivedAt":"2018-12-14T10:55:35Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Dec 14 2018, Jeff King wrote:\n\n> On Thu, Dec 13, 2018 at 05:08:43PM +0100, Ævar Arnfjörð Bjarmason wrote:\n>\n>> Now that we have this maybe we should discuss why these tests show\n>> different things:\n>>\n>> > diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\n>> > index 086f2c40f6..8b1217ea26 100755\n>> > --- a/t/t5500-fetch-pack.sh\n>> > +++ b/t/t5500-fetch-pack.sh\n>> > @@ -628,7 +628,10 @@ test_expect_success 'fetch-pack cannot fetch a raw sha1 that is not advertised a\n>> > \ttest_commit -C server 6 &&\n>> >\n>> > \tgit init client &&\n>> > -\ttest_must_fail git -C client fetch-pack ../server \\\n>> > +\n>> > +\t# Other protocol versions (e.g. 2) allow fetching an unadvertised\n>> > +\t# object, so run this test with the default protocol version (0).\n>> > +\ttest_must_fail env GIT_TEST_PROTOCOL_VERSION= git -C client fetch-pack ../server \\\n>> > \t\t$(git -C server rev-parse refs/heads/master^) 2>err &&\n>>\n>> What? So the equivalent of uploadpack.allowAnySHA1InWant=true is on for\n>> v2 all the time?\n>\n> Yeah, I actually didn't realize it until working on the earlier series,\n> but this is the documented behavior:\n>\n> $ git grep -hC3 'want ' Documentation/technical/protocol-v2.txt\n>\n> A `fetch` request can take the following arguments:\n>\n> want \n> \tIndicates to the server an object which the client wants to\n> \tretrieve. Wants can be anything and are not limited to\n> \tadvertised objects.\n>\n> An interesting implication of this at GitHub (and possibly other\n> hosters) is that it exposes objects from shared storage via unexpected\n> repos. If I fork torvalds/linux to peff/linux and push up object X, a v0\n> fetch will (by default) refuse to serve it to me. But v2 will happily\n> hand it over, which may confuse people into thinking that the object is\n> (or at least at some point was) in Linus's repository.\n\nMore importantly this bypasses the security guarantee we've had with the\ndefault of uploadpack.allowAnySHA1InWant=false.\n\nIf I push a id_rsa to a topic branch on $githost and immediately realize\nmy mistake and delete it, someone with access to a log of SHA-1s\n(e.g. an IRC bot spewing out from/to commits) won't be able to pull it\ndown. This is why we have that as a setting in the first place\n(f8edeaa05d (\"upload-pack: optionally allow fetching any sha1\",\n2016-11-11)).\n\nOf course this is:\n\n a) Subject to the \"SECURITY\" section of git-fetch, i.e. maybe this\n doesn't even work in the face of a determined attacker.\n\n b) Hosting sites like GitHub, GitLab, Gitweb etc. aren't doing\n reachability checks when you view /commit/. If I delete my topic\n branch it'll be viewable until the next GC kicks in (which would\n also be the case with uploadpack.allowAnySHA1InWant=true).\n\nSo I wonder how much we need to care about this in practice, but for\nsome configurations protocol v2 definitely breaks a security promise\nwe've been making, now whether that promise was always BS due to \"a)\"\nabove is another matter.\n\nI'm inclined to say that in the face of that \"SECURITY\" section we\nshould just:\n\n * Turn on uploadpack.allowReachableSHA1InWant for v0/v1 by\n default. Make saying uploadpack.allowReachableSHA1InWant=false warn\n with \"this won't work, see SECURITY...\".\n\n * The uploadpack.allowTipSHA1InWant setting will also be turned on by\n default, and will be much faster, since it'll just degrade to\n uploadpack.allowReachableSHA1InWant=true and we won't need any\n reachability check. We'll also warn saying that setting it is\n useless.\n\nOtherwise what's th point of these settings given what we're talking\nabout in that \"SECURITY\" section? It seems to just lure users into a\nfalse sense of security. Better to not make promises we're not confident\nwe can keep, and say that if you push your id_rsa you need to run a\ngc/prune on the server.\n"},{"id":"365343","messageId":"87mup81i3x.fsf@evledraar.gmail.com","threadId":"50006","inReplyTo":"87o99o1iot.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-12-14T11:08:02Z","receivedAt":"2018-12-14T11:08:07Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Dec 14 2018, Ævar Arnfjörð Bjarmason wrote:\n\n> On Fri, Dec 14 2018, Jeff King wrote:\n>\n>> On Thu, Dec 13, 2018 at 05:08:43PM +0100, Ævar Arnfjörð Bjarmason wrote:\n>>\n>>> Now that we have this maybe we should discuss why these tests show\n>>> different things:\n>>>\n>>> > diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\n>>> > index 086f2c40f6..8b1217ea26 100755\n>>> > --- a/t/t5500-fetch-pack.sh\n>>> > +++ b/t/t5500-fetch-pack.sh\n>>> > @@ -628,7 +628,10 @@ test_expect_success 'fetch-pack cannot fetch a raw sha1 that is not advertised a\n>>> > \ttest_commit -C server 6 &&\n>>> >\n>>> > \tgit init client &&\n>>> > -\ttest_must_fail git -C client fetch-pack ../server \\\n>>> > +\n>>> > +\t# Other protocol versions (e.g. 2) allow fetching an unadvertised\n>>> > +\t# object, so run this test with the default protocol version (0).\n>>> > +\ttest_must_fail env GIT_TEST_PROTOCOL_VERSION= git -C client fetch-pack ../server \\\n>>> > \t\t$(git -C server rev-parse refs/heads/master^) 2>err &&\n>>>\n>>> What? So the equivalent of uploadpack.allowAnySHA1InWant=true is on for\n>>> v2 all the time?\n>>\n>> Yeah, I actually didn't realize it until working on the earlier series,\n>> but this is the documented behavior:\n>>\n>> $ git grep -hC3 'want ' Documentation/technical/protocol-v2.txt\n>>\n>> A `fetch` request can take the following arguments:\n>>\n>> want \n>> \tIndicates to the server an object which the client wants to\n>> \tretrieve. Wants can be anything and are not limited to\n>> \tadvertised objects.\n>>\n>> An interesting implication of this at GitHub (and possibly other\n>> hosters) is that it exposes objects from shared storage via unexpected\n>> repos. If I fork torvalds/linux to peff/linux and push up object X, a v0\n>> fetch will (by default) refuse to serve it to me. But v2 will happily\n>> hand it over, which may confuse people into thinking that the object is\n>> (or at least at some point was) in Linus's repository.\n>\n> More importantly this bypasses the security guarantee we've had with the\n> default of uploadpack.allowAnySHA1InWant=false.\n>\n> If I push a id_rsa to a topic branch on $githost and immediately realize\n> my mistake and delete it, someone with access to a log of SHA-1s\n> (e.g. an IRC bot spewing out from/to commits) won't be able to pull it\n> down. This is why we have that as a setting in the first place\n> (f8edeaa05d (\"upload-pack: optionally allow fetching any sha1\",\n> 2016-11-11)).\n>\n> Of course this is:\n>\n> a) Subject to the \"SECURITY\" section of git-fetch, i.e. maybe this\n> doesn't even work in the face of a determined attacker.\n>\n> b) Hosting sites like GitHub, GitLab, Gitweb etc. aren't doing\n> reachability checks when you view /commit/. If I delete my topic\n> branch it'll be viewable until the next GC kicks in (which would\n> also be the case with uploadpack.allowAnySHA1InWant=true).\n>\n> So I wonder how much we need to care about this in practice, but for\n> some configurations protocol v2 definitely breaks a security promise\n> we've been making, now whether that promise was always BS due to \"a)\"\n> above is another matter.\n>\n> I'm inclined to say that in the face of that \"SECURITY\" section we\n> should just:\n>\n> * Turn on uploadpack.allowReachableSHA1InWant for v0/v1 by\n> default. Make saying uploadpack.allowReachableSHA1InWant=false warn\n> with \"this won't work, see SECURITY...\".\n>\n> * The uploadpack.allowTipSHA1InWant setting will also be turned on by\n> default, and will be much faster, since it'll just degrade to\n> uploadpack.allowReachableSHA1InWant=true and we won't need any\n> reachability check. We'll also warn saying that setting it is\n> useless.\n>\n> Otherwise what's th point of these settings given what we're talking\n> about in that \"SECURITY\" section? It seems to just lure users into a\n> false sense of security. Better to not make promises we're not confident\n> we can keep, and say that if you push your id_rsa you need to run a\n> gc/prune on the server.\n\nI sent that too fast. What I meant is that there's 3\nsettings. uploadpack.{allowTipSHA1InWant,allowReachableSHA1InWant,allowAnySHA1InWant}. Those\nare, respectively, cheap/expensive/cheap to serve up.\n\nWe could make them cheap/cheap/cheap by just making the first two a\nsynonym for the 3rd (allowAnySHA1InWant), because as noted above\n\nAlso, parts of the v2 code, e.g. this in 685fbd3291 (\"fetch-pack:\nperform a fetch using v2\", 2018-03-15):\n\n\t/* v2 supports these by default */\n\tallow_unadvertised_object_request |= ALLOW_REACHABLE_SHA1;\n\nSeem to have intended to turn on allowReachableSHA1InWant, not\nallowAnySHA1InWant, but apparently that's what we're doing?\n\nIn any case, regardless of what v2 intended there anything except having\nallowAnySHA1InWant on by default seems irresponsible given x) us\ndocumenting in \"SECURITY\" that it doesn't really work y) even if it did,\nthere being a *tiny* minority of people who'd in some way care about\nthis feature who don't also run some sort of web UI on the git server\nthat'll be bypassing this setting no matter if it worked as intended for\nthe wire protocol.\n"},{"id":"365421","messageId":"xmqq5zvvd418.fsf@gitster-ct.c.googlers.com","threadId":"50006","inReplyTo":"20181214082050.GA11777@sigill.intra.peff.net","subject":"Re: [PATCH 1/3] serve: pass \"config context\" through to individual commands","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-15T00:31:15Z","receivedAt":"2018-12-15T00:31:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I actually started by doing that, but \"struct serve_options\" is not\n> currently known by ls-refs.c, upload-pack.c, etc. So they'd have to\n> start including \"serve.h\". I don't think that's the end of the world,\n> but it felt like a funny mutual circular to me (my mental model now is\n> that ls-refs, upload-pack, etc are low-level commands, tied together by\n> the \"serve\" stuff).\n\nThat matches my mental model, too. I think the difference between\nus is what *_opt struct is. I viewed that it was like diff_options\nstruct where the driving machinery keeps state of the ongoing\noperation performed by lower level routines to fulfill the request\nby the API caller, i.e. holding both wish from the caller, and\nscratchpad data for the mchinery and the lower level routine to\ncommunicate with each other.\n\nAnd the new field feels like the last \"scratchpad used by the\nmachinery to tell lower-level ls-refs helper what context it is\noperting in\".\n\nI dunno.\n"},{"id":"365435","messageId":"87d0q21s8w.fsf@evledraar.gmail.com","threadId":"50006","inReplyTo":"20181214083507.GB11777@sigill.intra.peff.net","subject":"Re: [PATCH 0/3] protocol v2 and hidden refs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-12-15T19:53:35Z","receivedAt":"2018-12-15T19:53:41Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Dec 14 2018, Jeff King wrote:\n\n> On Thu, Dec 13, 2018 at 11:53:05AM -0800, Jonathan Tan wrote:\n>\n>> > I don't know if there's a good solution. I tried running the whole\n>> > test suite with v2 as the default. It does find this bug, but it has\n>> > a bunch of other problems (notably fetch-pack won't run as v2, but\n>> > some other tests I think also depend on v0's reachability rules,\n>> > which v2 is documented not to enforce).\n>>\n>> I think Aevar's patches (sent after you wrote this) is a good start, and\n>> I have started looking at it too.\n>\n> Yeah, I'm excited to see it working with fetch-pack, as the current\n> behavior is to complain if you've tried to enable v2 config:\n>\n> $ git config protocol.version 2\n> $ git fetch-pack git://github.com/git/git\n> fatal: support for protocol v2 not implemented yet\n>\n> I haven't actually run into it in the real world, but somebody might if\n> they have scripted around fetch-pack and are experimenting with v2. A\n> much friendlier behavior would be falling back to v1, but actually\n> supporting v2 is better still. :)\n>\n>> > - The \"serve\" command is funky, because it has no concept of whether\n>> > the \"ls-refs\" is for fetching or pushing. Is git-serve even a thing\n>> > that we want to support going forward? I know part of the original\n>> > v2 conception was that one would be able to just connect to\n>> > \"git-serve\" and do a number of operations. But in practice the v2\n>> > probing requires saying \"I'd like to git-upload-pack, and v2 if you\n>> > please\". So no client ever calls git-serve.\n>> >\n>> > Is this something we plan to eventually move to? Or can it be\n>> > considered a funny vestige of the development? In the latter case, I\n>> > think we should consider removing it.\n>>\n>> Personally, I lean towards removing it, but there are arguments on both\n>> sides. In particular, removing \"serve\" means that both developers and\n>> users of Git need not be concerned with a 3rd endpoint, but preserving\n>> \"serve\" (and planning to migrate away from \"upload-pack\" and\n>> \"receive-pack\") means that we will only have one endpoint, eliminating\n>> confusion about which endpoint to use when making certain requests (when\n>> we add requests other than \"fetch\" and \"push\").\n>\n> Yeah, at first glance I like the simplicity of a unified model. But the\n> separate fetch/push endpoints have been useful in the past. Separate\n> uploadpack/receive.hiderefs that I dealt with here are one form. Another\n> is that many people do HTTP access control using the endpoints. For\n> example, if I have a repo which is public-read and private-write, the\n> config we recommend in git-http-backend(1) is to lock down the\n> receive-pack access using webserver config.\n>\n> If all the webserver sees is \"somebody wants to connect to git-serve\",\n> it doesn't know if it should be challenging them for authentication or\n> not. It would have to start peeking into the git-serve conversation to\n> see what the client actually wants to do. That's _possible_ to do, but\n> it gets pretty awkward with existing webserver tools (whereas matching\n> the URI endpoint is pretty easy).\n>\n> Ditto for locked down ssh sessions like git-shell (or custom solutions\n> like gitolite). Right now we can \"git-upload-pack is OK on this repo,\n> git-receive-pack is not\". But blindly running \"git serve\" would be\n> dangerous. In this case I think we have a few more options, because the\n> user has always already authenticated. So we can just tell \"git serve\"\n> via the environment whether the user is authorized for push. It's harder\n> with HTTP because most setups avoid even challenging for auth unless\n> it's necessary.\n>\n> So I'm a bit worried that the unified endpoint model is going to be a\n> dead end, at which point carrying around git-serve just makes things\n> more complicated.\n\nThis is from wetware memory of something discussed at a previous Git\nMerge, so I may have (inadvertently) made it up, but wasn't part of the\nidea of \"git-serve\" to have an extensible next-gen protocol where we\ncould add new actions & verbs unrelated to sending or receiving packs?\n\nE.g. client<->server optimistic cooperation like offloading a\nlong-running \"git-grep\", \"git log -G\" etc. to a more powerful workhorse\nserver, which would use \"git-serve\" as a routing layer.\n\nOf course that's not in itself an argument for having a general \"serve\"\ncommand, actually the opposite for the reasons you mention with locking\ndown things. E.g. maybe I want to support server-side git-grep on my\nserver, but not git-log, and if it's one command it becomes a hassle to\ndo that via SSH config or HTTPD config for the reasons you mention.\n\nThe upside would be that once a host understands \"git serve\" I'm more\nlikely to be able to get past whatever middle layer there is between my\nclient and the \"git\" binary on the other side. E.g. if I have a newly\ncompiled \"git\" client/server binary, but something like GitLab's\n\"gitaly\" sitting between the two of us.\n"},{"id":"365442","messageId":"20181216102544.GA13704@sigill.intra.peff.net","threadId":"50006","inReplyTo":"xmqq5zvvd418.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/3] serve: pass \"config context\" through to individual commands","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-12-16T10:25:45Z","receivedAt":"2018-12-16T10:25:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Dec 15, 2018 at 09:31:15AM +0900, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > I actually started by doing that, but \"struct serve_options\" is not\n> > currently known by ls-refs.c, upload-pack.c, etc. So they'd have to\n> > start including \"serve.h\". I don't think that's the end of the world,\n> > but it felt like a funny mutual circular to me (my mental model now is\n> > that ls-refs, upload-pack, etc are low-level commands, tied together by\n> > the \"serve\" stuff).\n> \n> That matches my mental model, too. I think the difference between\n> us is what *_opt struct is. I viewed that it was like diff_options\n> struct where the driving machinery keeps state of the ongoing\n> operation performed by lower level routines to fulfill the request\n> by the API caller, i.e. holding both wish from the caller, and\n> scratchpad data for the mchinery and the lower level routine to\n> communicate with each other.\n> \n> And the new field feels like the last \"scratchpad used by the\n> machinery to tell lower-level ls-refs helper what context it is\n> operting in\".\n\nYeah, I agree that such a \"pass this through\" struct full of options and\ncontext would make sense. I just wouldn't tie it to the \"serve\"\nmachinery.\n\nDid you read the side-thread between me and Jonathan? Another option\nhere is to just have ls-refs assume that the client will tell it the\ncontext (and assume uploadpack for now, since that's all that v2\ncurrently does).\n\nThat would make this patch go away entirely. :)\n\n-Peff\n"},{"id":"365443","messageId":"20181216104017.GB13704@sigill.intra.peff.net","threadId":"50006","inReplyTo":"87d0q21s8w.fsf@evledraar.gmail.com","subject":"Re: [PATCH 0/3] protocol v2 and hidden refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-12-16T10:40:17Z","receivedAt":"2018-12-16T10:40:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Dec 15, 2018 at 08:53:35PM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> > So I'm a bit worried that the unified endpoint model is going to be a\n> > dead end, at which point carrying around git-serve just makes things\n> > more complicated.\n> \n> This is from wetware memory of something discussed at a previous Git\n> Merge, so I may have (inadvertently) made it up, but wasn't part of the\n> idea of \"git-serve\" to have an extensible next-gen protocol where we\n> could add new actions & verbs unrelated to sending or receiving packs?\n\nYes, I think that's a goal (and we already have upload-archive, which is\na similar thing).\n\n> Of course that's not in itself an argument for having a general \"serve\"\n> command, actually the opposite for the reasons you mention with locking\n> down things. E.g. maybe I want to support server-side git-grep on my\n> server, but not git-log, and if it's one command it becomes a hassle to\n> do that via SSH config or HTTPD config for the reasons you mention.\n\nRight, exactly. It pushes more of the information down into Git's own\nprotocol. Of course we _can_ build mechanisms at that level for\nconfiguring which verbs are allowed. But if some context is available at\nthe higher protocol level, then we can use the mechanisms at that higher\nlevel.\n\nI think of it as a tradeoff. By including the endpoint in the transport\nprotocol (e.g., in ssh the command name, in HTTP the URL), we get to use\nthe mechanisms in those transports to make policy decisions on the\nserver. But it also means we _have_ to implement those policies twice,\nonce per transport.\n\nIMHO having to deal with both transports is not that big a loss,\nconsidering that there are only two, and really not likely to be more.\ngit:// is already unauthenticated, and IMHO is mostly a dead-end for\nfuture protocol work since it provides no advantage over HTTP, and the\nfuture is mostly HTTP, with ssh for people who really prefer its\nauthentication mode.\n\n> The upside would be that once a host understands \"git serve\" I'm more\n> likely to be able to get past whatever middle layer there is between my\n> client and the \"git\" binary on the other side. E.g. if I have a newly\n> compiled \"git\" client/server binary, but something like GitLab's\n> \"gitaly\" sitting between the two of us.\n\nBut I think that's what makes it dangerous, too. :)\n\nGitaly (and we have our own equivalent at GitHub) is responsible for\nmaking those policy decisions about who can run what. Opening a pipe\nbetween the client and the backend that can issue arbitrary verbs is\nexactly what they _don't_ want to do.\n\nSo they have to intercept the conversation at least at the verb level.\nIt _is_ nice if conversation for each verb is standardized (so once a\nverb is issued, they can just step out of the way and proxy bytes[1]),\nand v2 helps with that.\n\nThat's not too hard for a Git-aware endpoint to implement. But when\nthat verb interception can be done at the HTTP/ssh level, then it's easy\nfor tools that _aren't_ Git-aware to do handle it (again, like the\nApache config we recommend in git-http-backend(1)).\n\n-Peff\n\n[1] Actually, we do much more intimate interception than that at GitHub\n already. The upload-pack conversation is mostly vanilla, but for\n receive-pack we handle replication at that layer. So your pack is\n streamed to 3-6 backend receive-packs simultaneously, and that\n endpoint layer handles quorum for updating refs, etc.\n"},{"id":"365449","messageId":"xmqqo99lbu9o.fsf@gitster-ct.c.googlers.com","threadId":"50006","inReplyTo":"20181216102544.GA13704@sigill.intra.peff.net","subject":"Re: [PATCH 1/3] serve: pass \"config context\" through to individual commands","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-16T11:12:03Z","receivedAt":"2018-12-16T11:12:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Yeah, I agree that such a \"pass this through\" struct full of options and\n> context would make sense. I just wouldn't tie it to the \"serve\"\n> machinery.\n>\n> Did you read the side-thread between me and Jonathan? Another option\n> here is to just have ls-refs assume that the client will tell it the\n> context (and assume uploadpack for now, since that's all that v2\n> currently does).\n\nYes, I'd be 100% happy with that, too. And it certainly is simpler.\n\nThanks.\n\nP.S. I expect to be mostly offline for the coming 72 hours, as I and\nmy wife are both down with a cold. I am guessing that we will enter\nslower weeks in many parts of the world, and hoping this won't be\ntoo disruptive.\n\n\n"},{"id":"365450","messageId":"87bm5l1ymo.fsf@evledraar.gmail.com","threadId":"50006","inReplyTo":"20181216104017.GB13704@sigill.intra.peff.net","subject":"Re: [PATCH 0/3] protocol v2 and hidden refs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-12-16T11:47:59Z","receivedAt":"2018-12-16T11:48:07Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Dec 16 2018, Jeff King wrote:\n\n> On Sat, Dec 15, 2018 at 08:53:35PM +0100, Ævar Arnfjörð Bjarmason wrote:\n>\n>> > So I'm a bit worried that the unified endpoint model is going to be a\n>> > dead end, at which point carrying around git-serve just makes things\n>> > more complicated.\n>>\n>> This is from wetware memory of something discussed at a previous Git\n>> Merge, so I may have (inadvertently) made it up, but wasn't part of the\n>> idea of \"git-serve\" to have an extensible next-gen protocol where we\n>> could add new actions & verbs unrelated to sending or receiving packs?\n>\n> Yes, I think that's a goal (and we already have upload-archive, which is\n> a similar thing).\n>\n>> Of course that's not in itself an argument for having a general \"serve\"\n>> command, actually the opposite for the reasons you mention with locking\n>> down things. E.g. maybe I want to support server-side git-grep on my\n>> server, but not git-log, and if it's one command it becomes a hassle to\n>> do that via SSH config or HTTPD config for the reasons you mention.\n>\n> Right, exactly. It pushes more of the information down into Git's own\n> protocol. Of course we _can_ build mechanisms at that level for\n> configuring which verbs are allowed. But if some context is available at\n> the higher protocol level, then we can use the mechanisms at that higher\n> level.\n>\n> I think of it as a tradeoff. By including the endpoint in the transport\n> protocol (e.g., in ssh the command name, in HTTP the URL), we get to use\n> the mechanisms in those transports to make policy decisions on the\n> server. But it also means we _have_ to implement those policies twice,\n> once per transport.\n>\n> IMHO having to deal with both transports is not that big a loss,\n> considering that there are only two, and really not likely to be more.\n> git:// is already unauthenticated, and IMHO is mostly a dead-end for\n> future protocol work since it provides no advantage over HTTP, and the\n> future is mostly HTTP, with ssh for people who really prefer its\n> authentication mode.\n>\n>> The upside would be that once a host understands \"git serve\" I'm more\n>> likely to be able to get past whatever middle layer there is between my\n>> client and the \"git\" binary on the other side. E.g. if I have a newly\n>> compiled \"git\" client/server binary, but something like GitLab's\n>> \"gitaly\" sitting between the two of us.\n>\n> But I think that's what makes it dangerous, too. :)\n>\n> Gitaly (and we have our own equivalent at GitHub) is responsible for\n> making those policy decisions about who can run what. Opening a pipe\n> between the client and the backend that can issue arbitrary verbs is\n> exactly what they _don't_ want to do.\n>\n> So they have to intercept the conversation at least at the verb level.\n> It _is_ nice if conversation for each verb is standardized (so once a\n> verb is issued, they can just step out of the way and proxy bytes[1]),\n> and v2 helps with that.\n>\n> That's not too hard for a Git-aware endpoint to implement. But when\n> that verb interception can be done at the HTTP/ssh level, then it's easy\n> for tools that _aren't_ Git-aware to do handle it (again, like the\n> Apache config we recommend in git-http-backend(1)).\n>\n> -Peff\n>\n> [1] Actually, we do much more intimate interception than that at GitHub\n> already. The upload-pack conversation is mostly vanilla, but for\n> receive-pack we handle replication at that layer. So your pack is\n> streamed to 3-6 backend receive-packs simultaneously, and that\n> endpoint layer handles quorum for updating refs, etc.\n\nYeah I think overall this makes sense. I was just thinking we'd have\nstuff like this needing to be maintained in all middleware:\nhttps://gitlab.com/gitlab-org/gitlab-shell/blob/v8.4.4/lib/gitlab_shell.rb#L15-28\n\nWhich, if and when we have a lot of verbs would be a pain, but of course\nserver operators might want to explicitly whitelist them.\n\nAlso for things like \"git-grep\" I can see e.g. \"no POSIX regex\" (due to\nwell known DoS issues0 being a configuration we ourselves would want to\ncarry, at that point server operators would need to maintain two\nwhitelists anyway, one in their custom code & another in /etc/gitconfig.\n\nBut I think that trade-off is worth it as you note because when you want\nto filter these it's handy to be able to do it in a dumb ssh/web server.\n\nAnother thing to consider is not having a proliferation of things in\ngit- completion again. AFAIK these things can't have spaces in them\nfor /etc/passwd & inetd tab completion. So perhaps call them all\ngit-serve-*, or not put them in our bin/ install path as a special case?\n"},{"id":"365492","messageId":"20181217195713.GA10673@sigill.intra.peff.net","threadId":"50006","inReplyTo":"87o99o1iot.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-12-17T19:57:13Z","receivedAt":"2018-12-17T19:57:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 14, 2018 at 11:55:30AM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> > An interesting implication of this at GitHub (and possibly other\n> > hosters) is that it exposes objects from shared storage via unexpected\n> > repos. If I fork torvalds/linux to peff/linux and push up object X, a v0\n> > fetch will (by default) refuse to serve it to me. But v2 will happily\n> > hand it over, which may confuse people into thinking that the object is\n> > (or at least at some point was) in Linus's repository.\n> \n> More importantly this bypasses the security guarantee we've had with the\n> default of uploadpack.allowAnySHA1InWant=false.\n\nIMHO those security guarantees there are overrated (due to delta\nguessing attacks, though things are not quite as bad if the attacker\ncan't actually push to the repo).\n\nBut I agree that people do assume it's the case. I was certainly\nsurprised by the v2 behavior, and I don't remember that aspect being\ndiscussed.\n\n(I suspect it was mostly because in the stateless world, the \"fetch\"\noperation requires iterating all of the refs again. That's made even\nharder by the fact that \"ls-refs\" takes arguments now, and might have\nonly advertised a subset of the refs. How can \"fetch\" know which ones\nwere advertised?).\n\n> a) Subject to the \"SECURITY\" section of git-fetch, i.e. maybe this\n> doesn't even work in the face of a determined attacker.\n\nYeah, this is the part I was thinking about above.\n\n> b) Hosting sites like GitHub, GitLab, Gitweb etc. aren't doing\n> reachability checks when you view /commit/. If I delete my topic\n> branch it'll be viewable until the next GC kicks in (which would\n> also be the case with uploadpack.allowAnySHA1InWant=true).\n\nActually, we don't ever prune unless a user asks us to (and certainly\nusers do ask us to after accidentally pushing secret stuff). In GitHub's\ncase, though, we always tell people to consider anything pushed to a\npublic repo to be non-secret, even if it was exposed for only a few\nseconds. My understanding is that there are literally clients watching\nthe public feeds and sucking down repo data looking for these kinds of\nmistakes.\n\nThe /commit/ thing has been a frequent topic of discussion within\nGitHub over the years, and we have looked at actually doing reachability\nchecks. They're expensive, though bitmaps help.\n\n> So I wonder how much we need to care about this in practice, but for\n> some configurations protocol v2 definitely breaks a security promise\n> we've been making, now whether that promise was always BS due to \"a)\"\n> above is another matter.\n> \n> I'm inclined to say that in the face of that \"SECURITY\" section we\n> should just:\n> \n> * Turn on uploadpack.allowReachableSHA1InWant for v0/v1 by\n> default. Make saying uploadpack.allowReachableSHA1InWant=false warn\n> with \"this won't work, see SECURITY...\".\n> \n> * The uploadpack.allowTipSHA1InWant setting will also be turned on by\n> default, and will be much faster, since it'll just degrade to\n> uploadpack.allowReachableSHA1InWant=true and we won't need any\n> reachability check. We'll also warn saying that setting it is\n> useless.\n\nNo real argument from me. I have always thought those security\nguarantees were BS.\n\nHowever, I do think that's all orthogonal to the issue I was mentioning,\nwhich is that it looks like repo A wants to serve you object X, even if\nthat repo never saw the object. That's unique to shared storage schemes,\nthough.\n\nIMHO this is also a user education issue (it's a question of trust: refs\nare the source of trust, and objects don't need to be trusted because\nthey're immutable). But it's pretty subtle for people who don't know the\ninner workings of Git.\n\n-Peff\n"},{"id":"365493","messageId":"20181217195919.GB10673@sigill.intra.peff.net","threadId":"50006","inReplyTo":"87mup81i3x.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-12-17T19:59:19Z","receivedAt":"2018-12-17T19:59:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 14, 2018 at 12:08:02PM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> Also, parts of the v2 code, e.g. this in 685fbd3291 (\"fetch-pack:\n> perform a fetch using v2\", 2018-03-15):\n> \n> \t/* v2 supports these by default */\n> \tallow_unadvertised_object_request |= ALLOW_REACHABLE_SHA1;\n> \n> Seem to have intended to turn on allowReachableSHA1InWant, not\n> allowAnySHA1InWant, but apparently that's what we're doing?\n\nThat I don't know. Maybe Brandon can answer what the intent was.\n\nI agree that turning on the reachability check would make it behavior\nmore or less the same as v0 (at some additional cost to walk the refs\nagain, but v0 smart-http already had to do that).\n\n-Peff\n"},{"id":"365500","messageId":"20181217221625.1523-1-avarab@gmail.com","threadId":"50006","inReplyTo":"20181217195713.GA10673@sigill.intra.peff.net","subject":"[PATCH] upload-pack: turn on uploadpack.allowAnySHA1InWant=true","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-12-17T22:16:25Z","receivedAt":"2018-12-17T22:16:42Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Unconditionally turn on uploadpack.allowAnySHA1InWant=true which means\nthat the lesser uploadpack.allow{Tip,Reachable}SHA1InWant settings are\nimplicitly \"true\" as well.\n\nThis is because as mentioned in 235ec24352e (\"doc: mention transfer\ndata leaks in more places\", 2016-11-14) this has always been leaky,\nand thus lulls users into a false sense of security. Better to not\nmake promises we're not confident we can keep.\n\nNow setting any of uploadpack.allow{Tip,Reachable,Any}SHA1InWant will\nwarn, to e.g. point users who've explicitly set it to \"false\" to the\ndocumentation. They're probably relying on behavior that was always\nbuggy. We may have users who trusted it to be \"false\" by default, but\nwarning if it's set seems like a good trade-off.\n\nThis change also has the effect of making any operation that needed\nuploadpack.allowReachableSHA1InWant=true much faster, since we won't\nneed any reachability check.\n\nThis change doesn't do any of the hard work of extracting the now-dead\nALLOW_* code out of upload-pack.c and fetch-pack.c. Most of that was\nchanged or added in the following commits:\n\n- 9f9fb57063a (\"upload-pack: turn on uploadpack.allowAnySHA1InWant=true\", 2018-12-17)\n- 6e7b66eebd1 (\"fetch: fetch objects by their exact SHA-1 object names\", 2013-01-29)\n- 7199c093ad4 (\"upload-pack: prepare to extend allow-tip-sha1-in-want\", 2015-05-21)\n- 68ee628932c (\"upload-pack: optionally allow fetching reachable sha1\", 2015-05-21)\n- f8edeaa05d8 (\"upload-pack: optionally allow fetching any sha1\", 2016-11-11)\n\nThat cleanup step can be done later. Starting with a change to switch\nthe default (and only) value and tweaking the docs makes it easier to\nreason about this than fully ripping out the code right away.\n\nSee https://public-inbox.org/git/87pnu51kac.fsf@evledraar.gmail.com/\nand related messages for further discussion.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/config/uploadpack.txt | 48 +++++++++++++++----------\n Documentation/transfer-data-leaks.txt | 8 +++++\n t/t0410-partial-clone.sh | 2 --\n t/t1090-sparse-checkout-scope.sh | 1 -\n t/t5500-fetch-pack.sh | 10 +++---\n t/t5512-ls-remote.sh | 14 ++++++++\n t/t5516-fetch-push.sh | 50 +++++++++++++--------------\n t/t5551-http-fetch-smart.sh | 4 ---\n t/t5601-clone.sh | 3 --\n t/t5616-partial-clone.sh | 8 +----\n t/t5702-protocol-v2.sh | 1 -\n t/t7406-submodule-update.sh | 5 +--\n upload-pack.c | 18 +++-------\n 13 files changed, 87 insertions(+), 85 deletions(-)\n\ndiff --git a/Documentation/config/uploadpack.txt b/Documentation/config/uploadpack.txt\nindex ed1c835695c..854801bd6cc 100644\n--- a/Documentation/config/uploadpack.txt\n+++ b/Documentation/config/uploadpack.txt\n@@ -1,31 +1,41 @@\n uploadpack.hideRefs::\n \tThis variable is the same as `transfer.hideRefs`, but applies\n \tonly to `upload-pack` (and so affects only fetches, not pushes).\n-\tAn attempt to fetch a hidden ref by `git fetch` will fail. See\n-\talso `uploadpack.allowTipSHA1InWant`.\n++\n+This can be used in some setups to reduce the size of the ref\n+advertisement, see also `protocol.version` for using a protocol\n+version that allows them to be filtered by the client.\n++\n+In earlier versions of Git attempts to fetch an object pointed to by a\n+hidden ref would fail, this is no longer the case. See\n+`uploadpack.allowAnySHA1InWant`.\n \n uploadpack.allowTipSHA1InWant::\n-\tWhen `uploadpack.hideRefs` is in effect, allow `upload-pack`\n-\tto accept a fetch request that asks for an object at the tip\n-\tof a hidden ref (by default, such a request is rejected).\n-\tSee also `uploadpack.hideRefs`. Even if this is false, a client\n-\tmay be able to steal objects via the techniques described in the\n-\t\"SECURITY\" section of the linkgit:gitnamespaces[7] man page; it's\n-\tbest to keep private data in a separate repository.\n+\tDepreacted. Used to allow `upload-pack` to accept a fetch\n+\trequest that asked for an object at the tip of a hidden\n+\tref. Now always `true`. See `uploadpack.allowAnySHA1InWant`\n+\tbelow for details.\n \n uploadpack.allowReachableSHA1InWant::\n-\tAllow `upload-pack` to accept a fetch request that asks for an\n-\tobject that is reachable from any ref tip. However, note that\n-\tcalculating object reachability is computationally expensive.\n-\tDefaults to `false`. Even if this is false, a client may be able\n-\tto steal objects via the techniques described in the \"SECURITY\"\n-\tsection of the linkgit:gitnamespaces[7] man page; it's best to\n-\tkeep private data in a separate repository.\n+\tDepreacted. Used to allow `upload-pack` to accept a fetch\n+\trequest that asked for an object that was reachable from any\n+\tref tip. Now always `true`. See\n+\t`uploadpack.allowAnySHA1InWant` below for details.\n \n uploadpack.allowAnySHA1InWant::\n-\tAllow `upload-pack` to accept a fetch request that asks for any\n-\tobject at all.\n-\tDefaults to `false`.\n+\tDeprecated. Before Git version 2.21 the\n+\t`uploadpack.allow{Tip,Any,Reachable}SHA1InWant` settings were\n+\t`false` by default. Now they're unconditionally `true`, which\n+\tmeans fetch request that ask for any object in the object\n+\tstore will be accepted, regardless of whether such an object\n+\tis at the tip of a ref, reachable by no ref etc.\n++\n+The reason for the change in default is as noted in the \"SECURITY\"\n+section of linkgit:git-fetch[1] a determined client can get to any\n+previously forbidden objects the server has access to by manipulating\n+the client/server dialog. If you host objects that should be kept\n+secret from some clients it's best to keep them in a separate\n+repository.\n \n uploadpack.keepAlive::\n \tWhen `upload-pack` has started `pack-objects`, there may be a\ndiff --git a/Documentation/transfer-data-leaks.txt b/Documentation/transfer-data-leaks.txt\nindex 914bacc39e0..f8beb334b84 100644\n--- a/Documentation/transfer-data-leaks.txt\n+++ b/Documentation/transfer-data-leaks.txt\n@@ -28,3 +28,11 @@ The known attack vectors are as follows:\n an object Y that the attacker already has, and the attacker falsely\n claims to have X and not Y, so the victim sends Y as a delta against X.\n The delta reveals regions of X that are similar to Y to the attacker.\n+\n+Before Git version 2.21 the\n+`uploadpack.allow{Tip,Any,Reachable}SHA1InWant` allowed for tweaking\n+linkgit:git-upload-pack[1] behavior to refuse to serve some refs. As\n+noted above this served to lull users into a false sense of security,\n+and the fetch protocol will now allow clients to fetch any object in\n+the ref store. See `uploadpack.allowAnySHA1InWant` in\n+linkgit:git-config[1].\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex ba3887f178b..a9d24a94d9f 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -192,7 +192,6 @@ test_expect_success 'fetching of missing blobs works' '\n \tgit hash-object repo/foo.t >blobhash &&\n \trm -rf repo/.git/objects/* &&\n \n-\tgit -C server config uploadpack.allowanysha1inwant 1 &&\n \tgit -C server config uploadpack.allowfilter 1 &&\n \tgit -C repo config core.repositoryformatversion 1 &&\n \tgit -C repo config extensions.partialclone \"origin\" &&\n@@ -211,7 +210,6 @@ test_expect_success 'fetching of missing trees does not fetch blobs' '\n \tgit hash-object repo/foo.t >blobhash &&\n \trm -rf repo/.git/objects/* &&\n \n-\tgit -C server config uploadpack.allowanysha1inwant 1 &&\n \tgit -C server config uploadpack.allowfilter 1 &&\n \tgit -C repo config core.repositoryformatversion 1 &&\n \tgit -C repo config extensions.partialclone \"origin\" &&\ndiff --git a/t/t1090-sparse-checkout-scope.sh b/t/t1090-sparse-checkout-scope.sh\nindex 090b7fc3d35..4de2e2dd1bb 100755\n--- a/t/t1090-sparse-checkout-scope.sh\n+++ b/t/t1090-sparse-checkout-scope.sh\n@@ -68,7 +68,6 @@ test_expect_success 'in partial clone, sparse checkout only fetches needed blobs\n \tgit clone \"file://$(pwd)/server\" client &&\n \n \ttest_config -C server uploadpack.allowfilter 1 &&\n-\ttest_config -C server uploadpack.allowanysha1inwant 1 &&\n \techo a >server/a &&\n \techo bb >server/b &&\n \tmkdir server/c &&\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex 086f2c40f68..b639629863d 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -592,8 +592,7 @@ test_expect_success 'fetch-pack can fetch a raw sha1' '\n \t\ttest_commit 1 &&\n \t\ttest_commit 2 &&\n \t\tgit update-ref refs/hidden/one HEAD^ &&\n-\t\tgit config transfer.hiderefs refs/hidden &&\n-\t\tgit config uploadpack.allowtipsha1inwant true\n+\t\tgit config transfer.hiderefs refs/hidden\n \t) &&\n \tgit fetch-pack hidden $(git -C hidden rev-parse refs/hidden/one)\n '\n@@ -619,7 +618,7 @@ test_expect_success 'fetch-pack can fetch a raw sha1 overlapping a named ref' '\n \t\t$(git -C server rev-parse refs/tags/1) refs/tags/1\n '\n \n-test_expect_success 'fetch-pack cannot fetch a raw sha1 that is not advertised as a ref' '\n+test_expect_success 'fetch-pack can fetch a raw sha1 that is not advertised as a ref' '\n \trm -rf server &&\n \n \tgit init server &&\n@@ -628,9 +627,8 @@ test_expect_success 'fetch-pack cannot fetch a raw sha1 that is not advertised a\n \ttest_commit -C server 6 &&\n \n \tgit init client &&\n-\ttest_must_fail git -C client fetch-pack ../server \\\n-\t\t$(git -C server rev-parse refs/heads/master^) 2>err &&\n-\ttest_i18ngrep \"Server does not allow request for unadvertised object\" err\n+\tgit -C client fetch-pack ../server \\\n+\t\t$(git -C server rev-parse refs/heads/master^)\n '\n \n check_prot_path () {\ndiff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh\nindex 32e722db2ed..0cd1aad9597 100755\n--- a/t/t5512-ls-remote.sh\n+++ b/t/t5512-ls-remote.sh\n@@ -195,6 +195,20 @@ do\n \t\t! grep refs/tags/magic/one actual\n \t'\n \n+\n+\ttest_expect_success \"fetching a divergent object hidden by $configsection.hiderefs\" \"\n+\t\ttest_when_finished 'test_unconfig $configsection.hiderefs' &&\n+\t\tgit config --add $configsection.hiderefs refs/tags &&\n+\t\ttest_when_finished 'git tag -d magic/divergent' &&\n+\t\tgit tag -a -m'a tag' magic/divergent &&\n+\t\tSHA1_TAG=\\$(git rev-parse magic/divergent) &&\n+\t\techo \\$SHA1_TAG >expect &&\n+\t\tgit init client &&\n+\t\ttest_when_finished 'rm -r client' &&\n+\t\tgit -C client fetch ../ \\$SHA1_TAG:refs/tags/magic/divergent &&\n+\t\tgit -C client rev-parse magic/divergent >actual &&\n+\t\ttest_cmp expect actual\n+\t\"\n done\n \n test_expect_success 'overrides work between mixed transfer/upload-pack hideRefs' '\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 37e8e80893d..8f2b12c17a5 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -1147,26 +1147,14 @@ test_expect_success 'fetch exact SHA1' '\n \t\tgit prune &&\n \t\ttest_must_fail git cat-file -t $the_commit &&\n \n-\t\t# fetching the hidden object should fail by default\n-\t\ttest_must_fail git fetch -v ../testrepo $the_commit:refs/heads/copy 2>err &&\n-\t\ttest_i18ngrep \"Server does not allow request for unadvertised object\" err &&\n-\t\ttest_must_fail git rev-parse --verify refs/heads/copy &&\n+\t\t# fetching the object should work\n+\t\tgit fetch -v ../testrepo $the_commit:refs/heads/copy &&\n+\t\tgit rev-parse --verify refs/heads/copy &&\n \n-\t\t# the server side can allow it to succeed\n-\t\t(\n-\t\t\tcd ../testrepo &&\n-\t\t\tgit config uploadpack.allowtipsha1inwant true\n-\t\t) &&\n-\n-\t\tgit fetch -v ../testrepo $the_commit:refs/heads/copy master:refs/heads/extra &&\n \t\tcat >expect <<-EOF &&\n \t\t$the_commit\n-\t\t$the_first_commit\n \t\tEOF\n-\t\t{\n-\t\t\tgit rev-parse --verify refs/heads/copy &&\n-\t\t\tgit rev-parse --verify refs/heads/extra\n-\t\t} >actual &&\n+\t\tgit rev-parse --verify refs/heads/copy >actual &&\n \t\ttest_cmp expect actual\n \t)\n '\n@@ -1190,6 +1178,25 @@ test_expect_success 'fetch exact SHA1 in protocol v2' '\n \tgit -C child fetch -v ../testrepo $the_commit:refs/heads/copy\n '\n \n+for state in true false\n+do\n+\ttest_expect_success \"Deprecated uploadpack.*=$state settings warn\" \"\n+\t\tmk_empty deprecated-warnings &&\n+\t\ttest_commit -C deprecated-warnings A &&\n+\t\ttest_config -C deprecated-warnings uploadpack.allowTipSHA1InWant $state &&\n+\t\ttest_config -C deprecated-warnings uploadpack.allowReachableSHA1InWant $state &&\n+\t\ttest_config -C deprecated-warnings uploadpack.allowAnySHA1InWant $state &&\n+\t\tmk_empty target &&\n+\t\t(\n+\t\t\tcd target &&\n+\t\t\tgit fetch ../deprecated-warnings/.git refs/tags/A:refs/tags/A 2>stderr &&\n+\t\t\ttest_i18ngrep 'warning: .*uploadpack.allowTipSHA1InWant .*deprecated' stderr &&\n+\t\t\ttest_i18ngrep 'warning: .*uploadpack.allowReachableSHA1InWant.* deprecated' stderr &&\n+\t\t\ttest_i18ngrep 'warning: .*uploadpack.allowAnySHA1InWant.* deprecated' stderr\n+\t\t)\n+\t\"\n+done\n+\n for configallowtipsha1inwant in true false\n do\n \ttest_expect_success \"shallow fetch reachable SHA1 (but not a ref), allowtipsha1inwant=$configallowtipsha1inwant\" '\n@@ -1204,8 +1211,6 @@ do\n \t\tmk_empty shallow &&\n \t\t(\n \t\t\tcd shallow &&\n-\t\t\ttest_must_fail git fetch --depth=1 ../testrepo/.git $SHA1 &&\n-\t\t\tgit --git-dir=../testrepo/.git config uploadpack.allowreachablesha1inwant true &&\n \t\t\tgit fetch --depth=1 ../testrepo/.git $SHA1 &&\n \t\t\tgit cat-file commit $SHA1\n \t\t)\n@@ -1232,15 +1237,10 @@ do\n \t\tmk_empty shallow &&\n \t\t(\n \t\t\tcd shallow &&\n-\t\t\ttest_must_fail ok=sigpipe git fetch ../testrepo/.git $SHA1_3 &&\n-\t\t\ttest_must_fail ok=sigpipe git fetch ../testrepo/.git $SHA1_1 &&\n-\t\t\tgit --git-dir=../testrepo/.git config uploadpack.allowreachablesha1inwant true &&\n+\t\t\tgit fetch ../testrepo/.git $SHA1_3 &&\n \t\t\tgit fetch ../testrepo/.git $SHA1_1 &&\n \t\t\tgit cat-file commit $SHA1_1 &&\n-\t\t\ttest_must_fail git cat-file commit $SHA1_2 &&\n-\t\t\tgit fetch ../testrepo/.git $SHA1_2 &&\n-\t\t\tgit cat-file commit $SHA1_2 &&\n-\t\t\ttest_must_fail ok=sigpipe git fetch ../testrepo/.git $SHA1_3\n+\t\t\tgit cat-file commit $SHA1_2\n \t\t)\n \t'\n done\ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex 8630b0cc390..b8f7c0b5563 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -283,7 +283,6 @@ test_expect_success 'test allowreachablesha1inwant' '\n \ttest_when_finished \"rm -rf test_reachable.git\" &&\n \tserver=\"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&\n \tmaster_sha=$(git -C \"$server\" rev-parse refs/heads/master) &&\n-\tgit -C \"$server\" config uploadpack.allowreachablesha1inwant 1 &&\n \n \tgit init --bare test_reachable.git &&\n \tgit -C test_reachable.git remote add origin \"$HTTPD_URL/smart/repo.git\" &&\n@@ -302,7 +301,6 @@ test_expect_success 'test allowreachablesha1inwant with unreachable' '\n \n \tserver=\"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&\n \tmaster_sha=$(git -C \"$server\" rev-parse refs/heads/master) &&\n-\tgit -C \"$server\" config uploadpack.allowreachablesha1inwant 1 &&\n \n \tgit init --bare test_reachable.git &&\n \tgit -C test_reachable.git remote add origin \"$HTTPD_URL/smart/repo.git\" &&\n@@ -321,13 +319,11 @@ test_expect_success 'test allowanysha1inwant with unreachable' '\n \n \tserver=\"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&\n \tmaster_sha=$(git -C \"$server\" rev-parse refs/heads/master) &&\n-\tgit -C \"$server\" config uploadpack.allowreachablesha1inwant 1 &&\n \n \tgit init --bare test_reachable.git &&\n \tgit -C test_reachable.git remote add origin \"$HTTPD_URL/smart/repo.git\" &&\n \ttest_must_fail git -C test_reachable.git fetch origin \"$(git rev-parse HEAD)\" &&\n \n-\tgit -C \"$server\" config uploadpack.allowanysha1inwant 1 &&\n \tgit -C test_reachable.git fetch origin \"$(git rev-parse HEAD)\"\n '\n \ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex 8bbc7068acb..ec7a3c46528 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -646,7 +646,6 @@ partial_clone () {\n \ttest_commit -C \"$SERVER\" two &&\n \tHASH2=$(git hash-object \"$SERVER/two.t\") &&\n \ttest_config -C \"$SERVER\" uploadpack.allowfilter 1 &&\n-\ttest_config -C \"$SERVER\" uploadpack.allowanysha1inwant 1 &&\n \n \tgit clone --filter=blob:limit=0 \"$URL\" client &&\n \n@@ -689,7 +688,6 @@ test_expect_success 'batch missing blob request during checkout' '\n \tgit -C server commit -m x &&\n \n \ttest_config -C server uploadpack.allowfilter 1 &&\n-\ttest_config -C server uploadpack.allowanysha1inwant 1 &&\n \n \tgit clone --filter=blob:limit=0 \"file://$(pwd)/server\" client &&\n \n@@ -720,7 +718,6 @@ test_expect_success 'batch missing blob request does not inadvertently try to fe\n \tgit -C server commit -m x &&\n \n \ttest_config -C server uploadpack.allowfilter 1 &&\n-\ttest_config -C server uploadpack.allowanysha1inwant 1 &&\n \n \t# Make sure that it succeeds\n \tgit clone --filter=blob:limit=0 \"file://$(pwd)/server\" client\ndiff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\nindex 336f02a41a6..213899a690d 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -25,8 +25,7 @@ test_expect_success 'setup normal src repo' '\n # bare clone \"src\" giving \"srv.bare\" for use as our server.\n test_expect_success 'setup bare clone for server' '\n \tgit clone --bare \"file://$(pwd)/src\" srv.bare &&\n-\tgit -C srv.bare config --local uploadpack.allowfilter 1 &&\n-\tgit -C srv.bare config --local uploadpack.allowanysha1inwant 1\n+\tgit -C srv.bare config --local uploadpack.allowfilter 1\n '\n \n # do basic partial clone from \"srv.bare\"\n@@ -159,7 +158,6 @@ test_expect_success 'partial clone with transfer.fsckobjects=1 uses index-pack -\n \tgit init src &&\n \ttest_commit -C src x &&\n \ttest_config -C src uploadpack.allowfilter 1 &&\n-\ttest_config -C src uploadpack.allowanysha1inwant 1 &&\n \n \tGIT_TRACE=\"$(pwd)/trace\" git -c transfer.fsckobjects=1 \\\n \t\tclone --filter=\"blob:none\" \"file://$(pwd)/src\" dst &&\n@@ -213,7 +211,6 @@ test_expect_success 'partial clone fetches blobs pointed to by refs even if norm\n \tgit init src &&\n \ttest_commit -C src x &&\n \ttest_config -C src uploadpack.allowfilter 1 &&\n-\ttest_config -C src uploadpack.allowanysha1inwant 1 &&\n \n \t# Create a tag pointing to a blob.\n \tBLOB=$(echo blob-contents | git -C src hash-object --stdin -w) &&\n@@ -229,7 +226,6 @@ test_expect_success 'fetch what is specified on CLI even if already promised' '\n \tgit init src &&\n \ttest_commit -C src foo &&\n \ttest_config -C src uploadpack.allowfilter 1 &&\n-\ttest_config -C src uploadpack.allowanysha1inwant 1 &&\n \n \tgit hash-object --stdin blob &&\n \n@@ -257,7 +253,6 @@ test_expect_success 'upon cloning, check that all refs point to objects' '\n \ttest_create_repo \"$SERVER\" &&\n \ttest_commit -C \"$SERVER\" foo &&\n \ttest_config -C \"$SERVER\" uploadpack.allowfilter 1 &&\n-\ttest_config -C \"$SERVER\" uploadpack.allowanysha1inwant 1 &&\n \n \t# Create a tag pointing to a blob.\n \tBLOB=$(echo blob-contents | git -C \"$SERVER\" hash-object --stdin -w) &&\n@@ -293,7 +288,6 @@ test_expect_success 'when partial cloning, tolerate server not sending target of\n \ttest_create_repo \"$SERVER\" &&\n \ttest_commit -C \"$SERVER\" foo &&\n \ttest_config -C \"$SERVER\" uploadpack.allowfilter 1 &&\n-\ttest_config -C \"$SERVER\" uploadpack.allowanysha1inwant 1 &&\n \n \t# Create an annotated tag pointing to a blob.\n \tBLOB=$(echo blob-contents | git -C \"$SERVER\" hash-object --stdin -w) &&\ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex 0f2b09ebb8d..af16f139a4a 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -274,7 +274,6 @@ test_expect_success 'setup filter tests' '\n \ttest_commit -C server message2 a.txt &&\n \tgit -C server config protocol.version 2 &&\n \tgit -C server config uploadpack.allowfilter 1 &&\n-\tgit -C server config uploadpack.allowanysha1inwant 1 &&\n \tgit -C server config protocol.version 2\n '\n \ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex e87164aa8ff..cf5bda08690 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -943,10 +943,7 @@ test_expect_success 'submodule update clone shallow submodule outside of depth'\n \t\tcd super3 &&\n \t\tsed -e \"s#url = ../#url = file://$pwd/#\" <.gitmodules >.gitmodules.tmp &&\n \t\tmv -f .gitmodules.tmp .gitmodules &&\n-\t\ttest_must_fail git submodule update --init --depth=1 2>actual &&\n-\t\ttest_i18ngrep \"Direct fetching of that commit failed.\" actual &&\n-\t\tgit -C ../submodule config uploadpack.allowReachableSHA1InWant true &&\n-\t\tgit submodule update --init --depth=1 >actual &&\n+\t\tgit submodule update --init --depth=1 &&\n \t\tgit -C submodule log --oneline >out &&\n \t\ttest_line_count = 1 out\n \t)\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 5e81f1ff24f..18e4c2d2452 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -54,7 +54,8 @@ static int no_progress, daemon_mode;\n #define ALLOW_REACHABLE_SHA1\t02\n /* Allow request of any sha1. Implies ALLOW_TIP_SHA1 and ALLOW_REACHABLE_SHA1. */\n #define ALLOW_ANY_SHA1\t07\n-static unsigned int allow_unadvertised_object_request;\n+static unsigned int allow_unadvertised_object_request = (\n+\tALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1 | ALLOW_ANY_SHA1);\n static int shallow_nr;\n static struct object_array extra_edge_obj;\n static unsigned int timeout;\n@@ -1019,20 +1020,11 @@ static int find_symref(const char *refname, const struct object_id *oid,\n static int upload_pack_config(const char *var, const char *value, void *unused)\n {\n \tif (!strcmp(\"uploadpack.allowtipsha1inwant\", var)) {\n-\t\tif (git_config_bool(var, value))\n-\t\t\tallow_unadvertised_object_request |= ALLOW_TIP_SHA1;\n-\t\telse\n-\t\t\tallow_unadvertised_object_request &= ~ALLOW_TIP_SHA1;\n+\t\twarning(_(\"The uploadpack.allowTipSHA1InWant setting is deprecated, and now always 'true'\"));\n \t} else if (!strcmp(\"uploadpack.allowreachablesha1inwant\", var)) {\n-\t\tif (git_config_bool(var, value))\n-\t\t\tallow_unadvertised_object_request |= ALLOW_REACHABLE_SHA1;\n-\t\telse\n-\t\t\tallow_unadvertised_object_request &= ~ALLOW_REACHABLE_SHA1;\n+\t\twarning(_(\"The uploadpack.allowReachableSHA1InWant setting is deprecated, and now always 'true'\"));\n \t} else if (!strcmp(\"uploadpack.allowanysha1inwant\", var)) {\n-\t\tif (git_config_bool(var, value))\n-\t\t\tallow_unadvertised_object_request |= ALLOW_ANY_SHA1;\n-\t\telse\n-\t\t\tallow_unadvertised_object_request &= ~ALLOW_ANY_SHA1;\n+\t\twarning(_(\"The uploadpack.allowAnySHA1InWant setting is deprecated, and now always 'true'\"));\n \t} else if (!strcmp(\"uploadpack.keepalive\", var)) {\n \t\tkeepalive = git_config_int(var, value);\n \t\tif (!keepalive)\n-- \n2.20.0.405.gbc1bbc6f85\n\n"},{"id":"365501","messageId":"20181217224054.4376-1-avarab@gmail.com","threadId":"50006","inReplyTo":"20181213155817.27666-1-avarab@gmail.com","subject":"[PATCH v3 0/4] protocol v2 fixes","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-12-17T22:40:50Z","receivedAt":"2018-12-17T22:41:07Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The v2 of this series conflicted with Josh Steadmon's work when merged\nin \"pu\". That's still outstanding, see\nhttps://public-inbox.org/git/87h8ff20ol.fsf@evledraar.gmail.com/\n\nThen my just-sent\nhttps://public-inbox.org/git/20181217221625.1523-1-avarab@gmail.com/\nconflicts with even more things in it.\n\nSo I'm dropping \"GIT_TEST_PROTOCOL_VERSION\" for now until things\nsettle down. That can land after all this protocol activity settles.\n\nNo changes to Jeff's patches since v2, for Jonathan's no changes to\nthe C code, but I added a test which I extracted from the\nGIT_TEST_PROTOCOL_VERSION=2 work.\n\nJeff King (3):\n serve: pass \"config context\" through to individual commands\n parse_hide_refs_config: handle NULL section\n upload-pack: support hidden refs with protocol v2\n\nJonathan Tan (1):\n fetch-pack: support protocol version 2\n\n builtin/fetch-pack.c | 9 ++++++---\n builtin/upload-pack.c | 1 +\n ls-refs.c | 16 +++++++++++++++-\n ls-refs.h | 3 ++-\n refs.c | 3 ++-\n serve.c | 9 +++++----\n serve.h | 7 +++++++\n t/t5500-fetch-pack.sh | 22 +++++++++++++++-------\n t/t5512-ls-remote.sh | 6 ++++++\n upload-pack.c | 4 ++--\n upload-pack.h | 4 ++--\n 11 files changed, 63 insertions(+), 21 deletions(-)\n\n-- \n2.20.0.405.gbc1bbc6f85\n\n"},{"id":"365502","messageId":"20181217224054.4376-2-avarab@gmail.com","threadId":"50006","inReplyTo":"20181213155817.27666-1-avarab@gmail.com","subject":"[PATCH v3 1/4] serve: pass \"config context\" through to individual commands","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-12-17T22:40:51Z","receivedAt":"2018-12-17T22:41:09Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"From: Jeff King \n\nIn protocol v2, instead of just running \"upload-pack\", we have a generic\n\"serve\" loop which runs command requests from the client. What used to\nbe \"upload-pack\" is now generally split into two operations: \"ls-refs\"\nand \"fetch\". The latter knows it must respect uploadpack.* config, but\nthe former is not actually specific to a fetch operation (we do not yet\ndo v2 receive-pack, but eventually we may, and ls-refs would support\nboth operations).\n\nHowever, ls-refs does need to know which operation we're performing, in\norder to read the correct config (e.g., uploadpack.hiderefs versus\nreceive.hiderefs; we don't read _either_ right now, but this is the\nfirst step to fixing that).\n\nIn the generic \"git-serve\" program, we don't have that information. But\nin the protocol as it is actually used by clients, the client still asks\nto run \"git-upload-pack\", and then issues an \"ls-refs\" from there. So we\n_do_ know at that point that \"uploadpack\" is the right config context.\nThis patch teaches the protocol v2 \"serve\" code to pass that context\nthrough to the individual commands (a future patch will teach ls-refs to\nactually use it).\n\nSigned-off-by: Jeff King \nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/upload-pack.c | 1 +\n ls-refs.c | 4 +++-\n ls-refs.h | 3 ++-\n serve.c | 9 +++++----\n serve.h | 7 +++++++\n upload-pack.c | 4 ++--\n upload-pack.h | 4 ++--\n 7 files changed, 22 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/upload-pack.c b/builtin/upload-pack.c\nindex 42dc4da5a1f..67192cfa9e9 100644\n--- a/builtin/upload-pack.c\n+++ b/builtin/upload-pack.c\n@@ -52,6 +52,7 @@ int cmd_upload_pack(int argc, const char **argv, const char *prefix)\n \tcase protocol_v2:\n \t\tserve_opts.advertise_capabilities = opts.advertise_refs;\n \t\tserve_opts.stateless_rpc = opts.stateless_rpc;\n+\t\tserve_opts.config_context = \"uploadpack\";\n \t\tserve(&serve_opts);\n \t\tbreak;\n \tcase protocol_v1:\ndiff --git a/ls-refs.c b/ls-refs.c\nindex a06f12eca8d..e8e31475f06 100644\n--- a/ls-refs.c\n+++ b/ls-refs.c\n@@ -69,7 +69,9 @@ static int send_ref(const char *refname, const struct object_id *oid,\n \treturn 0;\n }\n \n-int ls_refs(struct repository *r, struct argv_array *keys,\n+int ls_refs(struct repository *r,\n+\t const char *config_section,\n+\t struct argv_array *keys,\n \t struct packet_reader *request)\n {\n \tstruct ls_refs_data data;\ndiff --git a/ls-refs.h b/ls-refs.h\nindex b62877e8dad..da26fc9824d 100644\n--- a/ls-refs.h\n+++ b/ls-refs.h\n@@ -4,7 +4,8 @@\n struct repository;\n struct argv_array;\n struct packet_reader;\n-extern int ls_refs(struct repository *r, struct argv_array *keys,\n+extern int ls_refs(struct repository *r, const char *config_context,\n+\t\t struct argv_array *keys,\n \t\t struct packet_reader *request);\n \n #endif /* LS_REFS_H */\ndiff --git a/serve.c b/serve.c\nindex bda085f09c8..70f89cf0d98 100644\n--- a/serve.c\n+++ b/serve.c\n@@ -48,6 +48,7 @@ struct protocol_capability {\n \t * This field should be NULL for capabilities which are not commands.\n \t */\n \tint (*command)(struct repository *r,\n+\t\t const char *config_context,\n \t\t struct argv_array *keys,\n \t\t struct packet_reader *request);\n };\n@@ -158,7 +159,7 @@ enum request_state {\n \tPROCESS_REQUEST_DONE,\n };\n \n-static int process_request(void)\n+static int process_request(struct serve_options *opts)\n {\n \tenum request_state state = PROCESS_REQUEST_KEYS;\n \tstruct packet_reader reader;\n@@ -222,7 +223,7 @@ static int process_request(void)\n \tif (!command)\n \t\tdie(\"no command requested\");\n \n-\tcommand->command(the_repository, &keys, &reader);\n+\tcommand->command(the_repository, opts->config_context, &keys, &reader);\n \n \targv_array_clear(&keys);\n \treturn 0;\n@@ -249,10 +250,10 @@ void serve(struct serve_options *options)\n \t * a single request/response exchange\n \t */\n \tif (options->stateless_rpc) {\n-\t\tprocess_request();\n+\t\tprocess_request(options);\n \t} else {\n \t\tfor (;;)\n-\t\t\tif (process_request())\n+\t\t\tif (process_request(options))\n \t\t\t\tbreak;\n \t}\n }\ndiff --git a/serve.h b/serve.h\nindex fe65ba9f469..d527224cbb1 100644\n--- a/serve.h\n+++ b/serve.h\n@@ -8,6 +8,13 @@ extern int has_capability(const struct argv_array *keys, const char *capability,\n struct serve_options {\n \tunsigned advertise_capabilities;\n \tunsigned stateless_rpc;\n+\n+\t/*\n+\t * Some operations may need to know the context when looking up config;\n+\t * e.g., set this to \"uploadpack\" to respect \"uploadpack.hiderefs\" (as\n+\t * opposed to \"receive.hiderefs\").\n+\t */\n+\tconst char *config_context;\n };\n #define SERVE_OPTIONS_INIT { 0 }\n extern void serve(struct serve_options *options);\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 5e81f1ff24f..914bbb40bcd 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -1413,8 +1413,8 @@ enum fetch_state {\n \tFETCH_DONE,\n };\n \n-int upload_pack_v2(struct repository *r, struct argv_array *keys,\n-\t\t struct packet_reader *request)\n+int upload_pack_v2(struct repository *r, const char *config_context,\n+\t\t struct argv_array *keys, struct packet_reader *request)\n {\n \tenum fetch_state state = FETCH_PROCESS_ARGS;\n \tstruct upload_pack_data data;\ndiff --git a/upload-pack.h b/upload-pack.h\nindex cab2178796a..2a9f51197c4 100644\n--- a/upload-pack.h\n+++ b/upload-pack.h\n@@ -13,8 +13,8 @@ void upload_pack(struct upload_pack_options *options);\n struct repository;\n struct argv_array;\n struct packet_reader;\n-extern int upload_pack_v2(struct repository *r, struct argv_array *keys,\n-\t\t\t struct packet_reader *request);\n+extern int upload_pack_v2(struct repository *r, const char *config_context,\n+\t\t\t struct argv_array *keys, struct packet_reader *request);\n \n struct strbuf;\n extern int upload_pack_advertise(struct repository *r,\n-- \n2.20.0.405.gbc1bbc6f85\n\n"},{"id":"365503","messageId":"20181217224054.4376-4-avarab@gmail.com","threadId":"50006","inReplyTo":"20181213155817.27666-1-avarab@gmail.com","subject":"[PATCH v3 3/4] upload-pack: support hidden refs with protocol v2","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-12-17T22:40:53Z","receivedAt":"2018-12-17T22:41:12Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"From: Jeff King \n\nIn the v2 protocol, upload-pack's advertisement has been moved to the\n\"ls-refs\" command. That command does not respect hidden-ref config (like\ntransfer.hiderefs) at all, and advertises everything.\n\nWhile there are some features that are not supported in v2 (e.g., v2\nalways allows fetching any sha1 without respect to advertisements), the\nlack of this feature is not documented and is likely just a bug. Let's\nmake it work, as otherwise upgrading a server to a v2-capable git will\nstart exposing these refs that the repository admin has asked to remain\nhidden.\n\nNote that we depend on the config_context from the caller here to\nrealize that we should respect uploadpack.hiderefs. When run from the\n\"git-serve\" tool, we won't have that context and will only respect\ntransfer.hiderefs. This should be OK, as git-serve is not actually used\nfor normal protocol operations.\n\nNote also that we don't need to worry about the \"An attempt to update\nor delete a hidden ref by git push is rejected\" feature of\nreceive.hideRefs, see git-config(1). This is because there's no v2\npush protocol yet.\n\nSigned-off-by: Jeff King \nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n ls-refs.c | 12 ++++++++++++\n t/t5512-ls-remote.sh | 6 ++++++\n 2 files changed, 18 insertions(+)\n\ndiff --git a/ls-refs.c b/ls-refs.c\nindex e8e31475f06..8a8143338b4 100644\n--- a/ls-refs.c\n+++ b/ls-refs.c\n@@ -5,6 +5,7 @@\n #include \"argv-array.h\"\n #include \"ls-refs.h\"\n #include \"pkt-line.h\"\n+#include \"config.h\"\n \n /*\n * Check if one of the prefixes is a prefix of the ref.\n@@ -40,6 +41,9 @@ static int send_ref(const char *refname, const struct object_id *oid,\n \tconst char *refname_nons = strip_namespace(refname);\n \tstruct strbuf refline = STRBUF_INIT;\n \n+\tif (ref_is_hidden(refname_nons, refname))\n+\t\treturn 0;\n+\n \tif (!ref_match(&data->prefixes, refname))\n \t\treturn 0;\n \n@@ -69,6 +73,12 @@ static int send_ref(const char *refname, const struct object_id *oid,\n \treturn 0;\n }\n \n+static int ls_refs_config(const char *var, const char *value,\n+\t\t\t void *config_context)\n+{\n+\treturn parse_hide_refs_config(var, value, config_context);\n+}\n+\n int ls_refs(struct repository *r,\n \t const char *config_section,\n \t struct argv_array *keys,\n@@ -78,6 +88,8 @@ int ls_refs(struct repository *r,\n \n \tmemset(&data, 0, sizeof(data));\n \n+\tgit_config(ls_refs_config, (void *)config_section);\n+\n \twhile (packet_reader_read(request) != PACKET_READ_FLUSH) {\n \t\tconst char *arg = request->line;\n \t\tconst char *out;\ndiff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh\nindex 32e722db2ed..ca69636fd52 100755\n--- a/t/t5512-ls-remote.sh\n+++ b/t/t5512-ls-remote.sh\n@@ -204,6 +204,12 @@ test_expect_success 'overrides work between mixed transfer/upload-pack hideRefs'\n \tgrep refs/tags/magic actual\n '\n \n+test_expect_success 'protocol v2 supports hiderefs' '\n+\ttest_config uploadpack.hiderefs refs/tags &&\n+\tgit -c protocol.version=2 ls-remote . >actual &&\n+\t! grep refs/tags actual\n+'\n+\n test_expect_success 'ls-remote --symref' '\n \tgit fetch origin &&\n \tcat >expect <<-EOF &&\n-- \n2.20.0.405.gbc1bbc6f85\n\n"},{"id":"365504","messageId":"20181217224054.4376-3-avarab@gmail.com","threadId":"50006","inReplyTo":"20181213155817.27666-1-avarab@gmail.com","subject":"[PATCH v3 2/4] parse_hide_refs_config: handle NULL section","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-12-17T22:40:52Z","receivedAt":"2018-12-17T22:41:13Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"From: Jeff King \n\nThis helper function looks for config in two places: transfer.hiderefs,\nor $section.hiderefs, where $section is passed in by the caller (and is\n\"uploadpack\" or \"receive\", depending on the context).\n\nIn preparation for callers which do not even have that context (namely\nthe \"git-serve\" command), let's handle a NULL by looking only at\ntransfer.hiderefs (as opposed to segfaulting).\n\nSigned-off-by: Jeff King \nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n refs.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/refs.c b/refs.c\nindex f9936355cda..099e91d9cc0 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1267,7 +1267,8 @@ int parse_hide_refs_config(const char *var, const char *value, const char *secti\n {\n \tconst char *key;\n \tif (!strcmp(\"transfer.hiderefs\", var) ||\n-\t (!parse_config_key(var, section, NULL, NULL, &key) &&\n+\t (section &&\n+\t !parse_config_key(var, section, NULL, NULL, &key) &&\n \t !strcmp(key, \"hiderefs\"))) {\n \t\tchar *ref;\n \t\tint len;\n-- \n2.20.0.405.gbc1bbc6f85\n\n"},{"id":"365505","messageId":"20181217224054.4376-5-avarab@gmail.com","threadId":"50006","inReplyTo":"20181213155817.27666-1-avarab@gmail.com","subject":"[PATCH v3 4/4] fetch-pack: support protocol version 2","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-12-17T22:40:54Z","receivedAt":"2018-12-17T22:41:14Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"From: Jonathan Tan \n\nWhen the scaffolding for protocol version 2 was initially added in\n8f6982b4e1 (\"protocol: introduce enum protocol_version value\nprotocol_v2\", 2018-03-14). As seen in:\n\n git log -p -G'support for protocol v2 not implemented yet' --full-diff --reverse v2.17.0..v2.20.0\n\nMany of those scaffolding \"die\" placeholders were removed, but we\nhadn't gotten around to fetch-pack yet.\n\nThe test here for \"fetch refs from cmdline\" is very minimal. There's\nmuch better coverage when running the entire test suite under the WIP\nGIT_TEST_PROTOCOL_VERSION=2 mode[1], we should ideally have better\ncoverage without needing to invoke a special test mode.\n\n1. https://public-inbox.org/git/20181213155817.27666-1-avarab@gmail.com/\n\nSigned-off-by: Jonathan Tan \nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/fetch-pack.c | 9 ++++++---\n t/t5500-fetch-pack.sh | 22 +++++++++++++++-------\n 2 files changed, 21 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex 63e69a58011..f6a513495ea 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -55,6 +55,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \tstruct oid_array shallow = OID_ARRAY_INIT;\n \tstruct string_list deepen_not = STRING_LIST_INIT_DUP;\n \tstruct packet_reader reader;\n+\tenum protocol_version version;\n \n \tfetch_if_missing = 0;\n \n@@ -219,9 +220,11 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \t\t\t PACKET_READ_CHOMP_NEWLINE |\n \t\t\t PACKET_READ_GENTLE_ON_EOF);\n \n-\tswitch (discover_version(&reader)) {\n+\tversion = discover_version(&reader);\n+\tswitch (version) {\n \tcase protocol_v2:\n-\t\tdie(\"support for protocol v2 not implemented yet\");\n+\t\tget_remote_refs(fd[1], &reader, &ref, 0, NULL, NULL);\n+\t\tbreak;\n \tcase protocol_v1:\n \tcase protocol_v0:\n \t\tget_remote_heads(&reader, &ref, 0, NULL, &shallow);\n@@ -231,7 +234,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \t}\n \n \tref = fetch_pack(&args, fd, conn, ref, dest, sought, nr_sought,\n-\t\t\t &shallow, pack_lockfile_ptr, protocol_v0);\n+\t\t\t &shallow, pack_lockfile_ptr, version);\n \tif (pack_lockfile) {\n \t\tprintf(\"lock %s\\n\", pack_lockfile);\n \t\tfflush(stdout);\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex 086f2c40f68..49c540b1e1d 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -439,15 +439,23 @@ test_expect_success 'setup tests for the --stdin parameter' '\n \t) >input.dup\n '\n \n-test_expect_success 'fetch refs from cmdline' '\n-\t(\n-\t\tcd client &&\n-\t\tgit fetch-pack --no-progress .. $(cat ../input)\n-\t) >output &&\n-\tcut -d \" \" -f 2 actual &&\n-\ttest_cmp expect actual\n+test_expect_success 'setup fetch refs from cmdline v[12]' '\n+\tcp -r client client1 &&\n+\tcp -r client client2\n '\n \n+for version in '' 1 2\n+do\n+\ttest_expect_success \"protocol.version=$version fetch refs from cmdline\" \"\n+\t\t(\n+\t\t\tcd client$version &&\n+\t\t\tGIT_TEST_PROTOCOL_VERSION=$version git fetch-pack --no-progress .. \\$(cat ../input)\n+\t\t) >output &&\n+\t\tcut -d ' ' -f 2 actual &&\n+\t\ttest_cmp expect actual\n+\t\"\n+done\n+\n test_expect_success 'fetch refs from stdin' '\n \t(\n \t\tcd client &&\n-- \n2.20.0.405.gbc1bbc6f85\n\n"},{"id":"365506","messageId":"F368D768-7DC6-479C-8888-2A41360B0B6A@novalis.org","threadId":"50006","inReplyTo":"20181217221625.1523-1-avarab@gmail.com","subject":"Re: [PATCH] upload-pack: turn on uploadpack.allowAnySHA1InWant=true","fromName":"David Turner","fromEmail":"novalis@novalis.org","sentAt":"2018-12-17T22:34:33Z","receivedAt":"2018-12-17T22:41:46Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"Overall, I like this. One nit:\n\nOn December 17, 2018 5:16:25 PM EST, \"Ævar Arnfjörð Bjarmason\" wrote:\n>--- a/upload-pack.c\n>+++ b/upload-pack.c\n>@@ -54,7 +54,8 @@ static int no_progress, daemon_mode;\n> #define ALLOW_REACHABLE_SHA1\t02\n>/* Allow request of any sha1. Implies ALLOW_TIP_SHA1 and\n>ALLOW_REACHABLE_SHA1. */\n> #define ALLOW_ANY_SHA1\t07\n>-static unsigned int allow_unadvertised_object_request;\n>+static unsigned int allow_unadvertised_object_request = (\n>+\tALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1 | ALLOW_ANY_SHA1);\n\nALLOW_ANY_SHA1 already includes the other two.\n\n-- \nSent from my Android device with K-9 Mail. Please excuse my brevity.\n"},{"id":"365508","messageId":"875zvr2235.fsf@evledraar.gmail.com","threadId":"50006","inReplyTo":"F368D768-7DC6-479C-8888-2A41360B0B6A@novalis.org","subject":"Re: [PATCH] upload-pack: turn on uploadpack.allowAnySHA1InWant=true","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-12-17T22:57:50Z","receivedAt":"2018-12-17T22:57:56Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Dec 17 2018, David Turner wrote:\n\n> Overall, I like this. One nit:\n\nThanks for the review!\n\n> On December 17, 2018 5:16:25 PM EST, \"Ævar Arnfjörð Bjarmason\" wrote:\n>>--- a/upload-pack.c\n>>+++ b/upload-pack.c\n>>@@ -54,7 +54,8 @@ static int no_progress, daemon_mode;\n>> #define ALLOW_REACHABLE_SHA1\t02\n>>/* Allow request of any sha1. Implies ALLOW_TIP_SHA1 and\n>>ALLOW_REACHABLE_SHA1. */\n>> #define ALLOW_ANY_SHA1\t07\n>>-static unsigned int allow_unadvertised_object_request;\n>>+static unsigned int allow_unadvertised_object_request = (\n>>+\tALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1 | ALLOW_ANY_SHA1);\n>\n> ALLOW_ANY_SHA1 already includes the other two.\n\nFWIW (and maybe I made the wrong call, and have no preference realy) I\nopted for this as part of \"this change doesn't do any of the hard work\nof extracting the now-dead ALLOW_[...]*.\n\nI.e. the diff context I had doesn't show all the ALLOW_* values, and\nwith the context given it might be easier for reviewers to look no\nfurther than \"this is what you'd get before if all\nuploadpack.allow.*SHA1InWant was true\".\n"},{"id":"365509","messageId":"879B301E-C36D-404C-8354-5CFAF48B3B7E@novalis.org","threadId":"50006","inReplyTo":"875zvr2235.fsf@evledraar.gmail.com","subject":"Re: [PATCH] upload-pack: turn on uploadpack.allowAnySHA1InWant=true","fromName":"David Turner","fromEmail":"novalis@novalis.org","sentAt":"2018-12-17T23:07:37Z","receivedAt":"2018-12-17T23:07:51Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"\n\nOn December 17, 2018 5:57:50 PM EST, \"Ævar Arnfjörð Bjarmason\" wrote:\n>\n>On Mon, Dec 17 2018, David Turner wrote:\n>\n>> Overall, I like this. One nit:\n>\n>Thanks for the review!\n>\n>> On December 17, 2018 5:16:25 PM EST, \"Ævar Arnfjörð Bjarmason\"\n> wrote:\n>>>--- a/upload-pack.c\n>>>+++ b/upload-pack.c\n>>>@@ -54,7 +54,8 @@ static int no_progress, daemon_mode;\n>>> #define ALLOW_REACHABLE_SHA1\t02\n>>>/* Allow request of any sha1. Implies ALLOW_TIP_SHA1 and\n>>>ALLOW_REACHABLE_SHA1. */\n>>> #define ALLOW_ANY_SHA1\t07\n>>>-static unsigned int allow_unadvertised_object_request;\n>>>+static unsigned int allow_unadvertised_object_request = (\n>>>+\tALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1 | ALLOW_ANY_SHA1);\n>>\n>> ALLOW_ANY_SHA1 already includes the other two.\n>\n>FWIW (and maybe I made the wrong call, and have no preference realy) I\n>opted for this as part of \"this change doesn't do any of the hard work\n>of extracting the now-dead ALLOW_[...]*.\n>\n>I.e. the diff context I had doesn't show all the ALLOW_* values, and\n>with the context given it might be easier for reviewers to look no\n>further than \"this is what you'd get before if all\n>uploadpack.allow.*SHA1InWant was true\".\n\nThe context I quoted said /* Allow request of any sha1. Implies ALLOW_TIP_SHA1 and\nALLOW_REACHABLE_SHA1. */\n\nBut up to you (or Junio).\n-- \nSent from my Android device with K-9 Mail. Please excuse my brevity.\n"},{"id":"365510","messageId":"20181217231452.GA13835@google.com","threadId":"50006","inReplyTo":"20181217195713.GA10673@sigill.intra.peff.net","subject":"Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-12-17T23:14:52Z","receivedAt":"2018-12-17T23:14:57Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJeff King wrote:\n> On Fri, Dec 14, 2018 at 11:55:30AM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n>> More importantly this bypasses the security guarantee we've had with the\n>> default of uploadpack.allowAnySHA1InWant=false.\n>\n> IMHO those security guarantees there are overrated (due to delta\n> guessing attacks, though things are not quite as bad if the attacker\n> can't actually push to the repo).\n\nDo you have a proof of concept for delta guessing? My understanding\nwas that without using a broken hash (e.g. uncorrected SHA-1), it is\nnot feasible to carry out.\n\nJGit checks delta bases in received thin packs for reachability as\nwell.\n\n> But I agree that people do assume it's the case. I was certainly\n> surprised by the v2 behavior, and I don't remember that aspect being\n> discussed.\n\nIMHO it's a plain bug (either in implementation or documentation).\n\n[...]\n>> I'm inclined to say that in the face of that \"SECURITY\" section we\n>> should just:\n>>\n>> * Turn on uploadpack.allowReachableSHA1InWant for v0/v1 by\n>> default. Make saying uploadpack.allowReachableSHA1InWant=false warn\n>> with \"this won't work, see SECURITY...\".\n>>\n>> * The uploadpack.allowTipSHA1InWant setting will also be turned on by\n>> default, and will be much faster, since it'll just degrade to\n>> uploadpack.allowReachableSHA1InWant=true and we won't need any\n>> reachability check. We'll also warn saying that setting it is\n>> useless.\n>\n> No real argument from me. I have always thought those security\n> guarantees were BS.\n\nThis would make per-branch ACLs (as implemented both by Gerrit and\ngitolite) an essentially useless feature, so please no.\n\nI would be all for changing the default, but making turning off\nallowReachableSHA1InWant an unsupported deprecated thing is a step too\nfar, in my opinion.\n\nIs there somewhere that we can document these kinds of invariants or\ngoals so that we don't have to keep repeating the same discussions?\n\nThanks,\nJonathan\n"},{"id":"365511","messageId":"874lbb209x.fsf@evledraar.gmail.com","threadId":"50006","inReplyTo":"20181217231452.GA13835@google.com","subject":"Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-12-17T23:36:58Z","receivedAt":"2018-12-17T23:37:04Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Dec 17 2018, Jonathan Nieder wrote:\n\n> Hi,\n>\n> Jeff King wrote:\n>> On Fri, Dec 14, 2018 at 11:55:30AM +0100, Ævar Arnfjörð Bjarmason wrote:\n>\n>>> More importantly this bypasses the security guarantee we've had with the\n>>> default of uploadpack.allowAnySHA1InWant=false.\n>>\n>> IMHO those security guarantees there are overrated (due to delta\n>> guessing attacks, though things are not quite as bad if the attacker\n>> can't actually push to the repo).\n>\n> Do you have a proof of concept for delta guessing? My understanding\n> was that without using a broken hash (e.g. uncorrected SHA-1), it is\n> not feasible to carry out.\n>\n> JGit checks delta bases in received thin packs for reachability as\n> well.\n>\n>> But I agree that people do assume it's the case. I was certainly\n>> surprised by the v2 behavior, and I don't remember that aspect being\n>> discussed.\n>\n> IMHO it's a plain bug (either in implementation or documentation).\n>\n> [...]\n>>> I'm inclined to say that in the face of that \"SECURITY\" section we\n>>> should just:\n>>>\n>>> * Turn on uploadpack.allowReachableSHA1InWant for v0/v1 by\n>>> default. Make saying uploadpack.allowReachableSHA1InWant=false warn\n>>> with \"this won't work, see SECURITY...\".\n>>>\n>>> * The uploadpack.allowTipSHA1InWant setting will also be turned on by\n>>> default, and will be much faster, since it'll just degrade to\n>>> uploadpack.allowReachableSHA1InWant=true and we won't need any\n>>> reachability check. We'll also warn saying that setting it is\n>>> useless.\n>>\n>> No real argument from me. I have always thought those security\n>> guarantees were BS.\n>\n> This would make per-branch ACLs (as implemented both by Gerrit and\n> gitolite) an essentially useless feature, so please no.\n\nDoesn't Gerrit always use JGit?\n\nThe discussion upthread is about what we should do about git.git's\nversion of this feature since we document it doesn't do its job from a\nsecurity / ACL standpoint, but that doesn't preclude other server\nimplementations from having a working version of this.\n\nBut if gitolite is relying on this to hide refs, and if our security\ndocs are to be believed, then it's just providing security through\nobscurity.\n\nLike you I'm curious about a POC. The patch I submitted in\n<20181217221625.1523-1-avarab@gmail.com> takes the \"SECURITY\" docs at\nface value. I.e. I think it's dangerous to expose this as-is given the\nscary non-promise they make, but perhaps we'll find that this actually\nworks well enough in some cases, or there's other non-security uses for\nthis (e.g. simply discouraging users from cloning certain things,\ne.g. for cache performance purposes).\n\n> I would be all for changing the default, but making turning off\n> allowReachableSHA1InWant an unsupported deprecated thing is a step too\n> far, in my opinion.\n>\n> Is there somewhere that we can document these kinds of invariants or\n> goals so that we don't have to keep repeating the same discussions?\n>\n> Thanks,\n> Jonathan\n"},{"id":"365512","messageId":"20181218000227.GB13835@google.com","threadId":"50006","inReplyTo":"874lbb209x.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-12-18T00:02:27Z","receivedAt":"2018-12-18T00:02:33Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ævar Arnfjörð Bjarmason wrote:\n> On Mon, Dec 17 2018, Jonathan Nieder wrote:\n\n>> This would make per-branch ACLs (as implemented both by Gerrit and\n>> gitolite) an essentially useless feature, so please no.\n>\n> Doesn't Gerrit always use JGit?\n>\n> The discussion upthread is about what we should do about git.git's\n> version of this feature since we document it doesn't do its job from a\n> security / ACL standpoint, but that doesn't preclude other server\n> implementations from having a working version of this.\n>\n> But if gitolite is relying on this to hide refs, and if our security\n> docs are to be believed, then it's just providing security through\n> obscurity.\n\nYes, Gerrit uses JGit. JGit shares configuration with Git, so if we\nmake that configuration in Git warn \"This is just a placebo\", then\npeople will naturally assume that it's just a placebo in JGit, too.\n\nMore importantly, if Git upstream stops caring about this use case,\nthen the protocol and other features can evolve in directions that\nmake it even harder to support. I am willing to take on some of the\nburden of keeping it working, even when I do not run any servers that\nuse plain Git (I do interact with many).\n\n> Like you I'm curious about a POC. The patch I submitted in\n> <20181217221625.1523-1-avarab@gmail.com> takes the \"SECURITY\" docs at\n> face value.\n\nOne of the difficulties that security engineers have to deal with is\nliving without absolutes. In other words, their experience is\nconstant risk/benefit tradeoffs: if you want 0% risk, then don't use a\ncomputer. ;-)\n\nIf someone has thoughts on what would lead people to be comfortable\nwith removing the SECURITY notice, then I'm happy to listen. As\nalready mentioned, I am strongly against abandoning this use case.\n\nJonathan\n"},{"id":"365521","messageId":"8736qv18w4.fsf@evledraar.gmail.com","threadId":"50006","inReplyTo":"20181218000227.GB13835@google.com","subject":"Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-12-18T09:28:27Z","receivedAt":"2018-12-18T09:28:32Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Dec 18 2018, Jonathan Nieder wrote:\n\n> Ævar Arnfjörð Bjarmason wrote:\n>> On Mon, Dec 17 2018, Jonathan Nieder wrote:\n>\n>>> This would make per-branch ACLs (as implemented both by Gerrit and\n>>> gitolite) an essentially useless feature, so please no.\n>>\n>> Doesn't Gerrit always use JGit?\n>>\n>> The discussion upthread is about what we should do about git.git's\n>> version of this feature since we document it doesn't do its job from a\n>> security / ACL standpoint, but that doesn't preclude other server\n>> implementations from having a working version of this.\n>>\n>> But if gitolite is relying on this to hide refs, and if our security\n>> docs are to be believed, then it's just providing security through\n>> obscurity.\n>\n> Yes, Gerrit uses JGit. JGit shares configuration with Git, so if we\n> make that configuration in Git warn \"This is just a placebo\", then\n> people will naturally assume that it's just a placebo in JGit, too.\n\nAside from the merits of this change it sounds like a good idea to\ndocument the server options JGit shares with us somewhere, or have a\ntest mode similar to what I added in (but didn't follow-up on\nintegrating) in\nhttps://public-inbox.org/git/20170516203712.15921-1-avarab@gmail.com/\n\n> More importantly, if Git upstream stops caring about this use case,\n> then the protocol and other features can evolve in directions that\n> make it even harder to support. I am willing to take on some of the\n> burden of keeping it working, even when I do not run any servers that\n> use plain Git (I do interact with many).\n>\n>> Like you I'm curious about a POC. The patch I submitted in\n>> <20181217221625.1523-1-avarab@gmail.com> takes the \"SECURITY\" docs at\n>> face value.\n>\n> One of the difficulties that security engineers have to deal with is\n> living without absolutes. In other words, their experience is\n> constant risk/benefit tradeoffs: if you want 0% risk, then don't use a\n> computer. ;-)\n>\n> If someone has thoughts on what would lead people to be comfortable\n> with removing the SECURITY notice, then I'm happy to listen. As\n> already mentioned, I am strongly against abandoning this use case.\n\nFor me this would be docs backed up by tests (which can start as a ML\nreply) showing a case where this actually works to hide data.\n\nI.e. have a repo with \"master\" and a \"root-password\" branch, where the\n\"root-password\" branch has content that's irresistible to \"git repack\"\nfor delta purposes, do we end up sending the \"root-password\" content\nover on a fetch even when that branch isn't advertised & forbidden?\n\nOr, if that fails are there ways to make it work? E.g. using hidden/* in\ncombination with delta islands?\n"},{"id":"365527","messageId":"20181218123646.GA30471@sigill.intra.peff.net","threadId":"50006","inReplyTo":"20181217231452.GA13835@google.com","subject":"Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-12-18T12:36:46Z","receivedAt":"2018-12-18T12:36:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 17, 2018 at 03:14:52PM -0800, Jonathan Nieder wrote:\n\n> > IMHO those security guarantees there are overrated (due to delta\n> > guessing attacks, though things are not quite as bad if the attacker\n> > can't actually push to the repo).\n> \n> Do you have a proof of concept for delta guessing? My understanding\n> was that without using a broken hash (e.g. uncorrected SHA-1), it is\n> not feasible to carry out.\n\nI think we may be talking about two different things. I mean an attack\nwhere you want to know what is in object X, so you ask the server for\nobject Y and tell it that you already have X. If the sender generates a\ndelta against X, that tells you something about what's in X.\n\nFor a pure read-only server, you're restricted to the Y's that are\nalready in the repo. So how effective this is depends on what's in X,\nand what Y's are available.\n\nFor a case where X is in a victim repo you cannot make arbitrary writes\nto, but you _can_ make the victim repo aware of other objects (e.g., by\nopening a pull request that creates a ref), then you can iteratively\nprovide many Y's, improving your guess about X in each iteration.\n\nFor a case where the victim repo has fully shared storage (GitHub, and\nprobably other hosts; I'm not sure if it's available yet, but GitLab is\nclearly working on shared-storage too), you can probably skip all that\nand just push a ref pointing to X with an empty pack (Git just cares\nthat it has all of the objects afterwards, not that you pushed them).\n\nNone of those care about the quality of the hash (they do assume you\nknow the hash of X already, but then so does fetching by object id).\n\nAnd no, I've never written a proof-of-concept for that. It would depend\nlargely on the data you're trying to extract. E.g., if you think X\ncontains \"root:XXXXXX\", then you might hope to ask for \"root:AXXXXX\",\nthen \"root:BXXXXX\", etc. You know you've got a hit when the delta gets\nsmaller. So the complexity for guessing N bytes becomes 256*N, rather\nthan 256^N.\n\n> > But I agree that people do assume it's the case. I was certainly\n> > surprised by the v2 behavior, and I don't remember that aspect being\n> > discussed.\n> \n> IMHO it's a plain bug (either in implementation or documentation).\n\nOr both. :) The behavior and the documentation seem to agree.\n\n> [...]\n> >> I'm inclined to say that in the face of that \"SECURITY\" section we\n> >> should just:\n> >>\n> >> * Turn on uploadpack.allowReachableSHA1InWant for v0/v1 by\n> >> default. Make saying uploadpack.allowReachableSHA1InWant=false warn\n> >> with \"this won't work, see SECURITY...\".\n> >>\n> >> * The uploadpack.allowTipSHA1InWant setting will also be turned on by\n> >> default, and will be much faster, since it'll just degrade to\n> >> uploadpack.allowReachableSHA1InWant=true and we won't need any\n> >> reachability check. We'll also warn saying that setting it is\n> >> useless.\n> >\n> > No real argument from me. I have always thought those security\n> > guarantees were BS.\n> \n> This would make per-branch ACLs (as implemented both by Gerrit and\n> gitolite) an essentially useless feature, so please no.\n\nI think Ævar's argument is that those are providing a false sense of\nsecurity now (at least for read permissions).\n\nLet me clarify my position a little.\n\nI won't claim the existing scheme provides _no_ value (especially the\npure read-only case above is less dicey than the others). It's mostly\nthat the protections are very hand-wavy. I don't trust them _now_, and I\nhave little faith that other innocent-looking changes to the object\nnegotiation and the packing code will not significantly weaken them.\nThere's no security boundary expressed within Git's code, so there's a\nvery high chance of information leaking accidentally. A trustable system\nwould have boundaries built in from the ground up.\n\nEnough people seem to believe otherwise (i.e., that the hand-waving\narguments are worth _something_) that I've never pushed to actually\nchange the default behavior. But if Ævar wants to try to do so, I'm not\ngoing to stand in my way (hence my \"no argument from me\").\n\n> I would be all for changing the default, but making turning off\n> allowReachableSHA1InWant an unsupported deprecated thing is a step too\n> far, in my opinion.\n\nYes, I agree if we were to go down this road, it probably makes sense to\nflip the knobs and let them be \"unflipped\" if the user wants.\n\n> Is there somewhere that we can document these kinds of invariants or\n> goals so that we don't have to keep repeating the same discussions?\n\nIt's not clear to me that there's consensus on the invariants or goals.\n;)\n\n-Peff\n"},{"id":"365528","messageId":"20181218124127.GB30471@sigill.intra.peff.net","threadId":"50006","inReplyTo":"8736qv18w4.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-12-18T12:41:27Z","receivedAt":"2018-12-18T12:41:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 18, 2018 at 10:28:27AM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> I.e. have a repo with \"master\" and a \"root-password\" branch, where the\n> \"root-password\" branch has content that's irresistible to \"git repack\"\n> for delta purposes, do we end up sending the \"root-password\" content\n> over on a fetch even when that branch isn't advertised & forbidden?\n> \n> Or, if that fails are there ways to make it work? E.g. using hidden/* in\n> combination with delta islands?\n\nDelta islands wouldn't generally help here. They only tell us not to\nstore on-disk deltas that fetching clients aren't likely to be able to\nreuse (i.e, they want X but will generally not have Y, so don't make a\ndelta there).\n\nIn the attacks I mentioned in my previous email, the deltas would\nusually be computed on the fly for each fetch. So the lack of on-disk\ndeltas across \"security boundaries\" wouldn't help.\n\nYou could disable on-the-fly delta compression, but the resulting packs\nare much larger (and you'd think it would save some server CPU, but\nexperiments I've done show that it helps a lot less than you'd think,\nsince we often end up zlib-deflating more bytes).\n\n-Peff\n"},{"id":"365529","messageId":"20181218124750.GC30471@sigill.intra.peff.net","threadId":"50006","inReplyTo":"xmqqo99lbu9o.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/3] serve: pass \"config context\" through to individual commands","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-12-18T12:47:50Z","receivedAt":"2018-12-18T12:47:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Dec 16, 2018 at 08:12:03PM +0900, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Yeah, I agree that such a \"pass this through\" struct full of options and\n> > context would make sense. I just wouldn't tie it to the \"serve\"\n> > machinery.\n> >\n> > Did you read the side-thread between me and Jonathan? Another option\n> > here is to just have ls-refs assume that the client will tell it the\n> > context (and assume uploadpack for now, since that's all that v2\n> > currently does).\n> \n> Yes, I'd be 100% happy with that, too. And it certainly is simpler.\n\nOK, let's do that, then. The user-visible behavior is no different, so\nwe can always reverse course later when the v2 push scheme materializes.\nPatch is below.\n\n> P.S. I expect to be mostly offline for the coming 72 hours, as I and\n> my wife are both down with a cold. I am guessing that we will enter\n> slower weeks in many parts of the world, and hoping this won't be\n> too disruptive.\n\nHope you are feeling better. I'll be active through the rest of this\nweek, and then probably offline quite a bit for the two weeks following.\n\n-Peff\n\n-- >8 --\nSubject: [PATCH] upload-pack: support hidden refs with protocol v2\n\nIn the v2 protocol, upload-pack's advertisement has been moved to the\n\"ls-refs\" command. That command does not respect hidden-ref config (like\ntransfer.hiderefs) at all, and advertises everything.\n\nWhile there are some features that are not supported in v2 (e.g., v2\nalways allows fetching any sha1 without respect to advertisements), the\nlack of this feature is not documented and is likely just a bug. Let's\nmake it work, as otherwise upgrading a server to a v2-capable git will\nstart exposing these refs that the repository admin has asked to remain\nhidden.\n\nNote that we assume we're operating on behalf of a fetch here, since\nthat's the only thing implemented in v2 at this point. See the in-code\ncomment. We'll have to figure out how this works when the v2 push\nprotocol is designed (both here in ls-refs, but also rejecting updates\nto hidden refs).\n\nSigned-off-by: Jeff King \n---\n ls-refs.c | 16 ++++++++++++++++\n t/t5512-ls-remote.sh | 6 ++++++\n 2 files changed, 22 insertions(+)\n\ndiff --git a/ls-refs.c b/ls-refs.c\nindex a06f12eca8..9c9a7c647f 100644\n--- a/ls-refs.c\n+++ b/ls-refs.c\n@@ -5,6 +5,7 @@\n #include \"argv-array.h\"\n #include \"ls-refs.h\"\n #include \"pkt-line.h\"\n+#include \"config.h\"\n \n /*\n * Check if one of the prefixes is a prefix of the ref.\n@@ -40,6 +41,9 @@ static int send_ref(const char *refname, const struct object_id *oid,\n \tconst char *refname_nons = strip_namespace(refname);\n \tstruct strbuf refline = STRBUF_INIT;\n \n+\tif (ref_is_hidden(refname_nons, refname))\n+\t\treturn 0;\n+\n \tif (!ref_match(&data->prefixes, refname))\n \t\treturn 0;\n \n@@ -69,6 +73,16 @@ static int send_ref(const char *refname, const struct object_id *oid,\n \treturn 0;\n }\n \n+static int ls_refs_config(const char *var, const char *value, void *data)\n+{\n+\t/*\n+\t * We only serve fetches over v2 for now, so respect only \"uploadpack\"\n+\t * config. This may need to eventually be expanded to \"receive\", but we\n+\t * don't yet know how that information will be passed to ls-refs.\n+\t */\n+\treturn parse_hide_refs_config(var, value, \"uploadpack\");\n+}\n+\n int ls_refs(struct repository *r, struct argv_array *keys,\n \t struct packet_reader *request)\n {\n@@ -76,6 +90,8 @@ int ls_refs(struct repository *r, struct argv_array *keys,\n \n \tmemset(&data, 0, sizeof(data));\n \n+\tgit_config(ls_refs_config, NULL);\n+\n \twhile (packet_reader_read(request) != PACKET_READ_FLUSH) {\n \t\tconst char *arg = request->line;\n \t\tconst char *out;\ndiff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh\nindex 32e722db2e..ca69636fd5 100755\n--- a/t/t5512-ls-remote.sh\n+++ b/t/t5512-ls-remote.sh\n@@ -204,6 +204,12 @@ test_expect_success 'overrides work between mixed transfer/upload-pack hideRefs'\n \tgrep refs/tags/magic actual\n '\n \n+test_expect_success 'protocol v2 supports hiderefs' '\n+\ttest_config uploadpack.hiderefs refs/tags &&\n+\tgit -c protocol.version=2 ls-remote . >actual &&\n+\t! grep refs/tags actual\n+'\n+\n test_expect_success 'ls-remote --symref' '\n \tgit fetch origin &&\n \tcat >expect <<-EOF &&\n-- \n2.20.1.748.g4743e5683b\n\n"},{"id":"365530","messageId":"20181218124852.GD30471@sigill.intra.peff.net","threadId":"50006","inReplyTo":"20181217224054.4376-1-avarab@gmail.com","subject":"Re: [PATCH v3 0/4] protocol v2 fixes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-12-18T12:48:52Z","receivedAt":"2018-12-18T12:48:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 17, 2018 at 11:40:50PM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> No changes to Jeff's patches since v2, for Jonathan's no changes to\n> the C code, but I added a test which I extracted from the\n> GIT_TEST_PROTOCOL_VERSION=2 work.\n> \n> Jeff King (3):\n> serve: pass \"config context\" through to individual commands\n> parse_hide_refs_config: handle NULL section\n> upload-pack: support hidden refs with protocol v2\n\nThese should be dropped in favor of:\n\n https://public-inbox.org/git/20181218124750.GC30471@sigill.intra.peff.net/\n\nthat I just sent (I tweaked the commit message to address the questions\nyou had in push for patch 3).\n\n-Peff\n"},{"id":"365531","messageId":"87d0pzf0as.fsf@evledraar.gmail.com","threadId":"50006","inReplyTo":"20181218123646.GA30471@sigill.intra.peff.net","subject":"Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-12-18T13:10:19Z","receivedAt":"2018-12-18T13:10:26Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Dec 18 2018, Jeff King wrote:\n\n> On Mon, Dec 17, 2018 at 03:14:52PM -0800, Jonathan Nieder wrote:\n>\n>> > IMHO those security guarantees there are overrated (due to delta\n>> > guessing attacks, though things are not quite as bad if the attacker\n>> > can't actually push to the repo).\n>>\n>> Do you have a proof of concept for delta guessing? My understanding\n>> was that without using a broken hash (e.g. uncorrected SHA-1), it is\n>> not feasible to carry out.\n>\n> I think we may be talking about two different things. I mean an attack\n> where you want to know what is in object X, so you ask the server for\n> object Y and tell it that you already have X. If the sender generates a\n> delta against X, that tells you something about what's in X.\n>\n> For a pure read-only server, you're restricted to the Y's that are\n> already in the repo. So how effective this is depends on what's in X,\n> and what Y's are available.\n>\n> For a case where X is in a victim repo you cannot make arbitrary writes\n> to, but you _can_ make the victim repo aware of other objects (e.g., by\n> opening a pull request that creates a ref), then you can iteratively\n> provide many Y's, improving your guess about X in each iteration.\n>\n> For a case where the victim repo has fully shared storage (GitHub, and\n> probably other hosts; I'm not sure if it's available yet, but GitLab is\n> clearly working on shared-storage too), you can probably skip all that\n> and just push a ref pointing to X with an empty pack (Git just cares\n> that it has all of the objects afterwards, not that you pushed them).\n>\n> None of those care about the quality of the hash (they do assume you\n> know the hash of X already, but then so does fetching by object id).\n>\n> And no, I've never written a proof-of-concept for that. It would depend\n> largely on the data you're trying to extract. E.g., if you think X\n> contains \"root:XXXXXX\", then you might hope to ask for \"root:AXXXXX\",\n> then \"root:BXXXXX\", etc. You know you've got a hit when the delta gets\n> smaller. So the complexity for guessing N bytes becomes 256*N, rather\n> than 256^N.\n>\n>> > But I agree that people do assume it's the case. I was certainly\n>> > surprised by the v2 behavior, and I don't remember that aspect being\n>> > discussed.\n>>\n>> IMHO it's a plain bug (either in implementation or documentation).\n>\n> Or both. :) The behavior and the documentation seem to agree.\n>\n>> [...]\n>> >> I'm inclined to say that in the face of that \"SECURITY\" section we\n>> >> should just:\n>> >>\n>> >> * Turn on uploadpack.allowReachableSHA1InWant for v0/v1 by\n>> >> default. Make saying uploadpack.allowReachableSHA1InWant=false warn\n>> >> with \"this won't work, see SECURITY...\".\n>> >>\n>> >> * The uploadpack.allowTipSHA1InWant setting will also be turned on by\n>> >> default, and will be much faster, since it'll just degrade to\n>> >> uploadpack.allowReachableSHA1InWant=true and we won't need any\n>> >> reachability check. We'll also warn saying that setting it is\n>> >> useless.\n>> >\n>> > No real argument from me. I have always thought those security\n>> > guarantees were BS.\n>>\n>> This would make per-branch ACLs (as implemented both by Gerrit and\n>> gitolite) an essentially useless feature, so please no.\n>\n> I think Ævar's argument is that those are providing a false sense of\n> security now (at least for read permissions).\n>\n> Let me clarify my position a little.\n>\n> I won't claim the existing scheme provides _no_ value (especially the\n> pure read-only case above is less dicey than the others). It's mostly\n> that the protections are very hand-wavy. I don't trust them _now_, and I\n> have little faith that other innocent-looking changes to the object\n> negotiation and the packing code will not significantly weaken them.\n> There's no security boundary expressed within Git's code, so there's a\n> very high chance of information leaking accidentally. A trustable system\n> would have boundaries built in from the ground up.\n>\n> Enough people seem to believe otherwise (i.e., that the hand-waving\n> arguments are worth _something_) that I've never pushed to actually\n> change the default behavior. But if Ævar wants to try to do so, I'm not\n> going to stand in my way (hence my \"no argument from me\").\n\nFWIW I don't really care about this, I don't rely on\nuploadpack.allow{Tip,Reachable,Any}SHA1InWant=false I'm just on the\nside-quest of:\n\n 1. Try protocol v2\n 2. Discover that v2 implictly has uploadpack.allowAnySHA1InWant=true\n enabled\n 3. Some people (including Jonathan) can reasonable read our docs /\n seem to have understood this to be a security mechanism\n 4. What are we going to do about that v1 & v2 discrepancy? [You are\n here!]\n\nThe genreal ways I see forward from that are:\n\n A) Say that v2 has a security issue and that this is a feature that\n works in some circumstances, but given Jeff's explanation here we\n should at least improve our \"SECURITY\" docs to be less handwaivy.\n\n B) Improve security docs, turn uploadpack.allowAnySHA1InWant=true on by\n default, allow people to turn it off.\n\n C) Like B) but deprecate\n uploadpack.allow{Tip,Reachable,Any}SHA1InWant=false. This is my\n patch upthread\n\n D-Z) ???\n\n\nI'm not set on C), and yeah it's probably overzelous to just rip the\nthing out, but then what should we do?\n\n>> I would be all for changing the default, but making turning off\n>> allowReachableSHA1InWant an unsupported deprecated thing is a step too\n>> far, in my opinion.\n>\n> Yes, I agree if we were to go down this road, it probably makes sense to\n> flip the knobs and let them be \"unflipped\" if the user wants.\n>\n>> Is there somewhere that we can document these kinds of invariants or\n>> goals so that we don't have to keep repeating the same discussions?\n>\n> It's not clear to me that there's consensus on the invariants or goals.\n> ;)\n>\n> -Peff\n"},{"id":"365818","messageId":"xmqqlg4cq6kd.fsf@gitster-ct.c.googlers.com","threadId":"50006","inReplyTo":"87d0pzf0as.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-26T22:14:26Z","receivedAt":"2018-12-26T22:15:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> The genreal ways I see forward from that are:\n>\n> A) Say that v2 has a security issue and that this is a feature that\n> works in some circumstances, but given Jeff's explanation here we\n> should at least improve our \"SECURITY\" docs to be less handwaivy.\n>\n> B) Improve security docs, turn uploadpack.allowAnySHA1InWant=true on by\n> default, allow people to turn it off.\n>\n> C) Like B) but deprecate\n> uploadpack.allow{Tip,Reachable,Any}SHA1InWant=false. This is my\n> patch upthread\n>\n> D-Z) ???\n>\n>\n> I'm not set on C), and yeah it's probably overzelous to just rip the\n> thing out, but then what should we do?\n\nHmph. The other overzealous thing you could do is to strenthen A\nand \"fix\" the security issue in v2? Which letter comes before A in\nthe alphabet? ;-)\n"},{"id":"365835","messageId":"87va3fdxcd.fsf@evledraar.gmail.com","threadId":"50006","inReplyTo":"xmqqlg4cq6kd.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-12-27T11:26:42Z","receivedAt":"2018-12-27T11:26:46Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Dec 26 2018, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> The genreal ways I see forward from that are:\n>>\n>> A) Say that v2 has a security issue and that this is a feature that\n>> works in some circumstances, but given Jeff's explanation here we\n>> should at least improve our \"SECURITY\" docs to be less handwaivy.\n>>\n>> B) Improve security docs, turn uploadpack.allowAnySHA1InWant=true on by\n>> default, allow people to turn it off.\n>>\n>> C) Like B) but deprecate\n>> uploadpack.allow{Tip,Reachable,Any}SHA1InWant=false. This is my\n>> patch upthread\n>>\n>> D-Z) ???\n>>\n>>\n>> I'm not set on C), and yeah it's probably overzelous to just rip the\n>> thing out, but then what should we do?\n>\n> Hmph. The other overzealous thing you could do is to strenthen A\n> and \"fix\" the security issue in v2? Which letter comes before A in\n> the alphabet? ;-)\n\nSure, but that being useful is predicated on this supposed security\nmechanism being useful and not just security-through-obscurity, as noted\nin side-threads I don't think we have a convincing argument either way\n(and the one we do have is more on the \"it's not secure\" side).\n\nOf course we had that with v1 all along, but now that v2 is in released\nversions and in this insecure mode, we have a reason to closely look at\nwhether we need to be issuing security releases, or doubling down on the\n\"SECURITY\" wording in git-fetch and then not carrying the mode forward.\n"},{"id":"365871","messageId":"20181227171058.GA146609@google.com","threadId":"50006","inReplyTo":"87va3fdxcd.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-12-27T17:10:58Z","receivedAt":"2018-12-27T17:11:04Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nÆvar Arnfjörð Bjarmason wrote:\n> On Wed, Dec 26 2018, Junio C Hamano wrote:\n\n>> Hmph. The other overzealous thing you could do is to strenthen A\n>> and \"fix\" the security issue in v2? Which letter comes before A in\n>> the alphabet? ;-)\n\nYes, agreed. This is what I was hinting at in [1] with \"it's a plain\nbug\".\n\n> Sure, but that being useful is predicated on this supposed security\n> mechanism being useful and not just security-through-obscurity, as noted\n> in side-threads I don't think we have a convincing argument either way\n> (and the one we do have is more on the \"it's not secure\" side).\n>\n> Of course we had that with v1 all along, but now that v2 is in released\n> versions and in this insecure mode, we have a reason to closely look at\n> whether we need to be issuing security releases, or doubling down on the\n> \"SECURITY\" wording in git-fetch and then not carrying the mode forward.\n\nJust for the record, as I've already said, I would be strongly against\nremoving this feature. I know of multiple populations that make use\nof it, and removing it would not serve them well.\n\nChanging defaults and documentation is a separate story.\n\nSincerely,\nJonathan\n\n[1] https://public-inbox.org/git/20181217231452.GA13835@google.com/\n"},{"id":"366360","messageId":"xmqqva2z3pdp.fsf@gitster-ct.c.googlers.com","threadId":"50006","inReplyTo":"20181217224054.4376-5-avarab@gmail.com","subject":"Re: [PATCH v3 4/4] fetch-pack: support protocol version 2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-08T19:45:06Z","receivedAt":"2019-01-08T19:45:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> From: Jonathan Tan \n\n\nI was looking at the topics in 'pu' and noticed that I had v2 of\nthis series, wanted to update to v3, but major part of it was\nsuperseded by another topic (jk/proto-v2-hidden-refs-fix). That\nleaves only this patch in the v3 of this series.\n\nIs this one still relevant?\n\n> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\n> index 086f2c40f68..49c540b1e1d 100755\n> --- a/t/t5500-fetch-pack.sh\n> +++ b/t/t5500-fetch-pack.sh\n> @@ -439,15 +439,23 @@ test_expect_success 'setup tests for the --stdin parameter' '\n> \t) >input.dup\n> '\n> \n> -test_expect_success 'fetch refs from cmdline' '\n> -\t(\n> -\t\tcd client &&\n> -\t\tgit fetch-pack --no-progress .. $(cat ../input)\n> -\t) >output &&\n> -\tcut -d \" \" -f 2 actual &&\n> -\ttest_cmp expect actual\n> +test_expect_success 'setup fetch refs from cmdline v[12]' '\n> +\tcp -r client client1 &&\n> +\tcp -r client client2\n> '\n> \n> +for version in '' 1 2\n> +do\n> +\ttest_expect_success \"protocol.version=$version fetch refs from cmdline\" \"\n> +\t\t(\n> +\t\t\tcd client$version &&\n> +\t\t\tGIT_TEST_PROTOCOL_VERSION=$version git fetch-pack --no-progress .. \\$(cat ../input)\n> +\t\t) >output &&\n> +\t\tcut -d ' ' -f 2 actual &&\n> +\t\ttest_cmp expect actual\n> +\t\"\n> +done\n> +\n> test_expect_success 'fetch refs from stdin' '\n> \t(\n> \t\tcd client &&\n"},{"id":"366362","messageId":"20190108203826.564-1-jonathantanmy@google.com","threadId":"50006","inReplyTo":"xmqqva2z3pdp.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3 4/4] fetch-pack: support protocol version 2","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-01-08T20:38:26Z","receivedAt":"2019-01-08T20:38:32Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Ævar Arnfjörð Bjarmason writes:\n> \n> > From: Jonathan Tan \n> \n> \n> I was looking at the topics in 'pu' and noticed that I had v2 of\n> this series, wanted to update to v3, but major part of it was\n> superseded by another topic (jk/proto-v2-hidden-refs-fix). That\n> leaves only this patch in the v3 of this series.\n> \n> Is this one still relevant?\n\nThis patch is more relevant to the GIT_TEST_PROTOCOL_VERSION patches,\nsince it means that several tests work even if\nGIT_TEST_PROTOCOL_VERSION=2 is set. I think it can be dropped until\nsomeone restarts the GIT_TEST_PROTOCOL_VERSION effort, but I'm not sure\nif Ævar has another opinion.\n"},{"id":"366365","messageId":"20190108211402.GA22946@sigill.intra.peff.net","threadId":"50006","inReplyTo":"20190108203826.564-1-jonathantanmy@google.com","subject":"Re: [PATCH v3 4/4] fetch-pack: support protocol version 2","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-08T21:14:03Z","receivedAt":"2019-01-08T21:14:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 08, 2019 at 12:38:26PM -0800, Jonathan Tan wrote:\n\n> > Ævar Arnfjörð Bjarmason writes:\n> > \n> > > From: Jonathan Tan \n> > \n> > \n> > I was looking at the topics in 'pu' and noticed that I had v2 of\n> > this series, wanted to update to v3, but major part of it was\n> > superseded by another topic (jk/proto-v2-hidden-refs-fix). That\n> > leaves only this patch in the v3 of this series.\n> > \n> > Is this one still relevant?\n> \n> This patch is more relevant to the GIT_TEST_PROTOCOL_VERSION patches,\n> since it means that several tests work even if\n> GIT_TEST_PROTOCOL_VERSION=2 is set. I think it can be dropped until\n> someone restarts the GIT_TEST_PROTOCOL_VERSION effort, but I'm not sure\n> if Ævar has another opinion.\n\nI think it's independently useful. If you set protocol.version=2 in your\nconfig, then you can no longer run fetch-pack directly. Most people\ndon't, but it's possible their scripts might (we also use fetch-pack\nunder the hood for smart-http, but I wasn't able to get it to complain,\nthough).\n\n-Peff\n"}]}