* [PATCH v2 0/8] protocol v2 fixes
2018-12-13 2:49 ` [PATCH 0/3] Some fixes and improvements Junio C Hamano
@ 2018-12-13 15:58 ` Ævar Arnfjörð Bjarmason
2018-12-17 22:40 ` [PATCH v3 0/4] " Ævar Arnfjörð Bjarmason
` (4 more replies)
2018-12-13 15:58 ` [PATCH v2 1/8] serve: pass "config context" through to individual commands Ævar Arnfjörð Bjarmason
` (7 subsequent siblings)
8 siblings, 5 replies; 73+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-12-13 15:58 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Brandon Williams, Jonathan Tan,
Ævar Arnfjörð Bjarmason
I figured it would be easier for everyone if I rolled this all into
one series instead of Junio & us needing to keep track of what's based
on what.
The only change I made to Jeff's patches is my SOB and adding a
paragraph to the end of his 3/3 saying that the v2 push protocol
doesn't have the same issue (because it doesn't exist yet). I had that
question in this thread, and thought it was useful to clarify it.
No changes to Jonathan's one patch, except my SOB.
For the rest I incorporated Jonathan's suggestions / fixes with some
amendments. The suggestion to use env --unset isn't portable (and
there's now a check for that while we're at it), so instead we support
"GIT_TEST_PROTOCOL_VERSION=" which'll ignore the environment value.
Other changes in my patches are more narrowly skipping tests, i.e. no
"unset" anymore except for those tests where we're only doing v1 and
v2 tests. I also removed the "env" use in those cases that don't need
it (where we use e.g. test_must_fail), instead we just set the env
variable ourselves with native shell syntax.
Jeff King (3):
serve: pass "config context" through to individual commands
parse_hide_refs_config: handle NULL section
upload-pack: support hidden refs with protocol v2
Jonathan Tan (1):
builtin/fetch-pack: support protocol version 2
Ævar Arnfjörð Bjarmason (4):
tests: add a check for unportable env --unset
tests: add a special setup where for protocol.version
tests: mark & fix tests broken under GIT_TEST_PROTOCOL_VERSION=1
tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2
builtin/fetch-pack.c | 9 ++++++---
builtin/upload-pack.c | 1 +
ls-refs.c | 16 +++++++++++++++-
ls-refs.h | 3 ++-
protocol.c | 13 ++++++++++++-
refs.c | 3 ++-
serve.c | 9 +++++----
serve.h | 7 +++++++
t/README | 6 ++++++
t/check-non-portable-shell.pl | 1 +
t/t0410-partial-clone.sh | 3 ++-
t/t5400-send-pack.sh | 2 +-
t/t5500-fetch-pack.sh | 9 ++++++---
t/t5503-tagfollow.sh | 8 ++++----
t/t5512-ls-remote.sh | 14 ++++++++++----
t/t5515-fetch-merge-logic.sh | 2 +-
t/t5516-fetch-push.sh | 20 +++++++++++++-------
t/t5537-fetch-shallow.sh | 3 ++-
t/t5539-fetch-http-shallow.sh | 9 +++++----
t/t5541-http-push-smart.sh | 9 +++++++--
t/t5551-http-fetch-smart.sh | 19 +++++++++++--------
t/t5552-skipping-fetch-negotiator.sh | 4 ++--
t/t5570-git-daemon.sh | 2 +-
t/t5601-clone.sh | 11 +++++++++--
t/t5700-protocol-v1.sh | 1 +
t/t5702-protocol-v2.sh | 1 +
t/t7406-submodule-update.sh | 3 ++-
upload-pack.c | 4 ++--
upload-pack.h | 4 ++--
29 files changed, 139 insertions(+), 57 deletions(-)
--
2.20.0.405.gbc1bbc6f85
^ permalink raw reply [flat|nested] 73+ messages in thread
* [PATCH v3 0/4] protocol v2 fixes
2018-12-13 15:58 ` [PATCH v2 0/8] protocol v2 fixes Ævar Arnfjörð Bjarmason
@ 2018-12-17 22:40 ` Ævar Arnfjörð Bjarmason
2018-12-18 12:48 ` Jeff King
2018-12-17 22:40 ` [PATCH v3 1/4] serve: pass "config context" through to individual commands Ævar Arnfjörð Bjarmason
` (3 subsequent siblings)
4 siblings, 1 reply; 73+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-12-17 22:40 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Brandon Williams, Jonathan Tan,
Josh Steadmon, Ævar Arnfjörð Bjarmason
The v2 of this series conflicted with Josh Steadmon's work when merged
in "pu". That's still outstanding, see
https://public-inbox.org/git/87h8ff20ol.fsf@evledraar.gmail.com/
Then my just-sent
https://public-inbox.org/git/20181217221625.1523-1-avarab@gmail.com/
conflicts with even more things in it.
So I'm dropping "GIT_TEST_PROTOCOL_VERSION" for now until things
settle down. That can land after all this protocol activity settles.
No changes to Jeff's patches since v2, for Jonathan's no changes to
the C code, but I added a test which I extracted from the
GIT_TEST_PROTOCOL_VERSION=2 work.
Jeff King (3):
serve: pass "config context" through to individual commands
parse_hide_refs_config: handle NULL section
upload-pack: support hidden refs with protocol v2
Jonathan Tan (1):
fetch-pack: support protocol version 2
builtin/fetch-pack.c | 9 ++++++---
builtin/upload-pack.c | 1 +
ls-refs.c | 16 +++++++++++++++-
ls-refs.h | 3 ++-
refs.c | 3 ++-
serve.c | 9 +++++----
serve.h | 7 +++++++
t/t5500-fetch-pack.sh | 22 +++++++++++++++-------
t/t5512-ls-remote.sh | 6 ++++++
upload-pack.c | 4 ++--
upload-pack.h | 4 ++--
11 files changed, 63 insertions(+), 21 deletions(-)
--
2.20.0.405.gbc1bbc6f85
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v3 0/4] protocol v2 fixes
2018-12-17 22:40 ` [PATCH v3 0/4] " Ævar Arnfjörð Bjarmason
@ 2018-12-18 12:48 ` Jeff King
0 siblings, 0 replies; 73+ messages in thread
From: Jeff King @ 2018-12-18 12:48 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, Brandon Williams, Jonathan Tan,
Josh Steadmon
On Mon, Dec 17, 2018 at 11:40:50PM +0100, Ævar Arnfjörð Bjarmason wrote:
> No changes to Jeff's patches since v2, for Jonathan's no changes to
> the C code, but I added a test which I extracted from the
> GIT_TEST_PROTOCOL_VERSION=2 work.
>
> Jeff King (3):
> serve: pass "config context" through to individual commands
> parse_hide_refs_config: handle NULL section
> upload-pack: support hidden refs with protocol v2
These should be dropped in favor of:
https://public-inbox.org/git/20181218124750.GC30471@sigill.intra.peff.net/
that I just sent (I tweaked the commit message to address the questions
you had in push for patch 3).
-Peff
^ permalink raw reply [flat|nested] 73+ messages in thread
* [PATCH v3 1/4] serve: pass "config context" through to individual commands
2018-12-13 15:58 ` [PATCH v2 0/8] protocol v2 fixes Ævar Arnfjörð Bjarmason
2018-12-17 22:40 ` [PATCH v3 0/4] " Ævar Arnfjörð Bjarmason
@ 2018-12-17 22:40 ` Ævar Arnfjörð Bjarmason
2018-12-17 22:40 ` [PATCH v3 2/4] parse_hide_refs_config: handle NULL section Ævar Arnfjörð Bjarmason
` (2 subsequent siblings)
4 siblings, 0 replies; 73+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-12-17 22:40 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Brandon Williams, Jonathan Tan,
Josh Steadmon, Ævar Arnfjörð Bjarmason
From: Jeff King <peff@peff.net>
In protocol v2, instead of just running "upload-pack", we have a generic
"serve" loop which runs command requests from the client. What used to
be "upload-pack" is now generally split into two operations: "ls-refs"
and "fetch". The latter knows it must respect uploadpack.* config, but
the former is not actually specific to a fetch operation (we do not yet
do v2 receive-pack, but eventually we may, and ls-refs would support
both operations).
However, ls-refs does need to know which operation we're performing, in
order to read the correct config (e.g., uploadpack.hiderefs versus
receive.hiderefs; we don't read _either_ right now, but this is the
first step to fixing that).
In the generic "git-serve" program, we don't have that information. But
in the protocol as it is actually used by clients, the client still asks
to run "git-upload-pack", and then issues an "ls-refs" from there. So we
_do_ know at that point that "uploadpack" is the right config context.
This patch teaches the protocol v2 "serve" code to pass that context
through to the individual commands (a future patch will teach ls-refs to
actually use it).
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/upload-pack.c | 1 +
ls-refs.c | 4 +++-
ls-refs.h | 3 ++-
serve.c | 9 +++++----
serve.h | 7 +++++++
upload-pack.c | 4 ++--
upload-pack.h | 4 ++--
7 files changed, 22 insertions(+), 10 deletions(-)
diff --git a/builtin/upload-pack.c b/builtin/upload-pack.c
index 42dc4da5a1f..67192cfa9e9 100644
--- a/builtin/upload-pack.c
+++ b/builtin/upload-pack.c
@@ -52,6 +52,7 @@ int cmd_upload_pack(int argc, const char **argv, const char *prefix)
case protocol_v2:
serve_opts.advertise_capabilities = opts.advertise_refs;
serve_opts.stateless_rpc = opts.stateless_rpc;
+ serve_opts.config_context = "uploadpack";
serve(&serve_opts);
break;
case protocol_v1:
diff --git a/ls-refs.c b/ls-refs.c
index a06f12eca8d..e8e31475f06 100644
--- a/ls-refs.c
+++ b/ls-refs.c
@@ -69,7 +69,9 @@ static int send_ref(const char *refname, const struct object_id *oid,
return 0;
}
-int ls_refs(struct repository *r, struct argv_array *keys,
+int ls_refs(struct repository *r,
+ const char *config_section,
+ struct argv_array *keys,
struct packet_reader *request)
{
struct ls_refs_data data;
diff --git a/ls-refs.h b/ls-refs.h
index b62877e8dad..da26fc9824d 100644
--- a/ls-refs.h
+++ b/ls-refs.h
@@ -4,7 +4,8 @@
struct repository;
struct argv_array;
struct packet_reader;
-extern int ls_refs(struct repository *r, struct argv_array *keys,
+extern int ls_refs(struct repository *r, const char *config_context,
+ struct argv_array *keys,
struct packet_reader *request);
#endif /* LS_REFS_H */
diff --git a/serve.c b/serve.c
index bda085f09c8..70f89cf0d98 100644
--- a/serve.c
+++ b/serve.c
@@ -48,6 +48,7 @@ struct protocol_capability {
* This field should be NULL for capabilities which are not commands.
*/
int (*command)(struct repository *r,
+ const char *config_context,
struct argv_array *keys,
struct packet_reader *request);
};
@@ -158,7 +159,7 @@ enum request_state {
PROCESS_REQUEST_DONE,
};
-static int process_request(void)
+static int process_request(struct serve_options *opts)
{
enum request_state state = PROCESS_REQUEST_KEYS;
struct packet_reader reader;
@@ -222,7 +223,7 @@ static int process_request(void)
if (!command)
die("no command requested");
- command->command(the_repository, &keys, &reader);
+ command->command(the_repository, opts->config_context, &keys, &reader);
argv_array_clear(&keys);
return 0;
@@ -249,10 +250,10 @@ void serve(struct serve_options *options)
* a single request/response exchange
*/
if (options->stateless_rpc) {
- process_request();
+ process_request(options);
} else {
for (;;)
- if (process_request())
+ if (process_request(options))
break;
}
}
diff --git a/serve.h b/serve.h
index fe65ba9f469..d527224cbb1 100644
--- a/serve.h
+++ b/serve.h
@@ -8,6 +8,13 @@ extern int has_capability(const struct argv_array *keys, const char *capability,
struct serve_options {
unsigned advertise_capabilities;
unsigned stateless_rpc;
+
+ /*
+ * Some operations may need to know the context when looking up config;
+ * e.g., set this to "uploadpack" to respect "uploadpack.hiderefs" (as
+ * opposed to "receive.hiderefs").
+ */
+ const char *config_context;
};
#define SERVE_OPTIONS_INIT { 0 }
extern void serve(struct serve_options *options);
diff --git a/upload-pack.c b/upload-pack.c
index 5e81f1ff24f..914bbb40bcd 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -1413,8 +1413,8 @@ enum fetch_state {
FETCH_DONE,
};
-int upload_pack_v2(struct repository *r, struct argv_array *keys,
- struct packet_reader *request)
+int upload_pack_v2(struct repository *r, const char *config_context,
+ struct argv_array *keys, struct packet_reader *request)
{
enum fetch_state state = FETCH_PROCESS_ARGS;
struct upload_pack_data data;
diff --git a/upload-pack.h b/upload-pack.h
index cab2178796a..2a9f51197c4 100644
--- a/upload-pack.h
+++ b/upload-pack.h
@@ -13,8 +13,8 @@ void upload_pack(struct upload_pack_options *options);
struct repository;
struct argv_array;
struct packet_reader;
-extern int upload_pack_v2(struct repository *r, struct argv_array *keys,
- struct packet_reader *request);
+extern int upload_pack_v2(struct repository *r, const char *config_context,
+ struct argv_array *keys, struct packet_reader *request);
struct strbuf;
extern int upload_pack_advertise(struct repository *r,
--
2.20.0.405.gbc1bbc6f85
^ permalink raw reply related [flat|nested] 73+ messages in thread
* [PATCH v3 2/4] parse_hide_refs_config: handle NULL section
2018-12-13 15:58 ` [PATCH v2 0/8] protocol v2 fixes Ævar Arnfjörð Bjarmason
2018-12-17 22:40 ` [PATCH v3 0/4] " Ævar Arnfjörð Bjarmason
2018-12-17 22:40 ` [PATCH v3 1/4] serve: pass "config context" through to individual commands Ævar Arnfjörð Bjarmason
@ 2018-12-17 22:40 ` Ævar Arnfjörð Bjarmason
2018-12-17 22:40 ` [PATCH v3 3/4] upload-pack: support hidden refs with protocol v2 Ævar Arnfjörð Bjarmason
2018-12-17 22:40 ` [PATCH v3 4/4] fetch-pack: support protocol version 2 Ævar Arnfjörð Bjarmason
4 siblings, 0 replies; 73+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-12-17 22:40 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Brandon Williams, Jonathan Tan,
Josh Steadmon, Ævar Arnfjörð Bjarmason
From: Jeff King <peff@peff.net>
This helper function looks for config in two places: transfer.hiderefs,
or $section.hiderefs, where $section is passed in by the caller (and is
"uploadpack" or "receive", depending on the context).
In preparation for callers which do not even have that context (namely
the "git-serve" command), let's handle a NULL by looking only at
transfer.hiderefs (as opposed to segfaulting).
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
refs.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index f9936355cda..099e91d9cc0 100644
--- a/refs.c
+++ b/refs.c
@@ -1267,7 +1267,8 @@ int parse_hide_refs_config(const char *var, const char *value, const char *secti
{
const char *key;
if (!strcmp("transfer.hiderefs", var) ||
- (!parse_config_key(var, section, NULL, NULL, &key) &&
+ (section &&
+ !parse_config_key(var, section, NULL, NULL, &key) &&
!strcmp(key, "hiderefs"))) {
char *ref;
int len;
--
2.20.0.405.gbc1bbc6f85
^ permalink raw reply related [flat|nested] 73+ messages in thread
* [PATCH v3 3/4] upload-pack: support hidden refs with protocol v2
2018-12-13 15:58 ` [PATCH v2 0/8] protocol v2 fixes Ævar Arnfjörð Bjarmason
` (2 preceding siblings ...)
2018-12-17 22:40 ` [PATCH v3 2/4] parse_hide_refs_config: handle NULL section Ævar Arnfjörð Bjarmason
@ 2018-12-17 22:40 ` Ævar Arnfjörð Bjarmason
2018-12-17 22:40 ` [PATCH v3 4/4] fetch-pack: support protocol version 2 Ævar Arnfjörð Bjarmason
4 siblings, 0 replies; 73+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-12-17 22:40 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Brandon Williams, Jonathan Tan,
Josh Steadmon, Ævar Arnfjörð Bjarmason
From: Jeff King <peff@peff.net>
In the v2 protocol, upload-pack's advertisement has been moved to the
"ls-refs" command. That command does not respect hidden-ref config (like
transfer.hiderefs) at all, and advertises everything.
While there are some features that are not supported in v2 (e.g., v2
always allows fetching any sha1 without respect to advertisements), the
lack of this feature is not documented and is likely just a bug. Let's
make it work, as otherwise upgrading a server to a v2-capable git will
start exposing these refs that the repository admin has asked to remain
hidden.
Note that we depend on the config_context from the caller here to
realize that we should respect uploadpack.hiderefs. When run from the
"git-serve" tool, we won't have that context and will only respect
transfer.hiderefs. This should be OK, as git-serve is not actually used
for normal protocol operations.
Note also that we don't need to worry about the "An attempt to update
or delete a hidden ref by git push is rejected" feature of
receive.hideRefs, see git-config(1). This is because there's no v2
push protocol yet.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
ls-refs.c | 12 ++++++++++++
t/t5512-ls-remote.sh | 6 ++++++
2 files changed, 18 insertions(+)
diff --git a/ls-refs.c b/ls-refs.c
index e8e31475f06..8a8143338b4 100644
--- a/ls-refs.c
+++ b/ls-refs.c
@@ -5,6 +5,7 @@
#include "argv-array.h"
#include "ls-refs.h"
#include "pkt-line.h"
+#include "config.h"
/*
* Check if one of the prefixes is a prefix of the ref.
@@ -40,6 +41,9 @@ static int send_ref(const char *refname, const struct object_id *oid,
const char *refname_nons = strip_namespace(refname);
struct strbuf refline = STRBUF_INIT;
+ if (ref_is_hidden(refname_nons, refname))
+ return 0;
+
if (!ref_match(&data->prefixes, refname))
return 0;
@@ -69,6 +73,12 @@ static int send_ref(const char *refname, const struct object_id *oid,
return 0;
}
+static int ls_refs_config(const char *var, const char *value,
+ void *config_context)
+{
+ return parse_hide_refs_config(var, value, config_context);
+}
+
int ls_refs(struct repository *r,
const char *config_section,
struct argv_array *keys,
@@ -78,6 +88,8 @@ int ls_refs(struct repository *r,
memset(&data, 0, sizeof(data));
+ git_config(ls_refs_config, (void *)config_section);
+
while (packet_reader_read(request) != PACKET_READ_FLUSH) {
const char *arg = request->line;
const char *out;
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 32e722db2ed..ca69636fd52 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -204,6 +204,12 @@ test_expect_success 'overrides work between mixed transfer/upload-pack hideRefs'
grep refs/tags/magic actual
'
+test_expect_success 'protocol v2 supports hiderefs' '
+ test_config uploadpack.hiderefs refs/tags &&
+ git -c protocol.version=2 ls-remote . >actual &&
+ ! grep refs/tags actual
+'
+
test_expect_success 'ls-remote --symref' '
git fetch origin &&
cat >expect <<-EOF &&
--
2.20.0.405.gbc1bbc6f85
^ permalink raw reply related [flat|nested] 73+ messages in thread
* [PATCH v3 4/4] fetch-pack: support protocol version 2
2018-12-13 15:58 ` [PATCH v2 0/8] protocol v2 fixes Ævar Arnfjörð Bjarmason
` (3 preceding siblings ...)
2018-12-17 22:40 ` [PATCH v3 3/4] upload-pack: support hidden refs with protocol v2 Ævar Arnfjörð Bjarmason
@ 2018-12-17 22:40 ` Ævar Arnfjörð Bjarmason
2019-01-08 19:45 ` Junio C Hamano
4 siblings, 1 reply; 73+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-12-17 22:40 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Brandon Williams, Jonathan Tan,
Josh Steadmon, Ævar Arnfjörð Bjarmason
From: Jonathan Tan <jonathantanmy@google.com>
When the scaffolding for protocol version 2 was initially added in
8f6982b4e1 ("protocol: introduce enum protocol_version value
protocol_v2", 2018-03-14). As seen in:
git log -p -G'support for protocol v2 not implemented yet' --full-diff --reverse v2.17.0..v2.20.0
Many of those scaffolding "die" placeholders were removed, but we
hadn't gotten around to fetch-pack yet.
The test here for "fetch refs from cmdline" is very minimal. There's
much better coverage when running the entire test suite under the WIP
GIT_TEST_PROTOCOL_VERSION=2 mode[1], we should ideally have better
coverage without needing to invoke a special test mode.
1. https://public-inbox.org/git/20181213155817.27666-1-avarab@gmail.com/
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/fetch-pack.c | 9 ++++++---
t/t5500-fetch-pack.sh | 22 +++++++++++++++-------
2 files changed, 21 insertions(+), 10 deletions(-)
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 63e69a58011..f6a513495ea 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -55,6 +55,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
struct oid_array shallow = OID_ARRAY_INIT;
struct string_list deepen_not = STRING_LIST_INIT_DUP;
struct packet_reader reader;
+ enum protocol_version version;
fetch_if_missing = 0;
@@ -219,9 +220,11 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
PACKET_READ_CHOMP_NEWLINE |
PACKET_READ_GENTLE_ON_EOF);
- switch (discover_version(&reader)) {
+ version = discover_version(&reader);
+ switch (version) {
case protocol_v2:
- die("support for protocol v2 not implemented yet");
+ get_remote_refs(fd[1], &reader, &ref, 0, NULL, NULL);
+ break;
case protocol_v1:
case protocol_v0:
get_remote_heads(&reader, &ref, 0, NULL, &shallow);
@@ -231,7 +234,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
}
ref = fetch_pack(&args, fd, conn, ref, dest, sought, nr_sought,
- &shallow, pack_lockfile_ptr, protocol_v0);
+ &shallow, pack_lockfile_ptr, version);
if (pack_lockfile) {
printf("lock %s\n", pack_lockfile);
fflush(stdout);
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 086f2c40f68..49c540b1e1d 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -439,15 +439,23 @@ test_expect_success 'setup tests for the --stdin parameter' '
) >input.dup
'
-test_expect_success 'fetch refs from cmdline' '
- (
- cd client &&
- git fetch-pack --no-progress .. $(cat ../input)
- ) >output &&
- cut -d " " -f 2 <output | sort >actual &&
- test_cmp expect actual
+test_expect_success 'setup fetch refs from cmdline v[12]' '
+ cp -r client client1 &&
+ cp -r client client2
'
+for version in '' 1 2
+do
+ test_expect_success "protocol.version=$version fetch refs from cmdline" "
+ (
+ cd client$version &&
+ GIT_TEST_PROTOCOL_VERSION=$version git fetch-pack --no-progress .. \$(cat ../input)
+ ) >output &&
+ cut -d ' ' -f 2 <output | sort >actual &&
+ test_cmp expect actual
+ "
+done
+
test_expect_success 'fetch refs from stdin' '
(
cd client &&
--
2.20.0.405.gbc1bbc6f85
^ permalink raw reply related [flat|nested] 73+ messages in thread
* Re: [PATCH v3 4/4] fetch-pack: support protocol version 2
2018-12-17 22:40 ` [PATCH v3 4/4] fetch-pack: support protocol version 2 Ævar Arnfjörð Bjarmason
@ 2019-01-08 19:45 ` Junio C Hamano
2019-01-08 20:38 ` Jonathan Tan
0 siblings, 1 reply; 73+ messages in thread
From: Junio C Hamano @ 2019-01-08 19:45 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Jeff King, Brandon Williams, Jonathan Tan, Josh Steadmon
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> From: Jonathan Tan <jonathantanmy@google.com>
I was looking at the topics in 'pu' and noticed that I had v2 of
this series, wanted to update to v3, but major part of it was
superseded by another topic (jk/proto-v2-hidden-refs-fix). That
leaves only this patch in the v3 of this series.
Is this one still relevant?
> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
> index 086f2c40f68..49c540b1e1d 100755
> --- a/t/t5500-fetch-pack.sh
> +++ b/t/t5500-fetch-pack.sh
> @@ -439,15 +439,23 @@ test_expect_success 'setup tests for the --stdin parameter' '
> ) >input.dup
> '
>
> -test_expect_success 'fetch refs from cmdline' '
> - (
> - cd client &&
> - git fetch-pack --no-progress .. $(cat ../input)
> - ) >output &&
> - cut -d " " -f 2 <output | sort >actual &&
> - test_cmp expect actual
> +test_expect_success 'setup fetch refs from cmdline v[12]' '
> + cp -r client client1 &&
> + cp -r client client2
> '
>
> +for version in '' 1 2
> +do
> + test_expect_success "protocol.version=$version fetch refs from cmdline" "
> + (
> + cd client$version &&
> + GIT_TEST_PROTOCOL_VERSION=$version git fetch-pack --no-progress .. \$(cat ../input)
> + ) >output &&
> + cut -d ' ' -f 2 <output | sort >actual &&
> + test_cmp expect actual
> + "
> +done
> +
> test_expect_success 'fetch refs from stdin' '
> (
> cd client &&
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v3 4/4] fetch-pack: support protocol version 2
2019-01-08 19:45 ` Junio C Hamano
@ 2019-01-08 20:38 ` Jonathan Tan
2019-01-08 21:14 ` Jeff King
0 siblings, 1 reply; 73+ messages in thread
From: Jonathan Tan @ 2019-01-08 20:38 UTC (permalink / raw)
To: gitster; +Cc: avarab, git, peff, bwilliamseng, jonathantanmy, steadmon
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
> > From: Jonathan Tan <jonathantanmy@google.com>
>
>
> I was looking at the topics in 'pu' and noticed that I had v2 of
> this series, wanted to update to v3, but major part of it was
> superseded by another topic (jk/proto-v2-hidden-refs-fix). That
> leaves only this patch in the v3 of this series.
>
> Is this one still relevant?
This patch is more relevant to the GIT_TEST_PROTOCOL_VERSION patches,
since it means that several tests work even if
GIT_TEST_PROTOCOL_VERSION=2 is set. I think it can be dropped until
someone restarts the GIT_TEST_PROTOCOL_VERSION effort, but I'm not sure
if Ævar has another opinion.
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v3 4/4] fetch-pack: support protocol version 2
2019-01-08 20:38 ` Jonathan Tan
@ 2019-01-08 21:14 ` Jeff King
0 siblings, 0 replies; 73+ messages in thread
From: Jeff King @ 2019-01-08 21:14 UTC (permalink / raw)
To: Jonathan Tan; +Cc: gitster, avarab, git, bwilliamseng, steadmon
On Tue, Jan 08, 2019 at 12:38:26PM -0800, Jonathan Tan wrote:
> > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> >
> > > From: Jonathan Tan <jonathantanmy@google.com>
> >
> >
> > I was looking at the topics in 'pu' and noticed that I had v2 of
> > this series, wanted to update to v3, but major part of it was
> > superseded by another topic (jk/proto-v2-hidden-refs-fix). That
> > leaves only this patch in the v3 of this series.
> >
> > Is this one still relevant?
>
> This patch is more relevant to the GIT_TEST_PROTOCOL_VERSION patches,
> since it means that several tests work even if
> GIT_TEST_PROTOCOL_VERSION=2 is set. I think it can be dropped until
> someone restarts the GIT_TEST_PROTOCOL_VERSION effort, but I'm not sure
> if Ævar has another opinion.
I think it's independently useful. If you set protocol.version=2 in your
config, then you can no longer run fetch-pack directly. Most people
don't, but it's possible their scripts might (we also use fetch-pack
under the hood for smart-http, but I wasn't able to get it to complain,
though).
-Peff
^ permalink raw reply [flat|nested] 73+ messages in thread
* [PATCH v2 1/8] serve: pass "config context" through to individual commands
2018-12-13 2:49 ` [PATCH 0/3] Some fixes and improvements Junio C Hamano
2018-12-13 15:58 ` [PATCH v2 0/8] protocol v2 fixes Ævar Arnfjörð Bjarmason
@ 2018-12-13 15:58 ` Ævar Arnfjörð Bjarmason
2018-12-13 15:58 ` [PATCH v2 2/8] parse_hide_refs_config: handle NULL section Ævar Arnfjörð Bjarmason
` (6 subsequent siblings)
8 siblings, 0 replies; 73+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-12-13 15:58 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Brandon Williams, Jonathan Tan,
Ævar Arnfjörð Bjarmason
From: Jeff King <peff@peff.net>
In protocol v2, instead of just running "upload-pack", we have a generic
"serve" loop which runs command requests from the client. What used to
be "upload-pack" is now generally split into two operations: "ls-refs"
and "fetch". The latter knows it must respect uploadpack.* config, but
the former is not actually specific to a fetch operation (we do not yet
do v2 receive-pack, but eventually we may, and ls-refs would support
both operations).
However, ls-refs does need to know which operation we're performing, in
order to read the correct config (e.g., uploadpack.hiderefs versus
receive.hiderefs; we don't read _either_ right now, but this is the
first step to fixing that).
In the generic "git-serve" program, we don't have that information. But
in the protocol as it is actually used by clients, the client still asks
to run "git-upload-pack", and then issues an "ls-refs" from there. So we
_do_ know at that point that "uploadpack" is the right config context.
This patch teaches the protocol v2 "serve" code to pass that context
through to the individual commands (a future patch will teach ls-refs to
actually use it).
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/upload-pack.c | 1 +
ls-refs.c | 4 +++-
ls-refs.h | 3 ++-
serve.c | 9 +++++----
serve.h | 7 +++++++
upload-pack.c | 4 ++--
upload-pack.h | 4 ++--
7 files changed, 22 insertions(+), 10 deletions(-)
diff --git a/builtin/upload-pack.c b/builtin/upload-pack.c
index 42dc4da5a1..67192cfa9e 100644
--- a/builtin/upload-pack.c
+++ b/builtin/upload-pack.c
@@ -52,6 +52,7 @@ int cmd_upload_pack(int argc, const char **argv, const char *prefix)
case protocol_v2:
serve_opts.advertise_capabilities = opts.advertise_refs;
serve_opts.stateless_rpc = opts.stateless_rpc;
+ serve_opts.config_context = "uploadpack";
serve(&serve_opts);
break;
case protocol_v1:
diff --git a/ls-refs.c b/ls-refs.c
index a06f12eca8..e8e31475f0 100644
--- a/ls-refs.c
+++ b/ls-refs.c
@@ -69,7 +69,9 @@ static int send_ref(const char *refname, const struct object_id *oid,
return 0;
}
-int ls_refs(struct repository *r, struct argv_array *keys,
+int ls_refs(struct repository *r,
+ const char *config_section,
+ struct argv_array *keys,
struct packet_reader *request)
{
struct ls_refs_data data;
diff --git a/ls-refs.h b/ls-refs.h
index b62877e8da..da26fc9824 100644
--- a/ls-refs.h
+++ b/ls-refs.h
@@ -4,7 +4,8 @@
struct repository;
struct argv_array;
struct packet_reader;
-extern int ls_refs(struct repository *r, struct argv_array *keys,
+extern int ls_refs(struct repository *r, const char *config_context,
+ struct argv_array *keys,
struct packet_reader *request);
#endif /* LS_REFS_H */
diff --git a/serve.c b/serve.c
index bda085f09c..70f89cf0d9 100644
--- a/serve.c
+++ b/serve.c
@@ -48,6 +48,7 @@ struct protocol_capability {
* This field should be NULL for capabilities which are not commands.
*/
int (*command)(struct repository *r,
+ const char *config_context,
struct argv_array *keys,
struct packet_reader *request);
};
@@ -158,7 +159,7 @@ enum request_state {
PROCESS_REQUEST_DONE,
};
-static int process_request(void)
+static int process_request(struct serve_options *opts)
{
enum request_state state = PROCESS_REQUEST_KEYS;
struct packet_reader reader;
@@ -222,7 +223,7 @@ static int process_request(void)
if (!command)
die("no command requested");
- command->command(the_repository, &keys, &reader);
+ command->command(the_repository, opts->config_context, &keys, &reader);
argv_array_clear(&keys);
return 0;
@@ -249,10 +250,10 @@ void serve(struct serve_options *options)
* a single request/response exchange
*/
if (options->stateless_rpc) {
- process_request();
+ process_request(options);
} else {
for (;;)
- if (process_request())
+ if (process_request(options))
break;
}
}
diff --git a/serve.h b/serve.h
index fe65ba9f46..d527224cbb 100644
--- a/serve.h
+++ b/serve.h
@@ -8,6 +8,13 @@ extern int has_capability(const struct argv_array *keys, const char *capability,
struct serve_options {
unsigned advertise_capabilities;
unsigned stateless_rpc;
+
+ /*
+ * Some operations may need to know the context when looking up config;
+ * e.g., set this to "uploadpack" to respect "uploadpack.hiderefs" (as
+ * opposed to "receive.hiderefs").
+ */
+ const char *config_context;
};
#define SERVE_OPTIONS_INIT { 0 }
extern void serve(struct serve_options *options);
diff --git a/upload-pack.c b/upload-pack.c
index 5e81f1ff24..914bbb40bc 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -1413,8 +1413,8 @@ enum fetch_state {
FETCH_DONE,
};
-int upload_pack_v2(struct repository *r, struct argv_array *keys,
- struct packet_reader *request)
+int upload_pack_v2(struct repository *r, const char *config_context,
+ struct argv_array *keys, struct packet_reader *request)
{
enum fetch_state state = FETCH_PROCESS_ARGS;
struct upload_pack_data data;
diff --git a/upload-pack.h b/upload-pack.h
index cab2178796..2a9f51197c 100644
--- a/upload-pack.h
+++ b/upload-pack.h
@@ -13,8 +13,8 @@ void upload_pack(struct upload_pack_options *options);
struct repository;
struct argv_array;
struct packet_reader;
-extern int upload_pack_v2(struct repository *r, struct argv_array *keys,
- struct packet_reader *request);
+extern int upload_pack_v2(struct repository *r, const char *config_context,
+ struct argv_array *keys, struct packet_reader *request);
struct strbuf;
extern int upload_pack_advertise(struct repository *r,
--
2.20.0.405.gbc1bbc6f85
^ permalink raw reply related [flat|nested] 73+ messages in thread
* [PATCH v2 2/8] parse_hide_refs_config: handle NULL section
2018-12-13 2:49 ` [PATCH 0/3] Some fixes and improvements Junio C Hamano
2018-12-13 15:58 ` [PATCH v2 0/8] protocol v2 fixes Ævar Arnfjörð Bjarmason
2018-12-13 15:58 ` [PATCH v2 1/8] serve: pass "config context" through to individual commands Ævar Arnfjörð Bjarmason
@ 2018-12-13 15:58 ` Ævar Arnfjörð Bjarmason
2018-12-13 15:58 ` [PATCH v2 3/8] upload-pack: support hidden refs with protocol v2 Ævar Arnfjörð Bjarmason
` (5 subsequent siblings)
8 siblings, 0 replies; 73+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-12-13 15:58 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Brandon Williams, Jonathan Tan,
Ævar Arnfjörð Bjarmason
From: Jeff King <peff@peff.net>
This helper function looks for config in two places: transfer.hiderefs,
or $section.hiderefs, where $section is passed in by the caller (and is
"uploadpack" or "receive", depending on the context).
In preparation for callers which do not even have that context (namely
the "git-serve" command), let's handle a NULL by looking only at
transfer.hiderefs (as opposed to segfaulting).
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
refs.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index f9936355cd..099e91d9cc 100644
--- a/refs.c
+++ b/refs.c
@@ -1267,7 +1267,8 @@ int parse_hide_refs_config(const char *var, const char *value, const char *secti
{
const char *key;
if (!strcmp("transfer.hiderefs", var) ||
- (!parse_config_key(var, section, NULL, NULL, &key) &&
+ (section &&
+ !parse_config_key(var, section, NULL, NULL, &key) &&
!strcmp(key, "hiderefs"))) {
char *ref;
int len;
--
2.20.0.405.gbc1bbc6f85
^ permalink raw reply related [flat|nested] 73+ messages in thread
* [PATCH v2 3/8] upload-pack: support hidden refs with protocol v2
2018-12-13 2:49 ` [PATCH 0/3] Some fixes and improvements Junio C Hamano
` (2 preceding siblings ...)
2018-12-13 15:58 ` [PATCH v2 2/8] parse_hide_refs_config: handle NULL section Ævar Arnfjörð Bjarmason
@ 2018-12-13 15:58 ` Ævar Arnfjörð Bjarmason
2018-12-13 15:58 ` [PATCH v2 4/8] tests: add a check for unportable env --unset Ævar Arnfjörð Bjarmason
` (4 subsequent siblings)
8 siblings, 0 replies; 73+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-12-13 15:58 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Brandon Williams, Jonathan Tan,
Ævar Arnfjörð Bjarmason
From: Jeff King <peff@peff.net>
In the v2 protocol, upload-pack's advertisement has been moved to the
"ls-refs" command. That command does not respect hidden-ref config (like
transfer.hiderefs) at all, and advertises everything.
While there are some features that are not supported in v2 (e.g., v2
always allows fetching any sha1 without respect to advertisements), the
lack of this feature is not documented and is likely just a bug. Let's
make it work, as otherwise upgrading a server to a v2-capable git will
start exposing these refs that the repository admin has asked to remain
hidden.
Note that we depend on the config_context from the caller here to
realize that we should respect uploadpack.hiderefs. When run from the
"git-serve" tool, we won't have that context and will only respect
transfer.hiderefs. This should be OK, as git-serve is not actually used
for normal protocol operations.
Note also that we don't need to worry about the "An attempt to update
or delete a hidden ref by git push is rejected" feature of
receive.hideRefs, see git-config(1). This is because there's no v2
push protocol yet.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
ls-refs.c | 12 ++++++++++++
t/t5512-ls-remote.sh | 6 ++++++
2 files changed, 18 insertions(+)
diff --git a/ls-refs.c b/ls-refs.c
index e8e31475f0..8a8143338b 100644
--- a/ls-refs.c
+++ b/ls-refs.c
@@ -5,6 +5,7 @@
#include "argv-array.h"
#include "ls-refs.h"
#include "pkt-line.h"
+#include "config.h"
/*
* Check if one of the prefixes is a prefix of the ref.
@@ -40,6 +41,9 @@ static int send_ref(const char *refname, const struct object_id *oid,
const char *refname_nons = strip_namespace(refname);
struct strbuf refline = STRBUF_INIT;
+ if (ref_is_hidden(refname_nons, refname))
+ return 0;
+
if (!ref_match(&data->prefixes, refname))
return 0;
@@ -69,6 +73,12 @@ static int send_ref(const char *refname, const struct object_id *oid,
return 0;
}
+static int ls_refs_config(const char *var, const char *value,
+ void *config_context)
+{
+ return parse_hide_refs_config(var, value, config_context);
+}
+
int ls_refs(struct repository *r,
const char *config_section,
struct argv_array *keys,
@@ -78,6 +88,8 @@ int ls_refs(struct repository *r,
memset(&data, 0, sizeof(data));
+ git_config(ls_refs_config, (void *)config_section);
+
while (packet_reader_read(request) != PACKET_READ_FLUSH) {
const char *arg = request->line;
const char *out;
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 32e722db2e..ca69636fd5 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -204,6 +204,12 @@ test_expect_success 'overrides work between mixed transfer/upload-pack hideRefs'
grep refs/tags/magic actual
'
+test_expect_success 'protocol v2 supports hiderefs' '
+ test_config uploadpack.hiderefs refs/tags &&
+ git -c protocol.version=2 ls-remote . >actual &&
+ ! grep refs/tags actual
+'
+
test_expect_success 'ls-remote --symref' '
git fetch origin &&
cat >expect <<-EOF &&
--
2.20.0.405.gbc1bbc6f85
^ permalink raw reply related [flat|nested] 73+ messages in thread
* [PATCH v2 4/8] tests: add a check for unportable env --unset
2018-12-13 2:49 ` [PATCH 0/3] Some fixes and improvements Junio C Hamano
` (3 preceding siblings ...)
2018-12-13 15:58 ` [PATCH v2 3/8] upload-pack: support hidden refs with protocol v2 Ævar Arnfjörð Bjarmason
@ 2018-12-13 15:58 ` Ævar Arnfjörð Bjarmason
2018-12-13 15:58 ` [PATCH v2 5/8] tests: add a special setup where for protocol.version Ævar Arnfjörð Bjarmason
` (3 subsequent siblings)
8 siblings, 0 replies; 73+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-12-13 15:58 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Brandon Williams, Jonathan Tan,
Ævar Arnfjörð Bjarmason
The "env --unset=*" argument isn't portable. Neither Solaris or AIX
have it, and probably not a bunch of other POSIX-like OS's.
Using this was suggested on-list[1]. Let's add a check for it so it
doesn't sneak into the codebase in the future.
1. https://public-inbox.org/git/cover.1544573604.git.jonathantanmy@google.com/
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
t/check-non-portable-shell.pl | 1 +
1 file changed, 1 insertion(+)
diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl
index b45bdac688..1cfb4608ee 100755
--- a/t/check-non-portable-shell.pl
+++ b/t/check-non-portable-shell.pl
@@ -35,6 +35,7 @@ sub err {
chomp;
}
+ /\benv (?:-u|--unset)\b/ and err 'env [-u|--unset] is not portable';
/\bsed\s+-i/ and err 'sed -i is not portable';
/\becho\s+-[neE]/ and err 'echo with option is not portable (use printf)';
/^\s*declare\s+/ and err 'arrays/declare not portable';
--
2.20.0.405.gbc1bbc6f85
^ permalink raw reply related [flat|nested] 73+ messages in thread
* [PATCH v2 5/8] tests: add a special setup where for protocol.version
2018-12-13 2:49 ` [PATCH 0/3] Some fixes and improvements Junio C Hamano
` (4 preceding siblings ...)
2018-12-13 15:58 ` [PATCH v2 4/8] tests: add a check for unportable env --unset Ævar Arnfjörð Bjarmason
@ 2018-12-13 15:58 ` Ævar Arnfjörð Bjarmason
2018-12-13 19:48 ` Jonathan Tan
2018-12-13 15:58 ` [PATCH v2 6/8] tests: mark & fix tests broken under GIT_TEST_PROTOCOL_VERSION=1 Ævar Arnfjörð Bjarmason
` (2 subsequent siblings)
8 siblings, 1 reply; 73+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-12-13 15:58 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Brandon Williams, Jonathan Tan,
Ævar Arnfjörð Bjarmason
Add a GIT_TEST_PROTOCOL_VERSION=X test mode which is equivalent to
running with protocol.version=X. This is needed to spot regressions
and differences such as "ls-refs" behaving differently with
transfer.hideRefs. See
https://public-inbox.org/git/20181211104236.GA6899@sigill.intra.peff.net/
for a fix for that regression.
With this all tests pass with GIT_TEST_PROTOCOL_VERSION=0, but fail
with GIT_TEST_PROTOCOL_VERSION=[1|2]. That's OK since this is a new
test mode, subsequent patches will fix up these test failures.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
protocol.c | 13 ++++++++++++-
t/README | 6 ++++++
t/t0410-partial-clone.sh | 3 ++-
t/t5516-fetch-push.sh | 3 ++-
t/t5700-protocol-v1.sh | 1 +
t/t5702-protocol-v2.sh | 1 +
6 files changed, 24 insertions(+), 3 deletions(-)
diff --git a/protocol.c b/protocol.c
index 5e636785d1..aa06c3b5bc 100644
--- a/protocol.c
+++ b/protocol.c
@@ -17,7 +17,18 @@ static enum protocol_version parse_protocol_version(const char *value)
enum protocol_version get_protocol_version_config(void)
{
const char *value;
- if (!git_config_get_string_const("protocol.version", &value)) {
+ const char *git_test_k = "GIT_TEST_PROTOCOL_VERSION";
+ const char *git_test_v = getenv(git_test_k);
+
+ if (git_test_v && strlen(git_test_v)) {
+ enum protocol_version version = parse_protocol_version(git_test_v);
+
+ if (version == protocol_unknown_version)
+ die("unknown value for %s: %s", git_test_k,
+ git_test_v);
+
+ return version;
+ } else if (!git_config_get_string_const("protocol.version", &value)) {
enum protocol_version version = parse_protocol_version(value);
if (version == protocol_unknown_version)
diff --git a/t/README b/t/README
index 28711cc508..89629c5818 100644
--- a/t/README
+++ b/t/README
@@ -311,6 +311,12 @@ marked strings" in po/README for details.
GIT_TEST_SPLIT_INDEX=<boolean> forces split-index mode on the whole
test suite. Accept any boolean values that are accepted by git-config.
+GIT_TEST_PROTOCOL_VERSION=<'protocol.version' config value>, when set,
+runs the test suite with the given protocol.version. E.g. "0", "1" or
+"2". Can be set to the empty string within tests themselves (e.g. "env
+GIT_TEST_PROTOCOL_VERSION= <cmd>") to unset the value in the
+environment as a workaround for "env --unset" not being portable.
+
GIT_TEST_FULL_IN_PACK_ARRAY=<boolean> exercises the uncommon
pack-objects code path where there are more than 1024 packs even if
the actual number of packs in repository is below this limit. Accept
diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
index ba3887f178..8ba3d9b5ab 100755
--- a/t/t0410-partial-clone.sh
+++ b/t/t0410-partial-clone.sh
@@ -178,7 +178,8 @@ test_expect_success 'fetching of missing objects works with ref-in-want enabled'
rm -rf repo/.git/objects/* &&
rm -f trace &&
- GIT_TRACE_PACKET="$(pwd)/trace" git -C repo cat-file -p "$HASH" &&
+ GIT_TRACE_PACKET="$(pwd)/trace" env GIT_TEST_PROTOCOL_VERSION= \
+ git -C repo cat-file -p "$HASH" &&
grep "git< fetch=.*ref-in-want" trace
'
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 37e8e80893..08cdee0b95 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1187,7 +1187,8 @@ test_expect_success 'fetch exact SHA1 in protocol v2' '
# fetching the hidden object succeeds by default
# NEEDSWORK: should this match the v0 behavior instead?
- git -C child fetch -v ../testrepo $the_commit:refs/heads/copy
+ env GIT_TEST_PROTOCOL_VERSION= \
+ git -C child fetch -v ../testrepo $the_commit:refs/heads/copy
'
for configallowtipsha1inwant in true false
diff --git a/t/t5700-protocol-v1.sh b/t/t5700-protocol-v1.sh
index ba86a44eb1..e4d375c462 100755
--- a/t/t5700-protocol-v1.sh
+++ b/t/t5700-protocol-v1.sh
@@ -8,6 +8,7 @@ TEST_NO_CREATE_REPO=1
# Test protocol v1 with 'git://' transport
#
+unset GIT_TEST_PROTOCOL_VERSION
. "$TEST_DIRECTORY"/lib-git-daemon.sh
start_git_daemon --export-all --enable=receive-pack
daemon_parent=$GIT_DAEMON_DOCUMENT_ROOT_PATH/parent
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 0f2b09ebb8..d1549f294e 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -8,6 +8,7 @@ TEST_NO_CREATE_REPO=1
# Test protocol v2 with 'git://' transport
#
+unset GIT_TEST_PROTOCOL_VERSION
. "$TEST_DIRECTORY"/lib-git-daemon.sh
start_git_daemon --export-all --enable=receive-pack
daemon_parent=$GIT_DAEMON_DOCUMENT_ROOT_PATH/parent
--
2.20.0.405.gbc1bbc6f85
^ permalink raw reply related [flat|nested] 73+ messages in thread
* Re: [PATCH v2 5/8] tests: add a special setup where for protocol.version
2018-12-13 15:58 ` [PATCH v2 5/8] tests: add a special setup where for protocol.version Ævar Arnfjörð Bjarmason
@ 2018-12-13 19:48 ` Jonathan Tan
0 siblings, 0 replies; 73+ messages in thread
From: Jonathan Tan @ 2018-12-13 19:48 UTC (permalink / raw)
To: avarab; +Cc: git, gitster, peff, bwilliamseng, jonathantanmy
Regarding the subject, I think you mean "add a special setup var", not
"add a special setup where".
> +GIT_TEST_PROTOCOL_VERSION=<'protocol.version' config value>, when set,
> +runs the test suite with the given protocol.version. E.g. "0", "1" or
> +"2". Can be set to the empty string within tests themselves (e.g. "env
> +GIT_TEST_PROTOCOL_VERSION= <cmd>") to unset the value in the
> +environment as a workaround for "env --unset" not being portable.
Optional: I prefer the "overrides" language used in
GIT_TEST_COMMIT_GRAPH to make it clear that any existing setting is
ignored. E.g.:
GIT_TEST_PROTOCOL_VERSION=<'protocol.version' config value>, when set
to a non-empty string, overrides the 'protocol.version' value. E.g.
"0", ...
Other than that, this patch and the other patches (1-4, 6, 8) look good
to me. My patch 7 looks good too, but it's probably better if someone
else looks at it too :-)
^ permalink raw reply [flat|nested] 73+ messages in thread
* [PATCH v2 6/8] tests: mark & fix tests broken under GIT_TEST_PROTOCOL_VERSION=1
2018-12-13 2:49 ` [PATCH 0/3] Some fixes and improvements Junio C Hamano
` (5 preceding siblings ...)
2018-12-13 15:58 ` [PATCH v2 5/8] tests: add a special setup where for protocol.version Ævar Arnfjörð Bjarmason
@ 2018-12-13 15:58 ` Ævar Arnfjörð Bjarmason
2018-12-13 15:58 ` [PATCH v2 7/8] builtin/fetch-pack: support protocol version 2 Ævar Arnfjörð Bjarmason
2018-12-13 15:58 ` [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2 Ævar Arnfjörð Bjarmason
8 siblings, 0 replies; 73+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-12-13 15:58 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Brandon Williams, Jonathan Tan,
Ævar Arnfjörð Bjarmason
There's one t5400-send-pack.sh test which is broken under
GIT_TEST_PROTOCOL_VERSION=1. It's easier to just coerce it to run
under protocol 1. The difference in the output is that there'll be a
"version 1" line which'll make the subsequent "test_cmp". See
protocol.version in git-config(1) which notes that "1" is just "0"
with a version number. the trace output will include fail.
Similarly in t5601-clone.sh we'll be passing an option to ssh, but
since so many tests would fail in this file let's go above & beyond
and make them pass by only testing the relevant part of the output.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
t/t5400-send-pack.sh | 2 +-
t/t5601-clone.sh | 11 +++++++++--
2 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh
index f1932ea431..571d620aed 100755
--- a/t/t5400-send-pack.sh
+++ b/t/t5400-send-pack.sh
@@ -288,7 +288,7 @@ test_expect_success 'receive-pack de-dupes .have lines' '
$shared .have
EOF
- GIT_TRACE_PACKET=$(pwd)/trace \
+ GIT_TRACE_PACKET=$(pwd)/trace GIT_TEST_PROTOCOL_VERSION= \
git push \
--receive-pack="unset GIT_TRACE_PACKET; git-receive-pack" \
fork HEAD:foo &&
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 8bbc7068ac..a7b7ab327c 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -325,7 +325,7 @@ copy_ssh_wrapper_as () {
expect_ssh () {
test_when_finished '
- (cd "$TRASH_DIRECTORY" && rm -f ssh-expect && >ssh-output)
+ (cd "$TRASH_DIRECTORY" && rm -f ssh-expect ssh-output.munged && >ssh-output)
' &&
{
case "$#" in
@@ -341,7 +341,14 @@ expect_ssh () {
echo "ssh: $1 $2 git-upload-pack '$3' $4"
esac
} >"$TRASH_DIRECTORY/ssh-expect" &&
- (cd "$TRASH_DIRECTORY" && test_cmp ssh-expect ssh-output)
+ (
+ cd "$TRASH_DIRECTORY" &&
+ # We don't care about this trivial difference in
+ # output with GIT_TEST_PROTOCOL_VERSION=[12]
+ sed 's/ssh: -o SendEnv=GIT_PROTOCOL /ssh: /' <ssh-output >ssh-output.munged &&
+ mv ssh-output.munged ssh-output &&
+ test_cmp ssh-expect ssh-output
+ )
}
test_expect_success 'clone myhost:src uses ssh' '
--
2.20.0.405.gbc1bbc6f85
^ permalink raw reply related [flat|nested] 73+ messages in thread
* [PATCH v2 7/8] builtin/fetch-pack: support protocol version 2
2018-12-13 2:49 ` [PATCH 0/3] Some fixes and improvements Junio C Hamano
` (6 preceding siblings ...)
2018-12-13 15:58 ` [PATCH v2 6/8] tests: mark & fix tests broken under GIT_TEST_PROTOCOL_VERSION=1 Ævar Arnfjörð Bjarmason
@ 2018-12-13 15:58 ` Ævar Arnfjörð Bjarmason
2018-12-14 10:17 ` Jeff King
2018-12-13 15:58 ` [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2 Ævar Arnfjörð Bjarmason
8 siblings, 1 reply; 73+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-12-13 15:58 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Brandon Williams, Jonathan Tan,
Ævar Arnfjörð Bjarmason
From: Jonathan Tan <jonathantanmy@google.com>
Currently, if support for running Git's entire test suite with protocol
v2 were added, some tests would fail because the fetch-pack CLI command
dies if it encounters protocol v2. To avoid this, teach fetch-pack
support for protocol v2.
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/fetch-pack.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 63e69a5801..f6a513495e 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -55,6 +55,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
struct oid_array shallow = OID_ARRAY_INIT;
struct string_list deepen_not = STRING_LIST_INIT_DUP;
struct packet_reader reader;
+ enum protocol_version version;
fetch_if_missing = 0;
@@ -219,9 +220,11 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
PACKET_READ_CHOMP_NEWLINE |
PACKET_READ_GENTLE_ON_EOF);
- switch (discover_version(&reader)) {
+ version = discover_version(&reader);
+ switch (version) {
case protocol_v2:
- die("support for protocol v2 not implemented yet");
+ get_remote_refs(fd[1], &reader, &ref, 0, NULL, NULL);
+ break;
case protocol_v1:
case protocol_v0:
get_remote_heads(&reader, &ref, 0, NULL, &shallow);
@@ -231,7 +234,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
}
ref = fetch_pack(&args, fd, conn, ref, dest, sought, nr_sought,
- &shallow, pack_lockfile_ptr, protocol_v0);
+ &shallow, pack_lockfile_ptr, version);
if (pack_lockfile) {
printf("lock %s\n", pack_lockfile);
fflush(stdout);
--
2.20.0.405.gbc1bbc6f85
^ permalink raw reply related [flat|nested] 73+ messages in thread
* Re: [PATCH v2 7/8] builtin/fetch-pack: support protocol version 2
2018-12-13 15:58 ` [PATCH v2 7/8] builtin/fetch-pack: support protocol version 2 Ævar Arnfjörð Bjarmason
@ 2018-12-14 10:17 ` Jeff King
0 siblings, 0 replies; 73+ messages in thread
From: Jeff King @ 2018-12-14 10:17 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, Brandon Williams, Jonathan Tan
On Thu, Dec 13, 2018 at 04:58:16PM +0100, Ævar Arnfjörð Bjarmason wrote:
> From: Jonathan Tan <jonathantanmy@google.com>
>
> Currently, if support for running Git's entire test suite with protocol
> v2 were added, some tests would fail because the fetch-pack CLI command
> dies if it encounters protocol v2. To avoid this, teach fetch-pack
> support for protocol v2.
I'm definitely on board with this goal.
> @@ -219,9 +220,11 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
> PACKET_READ_CHOMP_NEWLINE |
> PACKET_READ_GENTLE_ON_EOF);
>
> - switch (discover_version(&reader)) {
> + version = discover_version(&reader);
> + switch (version) {
> case protocol_v2:
> - die("support for protocol v2 not implemented yet");
> + get_remote_refs(fd[1], &reader, &ref, 0, NULL, NULL);
> + break;
> case protocol_v1:
> case protocol_v0:
> get_remote_heads(&reader, &ref, 0, NULL, &shallow);
> @@ -231,7 +234,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
> }
>
> ref = fetch_pack(&args, fd, conn, ref, dest, sought, nr_sought,
> - &shallow, pack_lockfile_ptr, protocol_v0);
> + &shallow, pack_lockfile_ptr, version);
This implementation looks absurdly simple. So simple that it makes me
wonder why we did not just do this in the first place. I.e., is there
some hidden gotcha blocking it, or was it simply that all of the
necessary work happened in the meantime and nobody bothered to flip the
switch?
-Peff
^ permalink raw reply [flat|nested] 73+ messages in thread
* [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2
2018-12-13 2:49 ` [PATCH 0/3] Some fixes and improvements Junio C Hamano
` (7 preceding siblings ...)
2018-12-13 15:58 ` [PATCH v2 7/8] builtin/fetch-pack: support protocol version 2 Ævar Arnfjörð Bjarmason
@ 2018-12-13 15:58 ` Ævar Arnfjörð Bjarmason
2018-12-13 16:08 ` Ævar Arnfjörð Bjarmason
8 siblings, 1 reply; 73+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-12-13 15:58 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Brandon Williams, Jonathan Tan,
Ævar Arnfjörð Bjarmason
Mark those tests that have behavior differences or bugs under
protocol.version=2.
Whether or not these tests should exhibit different behavior is
outside the scope of this change. Some (such as t5500-fetch-pack.sh)
are intentionally different, but others (such as
t7406-submodule-update.sh and t5515-fetch-merge-logic.sh) might
indicate bugs in the protocol v2 code.
Tracking down which is which is outside the scope of this
change. Let's first exhaustively annotate where the differences are,
so that we can spot future behavior differences or regressions.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
t/t5500-fetch-pack.sh | 9 ++++++---
t/t5503-tagfollow.sh | 8 ++++----
t/t5512-ls-remote.sh | 8 ++++----
t/t5515-fetch-merge-logic.sh | 2 +-
t/t5516-fetch-push.sh | 17 +++++++++++------
t/t5537-fetch-shallow.sh | 3 ++-
t/t5539-fetch-http-shallow.sh | 9 +++++----
t/t5541-http-push-smart.sh | 9 +++++++--
t/t5551-http-fetch-smart.sh | 19 +++++++++++--------
t/t5552-skipping-fetch-negotiator.sh | 4 ++--
t/t5570-git-daemon.sh | 2 +-
t/t7406-submodule-update.sh | 3 ++-
12 files changed, 56 insertions(+), 37 deletions(-)
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 086f2c40f6..8b1217ea26 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -628,7 +628,10 @@ test_expect_success 'fetch-pack cannot fetch a raw sha1 that is not advertised a
test_commit -C server 6 &&
git init client &&
- test_must_fail git -C client fetch-pack ../server \
+
+ # Other protocol versions (e.g. 2) allow fetching an unadvertised
+ # object, so run this test with the default protocol version (0).
+ test_must_fail env GIT_TEST_PROTOCOL_VERSION= git -C client fetch-pack ../server \
$(git -C server rev-parse refs/heads/master^) 2>err &&
test_i18ngrep "Server does not allow request for unadvertised object" err
'
@@ -788,7 +791,7 @@ test_expect_success 'shallow clone exclude tag two' '
'
test_expect_success 'fetch exclude tag one' '
- git -C shallow12 fetch --shallow-exclude one origin &&
+ GIT_TEST_PROTOCOL_VERSION= git -C shallow12 fetch --shallow-exclude one origin &&
git -C shallow12 log --pretty=tformat:%s origin/master >actual &&
test_write_lines three two >expected &&
test_cmp expected actual
@@ -806,7 +809,7 @@ test_expect_success 'fetching deepen' '
git -C deepen log --pretty=tformat:%s master >actual &&
echo three >expected &&
test_cmp expected actual &&
- git -C deepen fetch --deepen=1 &&
+ GIT_TEST_PROTOCOL_VERSION= git -C deepen fetch --deepen=1 &&
git -C deepen log --pretty=tformat:%s origin/master >actual &&
cat >expected <<-\EOF &&
four
diff --git a/t/t5503-tagfollow.sh b/t/t5503-tagfollow.sh
index 4ca48f0276..0e90a90294 100755
--- a/t/t5503-tagfollow.sh
+++ b/t/t5503-tagfollow.sh
@@ -56,7 +56,7 @@ test_expect_success 'fetch A (new commit : 1 connection)' '
rm -f $U &&
(
cd cloned &&
- GIT_TRACE_PACKET=$UPATH git fetch &&
+ GIT_TRACE_PACKET=$UPATH GIT_TEST_PROTOCOL_VERSION= git fetch &&
test $A = $(git rev-parse --verify origin/master)
) &&
get_needs $U >actual &&
@@ -86,7 +86,7 @@ test_expect_success 'fetch C, T (new branch, tag : 1 connection)' '
rm -f $U &&
(
cd cloned &&
- GIT_TRACE_PACKET=$UPATH git fetch &&
+ GIT_TRACE_PACKET=$UPATH GIT_TEST_PROTOCOL_VERSION= git fetch &&
test $C = $(git rev-parse --verify origin/cat) &&
test $T = $(git rev-parse --verify tag1) &&
test $A = $(git rev-parse --verify tag1^0)
@@ -122,7 +122,7 @@ test_expect_success 'fetch B, S (commit and tag : 1 connection)' '
rm -f $U &&
(
cd cloned &&
- GIT_TRACE_PACKET=$UPATH git fetch &&
+ GIT_TRACE_PACKET=$UPATH GIT_TEST_PROTOCOL_VERSION= git fetch &&
test $B = $(git rev-parse --verify origin/master) &&
test $B = $(git rev-parse --verify tag2^0) &&
test $S = $(git rev-parse --verify tag2)
@@ -146,7 +146,7 @@ test_expect_success 'new clone fetch master and tags' '
cd clone2 &&
git init &&
git remote add origin .. &&
- GIT_TRACE_PACKET=$UPATH git fetch &&
+ GIT_TRACE_PACKET=$UPATH GIT_TEST_PROTOCOL_VERSION= git fetch &&
test $B = $(git rev-parse --verify origin/master) &&
test $S = $(git rev-parse --verify tag2) &&
test $B = $(git rev-parse --verify tag2^0) &&
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index ca69636fd5..7b480587c9 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -223,7 +223,7 @@ test_expect_success 'ls-remote --symref' '
$(git rev-parse refs/tags/mark1.10) refs/tags/mark1.10
$(git rev-parse refs/tags/mark1.2) refs/tags/mark1.2
EOF
- git ls-remote --symref >actual &&
+ GIT_TEST_PROTOCOL_VERSION= git ls-remote --symref >actual &&
test_cmp expect actual
'
@@ -243,7 +243,7 @@ test_expect_failure 'ls-remote with filtered symref (--heads)' '
1bd44cb9d13204b0fe1958db0082f5028a16eb3a refs/heads/foo
1bd44cb9d13204b0fe1958db0082f5028a16eb3a refs/heads/master
EOF
- git ls-remote --symref --heads . >actual &&
+ GIT_TEST_PROTOCOL_VERSION= git ls-remote --symref --heads . >actual &&
test_cmp expect actual
'
@@ -252,9 +252,9 @@ test_expect_success 'ls-remote --symref omits filtered-out matches' '
1bd44cb9d13204b0fe1958db0082f5028a16eb3a refs/heads/foo
1bd44cb9d13204b0fe1958db0082f5028a16eb3a refs/heads/master
EOF
- git ls-remote --symref --heads . >actual &&
+ GIT_TEST_PROTOCOL_VERSION= git ls-remote --symref --heads . >actual &&
test_cmp expect actual &&
- git ls-remote --symref . "refs/heads/*" >actual &&
+ GIT_TEST_PROTOCOL_VERSION= git ls-remote --symref . "refs/heads/*" >actual &&
test_cmp expect actual
'
diff --git a/t/t5515-fetch-merge-logic.sh b/t/t5515-fetch-merge-logic.sh
index 36b0dbc01c..f095555c3e 100755
--- a/t/t5515-fetch-merge-logic.sh
+++ b/t/t5515-fetch-merge-logic.sh
@@ -147,7 +147,7 @@ do
do
git update-ref -d "$refname" "$val"
done
- git fetch "$@" >/dev/null
+ GIT_TEST_PROTOCOL_VERSION= git fetch "$@" >/dev/null
cat .git/FETCH_HEAD
} >"$actual_f" &&
git show-ref >"$actual_r" &&
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 08cdee0b95..1d1b717cd5 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1129,7 +1129,7 @@ do
'
done
-test_expect_success 'fetch exact SHA1' '
+test_expect_success 'fetch exact SHA1 in protocol v0' '
mk_test testrepo heads/master hidden/one &&
git push testrepo master:refs/hidden/one &&
(
@@ -1148,7 +1148,8 @@ test_expect_success 'fetch exact SHA1' '
test_must_fail git cat-file -t $the_commit &&
# fetching the hidden object should fail by default
- test_must_fail git fetch -v ../testrepo $the_commit:refs/heads/copy 2>err &&
+ test_must_fail env GIT_TEST_PROTOCOL_VERSION= \
+ git fetch -v ../testrepo $the_commit:refs/heads/copy 2>err &&
test_i18ngrep "Server does not allow request for unadvertised object" err &&
test_must_fail git rev-parse --verify refs/heads/copy &&
@@ -1205,7 +1206,8 @@ do
mk_empty shallow &&
(
cd shallow &&
- test_must_fail git fetch --depth=1 ../testrepo/.git $SHA1 &&
+ test_must_fail env GIT_TEST_PROTOCOL_VERSION= \
+ git fetch --depth=1 ../testrepo/.git $SHA1 &&
git --git-dir=../testrepo/.git config uploadpack.allowreachablesha1inwant true &&
git fetch --depth=1 ../testrepo/.git $SHA1 &&
git cat-file commit $SHA1
@@ -1233,15 +1235,18 @@ do
mk_empty shallow &&
(
cd shallow &&
- test_must_fail ok=sigpipe git fetch ../testrepo/.git $SHA1_3 &&
- test_must_fail ok=sigpipe git fetch ../testrepo/.git $SHA1_1 &&
+ test_must_fail ok=sigpipe env GIT_TEST_PROTOCOL_VERSION= \
+ git fetch ../testrepo/.git $SHA1_3 &&
+ test_must_fail ok=sigpipe env GIT_TEST_PROTOCOL_VERSION= \
+ git fetch ../testrepo/.git $SHA1_1 &&
git --git-dir=../testrepo/.git config uploadpack.allowreachablesha1inwant true &&
git fetch ../testrepo/.git $SHA1_1 &&
git cat-file commit $SHA1_1 &&
test_must_fail git cat-file commit $SHA1_2 &&
git fetch ../testrepo/.git $SHA1_2 &&
git cat-file commit $SHA1_2 &&
- test_must_fail ok=sigpipe git fetch ../testrepo/.git $SHA1_3
+ test_must_fail ok=sigpipe env GIT_TEST_PROTOCOL_VERSION= \
+ git fetch ../testrepo/.git $SHA1_3
)
'
done
diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
index 6faf17e17a..85b3022ce6 100755
--- a/t/t5537-fetch-shallow.sh
+++ b/t/t5537-fetch-shallow.sh
@@ -127,7 +127,8 @@ test_expect_success 'fetch that requires changes in .git/shallow is filtered' '
git init notshallow &&
(
cd notshallow &&
- git fetch ../shallow/.git refs/heads/*:refs/remotes/shallow/*&&
+ GIT_TEST_PROTOCOL_VERSION= \
+ git fetch ../shallow/.git refs/heads/*:refs/remotes/shallow/* &&
git for-each-ref --format="%(refname)" >actual.refs &&
cat <<EOF >expect.refs &&
refs/remotes/shallow/no-shallow
diff --git a/t/t5539-fetch-http-shallow.sh b/t/t5539-fetch-http-shallow.sh
index 5fbf67c446..a121e19bc7 100755
--- a/t/t5539-fetch-http-shallow.sh
+++ b/t/t5539-fetch-http-shallow.sh
@@ -67,7 +67,8 @@ test_expect_success 'no shallow lines after receiving ACK ready' '
cd clone &&
git checkout --orphan newnew &&
test_commit new-too &&
- GIT_TRACE_PACKET="$TRASH_DIRECTORY/trace" git fetch --depth=2 &&
+ GIT_TRACE_PACKET="$TRASH_DIRECTORY/trace" GIT_TEST_PROTOCOL_VERSION= \
+ git fetch --depth=2 &&
grep "fetch-pack< ACK .* ready" ../trace &&
! grep "fetch-pack> done" ../trace
)
@@ -114,7 +115,7 @@ test_expect_success 'shallow clone exclude tag two' '
'
test_expect_success 'fetch exclude tag one' '
- git -C shallow12 fetch --shallow-exclude one origin &&
+ GIT_TEST_PROTOCOL_VERSION= git -C shallow12 fetch --shallow-exclude one origin &&
git -C shallow12 log --pretty=tformat:%s origin/master >actual &&
test_write_lines three two >expected &&
test_cmp expected actual
@@ -128,14 +129,14 @@ test_expect_success 'fetching deepen' '
test_commit two &&
test_commit three &&
mv .git "$HTTPD_DOCUMENT_ROOT_PATH/shallow-deepen.git" &&
- git clone --depth 1 $HTTPD_URL/smart/shallow-deepen.git deepen &&
+ GIT_TEST_PROTOCOL_VERSION= git clone --depth 1 $HTTPD_URL/smart/shallow-deepen.git deepen &&
mv "$HTTPD_DOCUMENT_ROOT_PATH/shallow-deepen.git" .git &&
test_commit four &&
git -C deepen log --pretty=tformat:%s master >actual &&
echo three >expected &&
test_cmp expected actual &&
mv .git "$HTTPD_DOCUMENT_ROOT_PATH/shallow-deepen.git" &&
- git -C deepen fetch --deepen=1 &&
+ GIT_TEST_PROTOCOL_VERSION= git -C deepen fetch --deepen=1 &&
git -C deepen log --pretty=tformat:%s origin/master >actual &&
cat >expected <<-\EOF &&
four
diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh
index 5475afc052..180c9005b7 100755
--- a/t/t5541-http-push-smart.sh
+++ b/t/t5541-http-push-smart.sh
@@ -45,14 +45,19 @@ test_expect_success 'no empty path components' '
# In the URL, add a trailing slash, and see if git appends yet another
# slash.
cd "$ROOT_PATH" &&
- git clone $HTTPD_URL/smart/test_repo.git/ test_repo_clone &&
+ # Other protocol versions may make different requests, so perform this
+ # clone with the default protocol.
+ GIT_TEST_PROTOCOL_VERSION= git clone $HTTPD_URL/smart/test_repo.git/ test_repo_clone &&
check_access_log exp
'
test_expect_success 'clone remote repository' '
rm -rf test_repo_clone &&
- git clone $HTTPD_URL/smart/test_repo.git test_repo_clone &&
+ # Other protocol versions may make different requests, so perform this
+ # clone with the default protocol.
+ GIT_TEST_PROTOCOL_VERSION= git clone $HTTPD_URL/smart/test_repo.git test_repo_clone &&
+
(
cd test_repo_clone && git config push.default matching
)
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index 8630b0cc39..a51993f35a 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -43,7 +43,8 @@ test_expect_success 'clone http repository' '
< Cache-Control: no-cache, max-age=0, must-revalidate
< Content-Type: application/x-git-upload-pack-result
EOF
- GIT_TRACE_CURL=true git clone --quiet $HTTPD_URL/smart/repo.git clone 2>err &&
+ GIT_TRACE_CURL=true GIT_TEST_PROTOCOL_VERSION= \
+ git clone --quiet $HTTPD_URL/smart/repo.git clone 2>err &&
test_cmp file clone/file &&
tr '\''\015'\'' Q <err |
sed -e "
@@ -92,7 +93,7 @@ test_expect_success 'fetch changes via http' '
echo content >>file &&
git commit -a -m two &&
git push public &&
- (cd clone && git pull) &&
+ (cd clone && GIT_TEST_PROTOCOL_VERSION= git pull) &&
test_cmp file clone/file
'
@@ -143,7 +144,7 @@ test_expect_success 'clone from auth-only-for-push repository' '
test_expect_success 'clone from auth-only-for-objects repository' '
echo two >expect &&
set_askpass user@host pass@host &&
- git clone --bare "$HTTPD_URL/auth-fetch/smart/repo.git" half-auth &&
+ GIT_TEST_PROTOCOL_VERSION= git clone --bare "$HTTPD_URL/auth-fetch/smart/repo.git" half-auth &&
expect_askpass both user@host &&
git --git-dir=half-auth log -1 --format=%s >actual &&
test_cmp expect actual
@@ -151,7 +152,7 @@ test_expect_success 'clone from auth-only-for-objects repository' '
test_expect_success 'no-op half-auth fetch does not require a password' '
set_askpass wrong &&
- git --git-dir=half-auth fetch &&
+ GIT_TEST_PROTOCOL_VERSION= git --git-dir=half-auth fetch &&
expect_askpass none
'
@@ -187,7 +188,7 @@ test_expect_success 'create namespaced refs' '
'
test_expect_success 'smart clone respects namespace' '
- git clone "$HTTPD_URL/smart_namespace/repo.git" ns-smart &&
+ GIT_TEST_PROTOCOL_VERSION= git clone "$HTTPD_URL/smart_namespace/repo.git" ns-smart &&
echo namespaced >expect &&
git --git-dir=ns-smart/.git log -1 --format=%s >actual &&
test_cmp expect actual
@@ -214,7 +215,7 @@ test_expect_success 'cookies stored in http.cookiefile when http.savecookies set
EOF
git config http.cookiefile cookies.txt &&
git config http.savecookies true &&
- git ls-remote $HTTPD_URL/smart_cookies/repo.git master &&
+ GIT_TEST_PROTOCOL_VERSION= git ls-remote $HTTPD_URL/smart_cookies/repo.git master &&
tail -3 cookies.txt | sort >cookies_tail.txt &&
test_cmp expect_cookies.txt cookies_tail.txt
'
@@ -306,7 +307,8 @@ test_expect_success 'test allowreachablesha1inwant with unreachable' '
git init --bare test_reachable.git &&
git -C test_reachable.git remote add origin "$HTTPD_URL/smart/repo.git" &&
- test_must_fail git -C test_reachable.git fetch origin "$(git rev-parse HEAD)"
+ test_must_fail env GIT_TEST_PROTOCOL_VERSION= \
+ git -C test_reachable.git fetch origin "$(git rev-parse HEAD)"
'
test_expect_success 'test allowanysha1inwant with unreachable' '
@@ -325,7 +327,8 @@ test_expect_success 'test allowanysha1inwant with unreachable' '
git init --bare test_reachable.git &&
git -C test_reachable.git remote add origin "$HTTPD_URL/smart/repo.git" &&
- test_must_fail git -C test_reachable.git fetch origin "$(git rev-parse HEAD)" &&
+ test_must_fail env GIT_TEST_PROTOCOL_VERSION= \
+ git -C test_reachable.git fetch origin "$(git rev-parse HEAD)" &&
git -C "$server" config uploadpack.allowanysha1inwant 1 &&
git -C test_reachable.git fetch origin "$(git rev-parse HEAD)"
diff --git a/t/t5552-skipping-fetch-negotiator.sh b/t/t5552-skipping-fetch-negotiator.sh
index 30857b84a8..979e6583d4 100755
--- a/t/t5552-skipping-fetch-negotiator.sh
+++ b/t/t5552-skipping-fetch-negotiator.sh
@@ -127,7 +127,7 @@ test_expect_success 'use ref advertisement to filter out commits' '
# not need to send any ancestors of "c3", but we still need to send "c3"
# itself.
test_config -C client fetch.negotiationalgorithm skipping &&
- trace_fetch client origin to_fetch &&
+ GIT_TEST_PROTOCOL_VERSION= trace_fetch client origin to_fetch &&
have_sent c5 c4^ c2side &&
have_not_sent c4 c4^^ c4^^^
'
@@ -189,7 +189,7 @@ test_expect_success 'do not send "have" with ancestors of commits that server AC
test_commit -C server commit-on-b1 &&
test_config -C client fetch.negotiationalgorithm skipping &&
- trace_fetch client "$(pwd)/server" to_fetch &&
+ GIT_TEST_PROTOCOL_VERSION= trace_fetch client "$(pwd)/server" to_fetch &&
grep " fetch" trace &&
# fetch-pack sends 2 requests each containing 16 "have" lines before
diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
index 7466aad111..c42ee245cd 100755
--- a/t/t5570-git-daemon.sh
+++ b/t/t5570-git-daemon.sh
@@ -190,7 +190,7 @@ test_expect_success 'daemon log records all attributes' '
EOF
>daemon.log &&
GIT_OVERRIDE_VIRTUAL_HOST=localhost \
- git -c protocol.version=1 \
+ GIT_TEST_PROTOCOL_VERSION= git -c protocol.version=1 \
ls-remote "$GIT_DAEMON_URL/interp.git" &&
grep -i extended.attribute daemon.log | cut -d" " -f2- >actual &&
test_cmp expect actual
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index e87164aa8f..a555e38845 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -943,7 +943,8 @@ test_expect_success 'submodule update clone shallow submodule outside of depth'
cd super3 &&
sed -e "s#url = ../#url = file://$pwd/#" <.gitmodules >.gitmodules.tmp &&
mv -f .gitmodules.tmp .gitmodules &&
- test_must_fail git submodule update --init --depth=1 2>actual &&
+ test_must_fail env GIT_TEST_PROTOCOL_VERSION= \
+ git submodule update --init --depth=1 2>actual &&
test_i18ngrep "Direct fetching of that commit failed." actual &&
git -C ../submodule config uploadpack.allowReachableSHA1InWant true &&
git submodule update --init --depth=1 >actual &&
--
2.20.0.405.gbc1bbc6f85
^ permalink raw reply related [flat|nested] 73+ messages in thread
* Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2
2018-12-13 15:58 ` [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2 Ævar Arnfjörð Bjarmason
@ 2018-12-13 16:08 ` Ævar Arnfjörð Bjarmason
2018-12-14 2:18 ` Junio C Hamano
2018-12-14 10:12 ` Jeff King
0 siblings, 2 replies; 73+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-12-13 16:08 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Brandon Williams, Jonathan Tan
On Thu, Dec 13 2018, Ævar Arnfjörð Bjarmason wrote:
Now that we have this maybe we should discuss why these tests show
different things:
> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
> index 086f2c40f6..8b1217ea26 100755
> --- a/t/t5500-fetch-pack.sh
> +++ b/t/t5500-fetch-pack.sh
> @@ -628,7 +628,10 @@ test_expect_success 'fetch-pack cannot fetch a raw sha1 that is not advertised a
> test_commit -C server 6 &&
>
> git init client &&
> - test_must_fail git -C client fetch-pack ../server \
> +
> + # Other protocol versions (e.g. 2) allow fetching an unadvertised
> + # object, so run this test with the default protocol version (0).
> + test_must_fail env GIT_TEST_PROTOCOL_VERSION= git -C client fetch-pack ../server \
> $(git -C server rev-parse refs/heads/master^) 2>err &&
What? So the equivalent of uploadpack.allowAnySHA1InWant=true is on for
v2 all the time?
> test_i18ngrep "Server does not allow request for unadvertised object" err
> '
> @@ -788,7 +791,7 @@ test_expect_success 'shallow clone exclude tag two' '
> '
>
> test_expect_success 'fetch exclude tag one' '
> - git -C shallow12 fetch --shallow-exclude one origin &&
> + GIT_TEST_PROTOCOL_VERSION= git -C shallow12 fetch --shallow-exclude one origin &&
> git -C shallow12 log --pretty=tformat:%s origin/master >actual &&
> test_write_lines three two >expected &&
> test_cmp expected actual
> @@ -806,7 +809,7 @@ test_expect_success 'fetching deepen' '
> git -C deepen log --pretty=tformat:%s master >actual &&
> echo three >expected &&
> test_cmp expected actual &&
> - git -C deepen fetch --deepen=1 &&
> + GIT_TEST_PROTOCOL_VERSION= git -C deepen fetch --deepen=1 &&
> git -C deepen log --pretty=tformat:%s origin/master >actual &&
> cat >expected <<-\EOF &&
> four
> diff --git a/t/t5503-tagfollow.sh b/t/t5503-tagfollow.sh
> index 4ca48f0276..0e90a90294 100755
> --- a/t/t5503-tagfollow.sh
> +++ b/t/t5503-tagfollow.sh
> @@ -56,7 +56,7 @@ test_expect_success 'fetch A (new commit : 1 connection)' '
> rm -f $U &&
> (
> cd cloned &&
> - GIT_TRACE_PACKET=$UPATH git fetch &&
> + GIT_TRACE_PACKET=$UPATH GIT_TEST_PROTOCOL_VERSION= git fetch &&
> test $A = $(git rev-parse --verify origin/master)
> ) &&
> get_needs $U >actual &&
> @@ -86,7 +86,7 @@ test_expect_success 'fetch C, T (new branch, tag : 1 connection)' '
> rm -f $U &&
> (
> cd cloned &&
> - GIT_TRACE_PACKET=$UPATH git fetch &&
> + GIT_TRACE_PACKET=$UPATH GIT_TEST_PROTOCOL_VERSION= git fetch &&
> test $C = $(git rev-parse --verify origin/cat) &&
> test $T = $(git rev-parse --verify tag1) &&
> test $A = $(git rev-parse --verify tag1^0)
> @@ -122,7 +122,7 @@ test_expect_success 'fetch B, S (commit and tag : 1 connection)' '
> rm -f $U &&
> (
> cd cloned &&
> - GIT_TRACE_PACKET=$UPATH git fetch &&
> + GIT_TRACE_PACKET=$UPATH GIT_TEST_PROTOCOL_VERSION= git fetch &&
> test $B = $(git rev-parse --verify origin/master) &&
> test $B = $(git rev-parse --verify tag2^0) &&
> test $S = $(git rev-parse --verify tag2)
> @@ -146,7 +146,7 @@ test_expect_success 'new clone fetch master and tags' '
> cd clone2 &&
> git init &&
> git remote add origin .. &&
> - GIT_TRACE_PACKET=$UPATH git fetch &&
> + GIT_TRACE_PACKET=$UPATH GIT_TEST_PROTOCOL_VERSION= git fetch &&
> test $B = $(git rev-parse --verify origin/master) &&
> test $S = $(git rev-parse --verify tag2) &&
> test $B = $(git rev-parse --verify tag2^0) &&
> diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
> index ca69636fd5..7b480587c9 100755
> --- a/t/t5512-ls-remote.sh
> +++ b/t/t5512-ls-remote.sh
> @@ -223,7 +223,7 @@ test_expect_success 'ls-remote --symref' '
> $(git rev-parse refs/tags/mark1.10) refs/tags/mark1.10
> $(git rev-parse refs/tags/mark1.2) refs/tags/mark1.2
> EOF
> - git ls-remote --symref >actual &&
> + GIT_TEST_PROTOCOL_VERSION= git ls-remote --symref >actual &&
> test_cmp expect actual
> '
>
> @@ -243,7 +243,7 @@ test_expect_failure 'ls-remote with filtered symref (--heads)' '
> 1bd44cb9d13204b0fe1958db0082f5028a16eb3a refs/heads/foo
> 1bd44cb9d13204b0fe1958db0082f5028a16eb3a refs/heads/master
> EOF
> - git ls-remote --symref --heads . >actual &&
> + GIT_TEST_PROTOCOL_VERSION= git ls-remote --symref --heads . >actual &&
> test_cmp expect actual
> '
>
> @@ -252,9 +252,9 @@ test_expect_success 'ls-remote --symref omits filtered-out matches' '
> 1bd44cb9d13204b0fe1958db0082f5028a16eb3a refs/heads/foo
> 1bd44cb9d13204b0fe1958db0082f5028a16eb3a refs/heads/master
> EOF
> - git ls-remote --symref --heads . >actual &&
> + GIT_TEST_PROTOCOL_VERSION= git ls-remote --symref --heads . >actual &&
> test_cmp expect actual &&
> - git ls-remote --symref . "refs/heads/*" >actual &&
> + GIT_TEST_PROTOCOL_VERSION= git ls-remote --symref . "refs/heads/*" >actual &&
> test_cmp expect actual
> '
>
> diff --git a/t/t5515-fetch-merge-logic.sh b/t/t5515-fetch-merge-logic.sh
> index 36b0dbc01c..f095555c3e 100755
> --- a/t/t5515-fetch-merge-logic.sh
> +++ b/t/t5515-fetch-merge-logic.sh
This one should really be looked at by someone more familiar with
v2. Looks scary that we have different merge results with it.
> @@ -147,7 +147,7 @@ do
> do
> git update-ref -d "$refname" "$val"
> done
> - git fetch "$@" >/dev/null
> + GIT_TEST_PROTOCOL_VERSION= git fetch "$@" >/dev/null
> cat .git/FETCH_HEAD
> } >"$actual_f" &&
> git show-ref >"$actual_r" &&
> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index 08cdee0b95..1d1b717cd5 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -1129,7 +1129,7 @@ do
> '
> done
>
> -test_expect_success 'fetch exact SHA1' '
> +test_expect_success 'fetch exact SHA1 in protocol v0' '
> mk_test testrepo heads/master hidden/one &&
> git push testrepo master:refs/hidden/one &&
> (
> @@ -1148,7 +1148,8 @@ test_expect_success 'fetch exact SHA1' '
> test_must_fail git cat-file -t $the_commit &&
>
> # fetching the hidden object should fail by default
> - test_must_fail git fetch -v ../testrepo $the_commit:refs/heads/copy 2>err &&
> + test_must_fail env GIT_TEST_PROTOCOL_VERSION= \
> + git fetch -v ../testrepo $the_commit:refs/heads/copy 2>err &&
> test_i18ngrep "Server does not allow request for unadvertised object" err &&
> test_must_fail git rev-parse --verify refs/heads/copy &&
>
> @@ -1205,7 +1206,8 @@ do
> mk_empty shallow &&
> (
> cd shallow &&
> - test_must_fail git fetch --depth=1 ../testrepo/.git $SHA1 &&
> + test_must_fail env GIT_TEST_PROTOCOL_VERSION= \
> + git fetch --depth=1 ../testrepo/.git $SHA1 &&
> git --git-dir=../testrepo/.git config uploadpack.allowreachablesha1inwant true &&
> git fetch --depth=1 ../testrepo/.git $SHA1 &&
> git cat-file commit $SHA1
> @@ -1233,15 +1235,18 @@ do
> mk_empty shallow &&
> (
> cd shallow &&
> - test_must_fail ok=sigpipe git fetch ../testrepo/.git $SHA1_3 &&
> - test_must_fail ok=sigpipe git fetch ../testrepo/.git $SHA1_1 &&
> + test_must_fail ok=sigpipe env GIT_TEST_PROTOCOL_VERSION= \
> + git fetch ../testrepo/.git $SHA1_3 &&
> + test_must_fail ok=sigpipe env GIT_TEST_PROTOCOL_VERSION= \
> + git fetch ../testrepo/.git $SHA1_1 &&
> git --git-dir=../testrepo/.git config uploadpack.allowreachablesha1inwant true &&
> git fetch ../testrepo/.git $SHA1_1 &&
> git cat-file commit $SHA1_1 &&
> test_must_fail git cat-file commit $SHA1_2 &&
> git fetch ../testrepo/.git $SHA1_2 &&
> git cat-file commit $SHA1_2 &&
> - test_must_fail ok=sigpipe git fetch ../testrepo/.git $SHA1_3
> + test_must_fail ok=sigpipe env GIT_TEST_PROTOCOL_VERSION= \
> + git fetch ../testrepo/.git $SHA1_3
> )
> '
Ditto all this stuff.
> [...]
> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
> index e87164aa8f..a555e38845 100755
> --- a/t/t7406-submodule-update.sh
> +++ b/t/t7406-submodule-update.sh
> @@ -943,7 +943,8 @@ test_expect_success 'submodule update clone shallow submodule outside of depth'
> cd super3 &&
> sed -e "s#url = ../#url = file://$pwd/#" <.gitmodules >.gitmodules.tmp &&
> mv -f .gitmodules.tmp .gitmodules &&
> - test_must_fail git submodule update --init --depth=1 2>actual &&
> + test_must_fail env GIT_TEST_PROTOCOL_VERSION= \
> + git submodule update --init --depth=1 2>actual &&
> test_i18ngrep "Direct fetching of that commit failed." actual &&
> git -C ../submodule config uploadpack.allowReachableSHA1InWant true &&
> git submodule update --init --depth=1 >actual &&
And this one and various other shallow things look odd, are shallow
fetches different under v2?
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2
2018-12-13 16:08 ` Ævar Arnfjörð Bjarmason
@ 2018-12-14 2:18 ` Junio C Hamano
2018-12-14 10:12 ` Jeff King
1 sibling, 0 replies; 73+ messages in thread
From: Junio C Hamano @ 2018-12-14 2:18 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Jeff King, Brandon Williams, Jonathan Tan
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>> + # Other protocol versions (e.g. 2) allow fetching an unadvertised
>> + # object, so run this test with the default protocol version (0).
>> + test_must_fail env GIT_TEST_PROTOCOL_VERSION= git -C client fetch-pack ../server \
>> $(git -C server rev-parse refs/heads/master^) 2>err &&
>
> What? So the equivalent of uploadpack.allowAnySHA1InWant=true is on for
> v2 all the time?
UnfortunatelyYes (see Peff's three-patch series).
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2
2018-12-13 16:08 ` Ævar Arnfjörð Bjarmason
2018-12-14 2:18 ` Junio C Hamano
@ 2018-12-14 10:12 ` Jeff King
2018-12-14 10:55 ` Ævar Arnfjörð Bjarmason
1 sibling, 1 reply; 73+ messages in thread
From: Jeff King @ 2018-12-14 10:12 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, Brandon Williams, Jonathan Tan
On Thu, Dec 13, 2018 at 05:08:43PM +0100, Ævar Arnfjörð Bjarmason wrote:
> Now that we have this maybe we should discuss why these tests show
> different things:
>
> > diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
> > index 086f2c40f6..8b1217ea26 100755
> > --- a/t/t5500-fetch-pack.sh
> > +++ b/t/t5500-fetch-pack.sh
> > @@ -628,7 +628,10 @@ test_expect_success 'fetch-pack cannot fetch a raw sha1 that is not advertised a
> > test_commit -C server 6 &&
> >
> > git init client &&
> > - test_must_fail git -C client fetch-pack ../server \
> > +
> > + # Other protocol versions (e.g. 2) allow fetching an unadvertised
> > + # object, so run this test with the default protocol version (0).
> > + test_must_fail env GIT_TEST_PROTOCOL_VERSION= git -C client fetch-pack ../server \
> > $(git -C server rev-parse refs/heads/master^) 2>err &&
>
> What? So the equivalent of uploadpack.allowAnySHA1InWant=true is on for
> v2 all the time?
Yeah, I actually didn't realize it until working on the earlier series,
but this is the documented behavior:
$ git grep -hC3 'want <oid>' Documentation/technical/protocol-v2.txt
A `fetch` request can take the following arguments:
want <oid>
Indicates to the server an object which the client wants to
retrieve. Wants can be anything and are not limited to
advertised objects.
An interesting implication of this at GitHub (and possibly other
hosters) is that it exposes objects from shared storage via unexpected
repos. If I fork torvalds/linux to peff/linux and push up object X, a v0
fetch will (by default) refuse to serve it to me. But v2 will happily
hand it over, which may confuse people into thinking that the object is
(or at least at some point was) in Linus's repository.
-Peff
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2
2018-12-14 10:12 ` Jeff King
@ 2018-12-14 10:55 ` Ævar Arnfjörð Bjarmason
2018-12-14 11:08 ` Ævar Arnfjörð Bjarmason
2018-12-17 19:57 ` Jeff King
0 siblings, 2 replies; 73+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-12-14 10:55 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, Brandon Williams, Jonathan Tan
On Fri, Dec 14 2018, Jeff King wrote:
> On Thu, Dec 13, 2018 at 05:08:43PM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>> Now that we have this maybe we should discuss why these tests show
>> different things:
>>
>> > diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
>> > index 086f2c40f6..8b1217ea26 100755
>> > --- a/t/t5500-fetch-pack.sh
>> > +++ b/t/t5500-fetch-pack.sh
>> > @@ -628,7 +628,10 @@ test_expect_success 'fetch-pack cannot fetch a raw sha1 that is not advertised a
>> > test_commit -C server 6 &&
>> >
>> > git init client &&
>> > - test_must_fail git -C client fetch-pack ../server \
>> > +
>> > + # Other protocol versions (e.g. 2) allow fetching an unadvertised
>> > + # object, so run this test with the default protocol version (0).
>> > + test_must_fail env GIT_TEST_PROTOCOL_VERSION= git -C client fetch-pack ../server \
>> > $(git -C server rev-parse refs/heads/master^) 2>err &&
>>
>> What? So the equivalent of uploadpack.allowAnySHA1InWant=true is on for
>> v2 all the time?
>
> Yeah, I actually didn't realize it until working on the earlier series,
> but this is the documented behavior:
>
> $ git grep -hC3 'want <oid>' Documentation/technical/protocol-v2.txt
>
> A `fetch` request can take the following arguments:
>
> want <oid>
> Indicates to the server an object which the client wants to
> retrieve. Wants can be anything and are not limited to
> advertised objects.
>
> An interesting implication of this at GitHub (and possibly other
> hosters) is that it exposes objects from shared storage via unexpected
> repos. If I fork torvalds/linux to peff/linux and push up object X, a v0
> fetch will (by default) refuse to serve it to me. But v2 will happily
> hand it over, which may confuse people into thinking that the object is
> (or at least at some point was) in Linus's repository.
More importantly this bypasses the security guarantee we've had with the
default of uploadpack.allowAnySHA1InWant=false.
If I push a id_rsa to a topic branch on $githost and immediately realize
my mistake and delete it, someone with access to a log of SHA-1s
(e.g. an IRC bot spewing out from/to commits) won't be able to pull it
down. This is why we have that as a setting in the first place
(f8edeaa05d ("upload-pack: optionally allow fetching any sha1",
2016-11-11)).
Of course this is:
a) Subject to the "SECURITY" section of git-fetch, i.e. maybe this
doesn't even work in the face of a determined attacker.
b) Hosting sites like GitHub, GitLab, Gitweb etc. aren't doing
reachability checks when you view /commit/<id>. If I delete my topic
branch it'll be viewable until the next GC kicks in (which would
also be the case with uploadpack.allowAnySHA1InWant=true).
So I wonder how much we need to care about this in practice, but for
some configurations protocol v2 definitely breaks a security promise
we've been making, now whether that promise was always BS due to "a)"
above is another matter.
I'm inclined to say that in the face of that "SECURITY" section we
should just:
* Turn on uploadpack.allowReachableSHA1InWant for v0/v1 by
default. Make saying uploadpack.allowReachableSHA1InWant=false warn
with "this won't work, see SECURITY...".
* The uploadpack.allowTipSHA1InWant setting will also be turned on by
default, and will be much faster, since it'll just degrade to
uploadpack.allowReachableSHA1InWant=true and we won't need any
reachability check. We'll also warn saying that setting it is
useless.
Otherwise what's th point of these settings given what we're talking
about in that "SECURITY" section? It seems to just lure users into a
false sense of security. Better to not make promises we're not confident
we can keep, and say that if you push your id_rsa you need to run a
gc/prune on the server.
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2
2018-12-14 10:55 ` Ævar Arnfjörð Bjarmason
@ 2018-12-14 11:08 ` Ævar Arnfjörð Bjarmason
2018-12-17 19:59 ` Jeff King
2018-12-17 19:57 ` Jeff King
1 sibling, 1 reply; 73+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-12-14 11:08 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, Brandon Williams, Jonathan Tan
On Fri, Dec 14 2018, Ævar Arnfjörð Bjarmason wrote:
> On Fri, Dec 14 2018, Jeff King wrote:
>
>> On Thu, Dec 13, 2018 at 05:08:43PM +0100, Ævar Arnfjörð Bjarmason wrote:
>>
>>> Now that we have this maybe we should discuss why these tests show
>>> different things:
>>>
>>> > diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
>>> > index 086f2c40f6..8b1217ea26 100755
>>> > --- a/t/t5500-fetch-pack.sh
>>> > +++ b/t/t5500-fetch-pack.sh
>>> > @@ -628,7 +628,10 @@ test_expect_success 'fetch-pack cannot fetch a raw sha1 that is not advertised a
>>> > test_commit -C server 6 &&
>>> >
>>> > git init client &&
>>> > - test_must_fail git -C client fetch-pack ../server \
>>> > +
>>> > + # Other protocol versions (e.g. 2) allow fetching an unadvertised
>>> > + # object, so run this test with the default protocol version (0).
>>> > + test_must_fail env GIT_TEST_PROTOCOL_VERSION= git -C client fetch-pack ../server \
>>> > $(git -C server rev-parse refs/heads/master^) 2>err &&
>>>
>>> What? So the equivalent of uploadpack.allowAnySHA1InWant=true is on for
>>> v2 all the time?
>>
>> Yeah, I actually didn't realize it until working on the earlier series,
>> but this is the documented behavior:
>>
>> $ git grep -hC3 'want <oid>' Documentation/technical/protocol-v2.txt
>>
>> A `fetch` request can take the following arguments:
>>
>> want <oid>
>> Indicates to the server an object which the client wants to
>> retrieve. Wants can be anything and are not limited to
>> advertised objects.
>>
>> An interesting implication of this at GitHub (and possibly other
>> hosters) is that it exposes objects from shared storage via unexpected
>> repos. If I fork torvalds/linux to peff/linux and push up object X, a v0
>> fetch will (by default) refuse to serve it to me. But v2 will happily
>> hand it over, which may confuse people into thinking that the object is
>> (or at least at some point was) in Linus's repository.
>
> More importantly this bypasses the security guarantee we've had with the
> default of uploadpack.allowAnySHA1InWant=false.
>
> If I push a id_rsa to a topic branch on $githost and immediately realize
> my mistake and delete it, someone with access to a log of SHA-1s
> (e.g. an IRC bot spewing out from/to commits) won't be able to pull it
> down. This is why we have that as a setting in the first place
> (f8edeaa05d ("upload-pack: optionally allow fetching any sha1",
> 2016-11-11)).
>
> Of course this is:
>
> a) Subject to the "SECURITY" section of git-fetch, i.e. maybe this
> doesn't even work in the face of a determined attacker.
>
> b) Hosting sites like GitHub, GitLab, Gitweb etc. aren't doing
> reachability checks when you view /commit/<id>. If I delete my topic
> branch it'll be viewable until the next GC kicks in (which would
> also be the case with uploadpack.allowAnySHA1InWant=true).
>
> So I wonder how much we need to care about this in practice, but for
> some configurations protocol v2 definitely breaks a security promise
> we've been making, now whether that promise was always BS due to "a)"
> above is another matter.
>
> I'm inclined to say that in the face of that "SECURITY" section we
> should just:
>
> * Turn on uploadpack.allowReachableSHA1InWant for v0/v1 by
> default. Make saying uploadpack.allowReachableSHA1InWant=false warn
> with "this won't work, see SECURITY...".
>
> * The uploadpack.allowTipSHA1InWant setting will also be turned on by
> default, and will be much faster, since it'll just degrade to
> uploadpack.allowReachableSHA1InWant=true and we won't need any
> reachability check. We'll also warn saying that setting it is
> useless.
>
> Otherwise what's th point of these settings given what we're talking
> about in that "SECURITY" section? It seems to just lure users into a
> false sense of security. Better to not make promises we're not confident
> we can keep, and say that if you push your id_rsa you need to run a
> gc/prune on the server.
I sent that too fast. What I meant is that there's 3
settings. uploadpack.{allowTipSHA1InWant,allowReachableSHA1InWant,allowAnySHA1InWant}. Those
are, respectively, cheap/expensive/cheap to serve up.
We could make them cheap/cheap/cheap by just making the first two a
synonym for the 3rd (allowAnySHA1InWant), because as noted above
Also, parts of the v2 code, e.g. this in 685fbd3291 ("fetch-pack:
perform a fetch using v2", 2018-03-15):
/* v2 supports these by default */
allow_unadvertised_object_request |= ALLOW_REACHABLE_SHA1;
Seem to have intended to turn on allowReachableSHA1InWant, not
allowAnySHA1InWant, but apparently that's what we're doing?
In any case, regardless of what v2 intended there anything except having
allowAnySHA1InWant on by default seems irresponsible given x) us
documenting in "SECURITY" that it doesn't really work y) even if it did,
there being a *tiny* minority of people who'd in some way care about
this feature who don't also run some sort of web UI on the git server
that'll be bypassing this setting no matter if it worked as intended for
the wire protocol.
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2
2018-12-14 11:08 ` Ævar Arnfjörð Bjarmason
@ 2018-12-17 19:59 ` Jeff King
0 siblings, 0 replies; 73+ messages in thread
From: Jeff King @ 2018-12-17 19:59 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, Brandon Williams, Jonathan Tan
On Fri, Dec 14, 2018 at 12:08:02PM +0100, Ævar Arnfjörð Bjarmason wrote:
> Also, parts of the v2 code, e.g. this in 685fbd3291 ("fetch-pack:
> perform a fetch using v2", 2018-03-15):
>
> /* v2 supports these by default */
> allow_unadvertised_object_request |= ALLOW_REACHABLE_SHA1;
>
> Seem to have intended to turn on allowReachableSHA1InWant, not
> allowAnySHA1InWant, but apparently that's what we're doing?
That I don't know. Maybe Brandon can answer what the intent was.
I agree that turning on the reachability check would make it behavior
more or less the same as v0 (at some additional cost to walk the refs
again, but v0 smart-http already had to do that).
-Peff
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2
2018-12-14 10:55 ` Ævar Arnfjörð Bjarmason
2018-12-14 11:08 ` Ævar Arnfjörð Bjarmason
@ 2018-12-17 19:57 ` Jeff King
2018-12-17 22:16 ` [PATCH] upload-pack: turn on uploadpack.allowAnySHA1InWant=true Ævar Arnfjörð Bjarmason
2018-12-17 23:14 ` [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2 Jonathan Nieder
1 sibling, 2 replies; 73+ messages in thread
From: Jeff King @ 2018-12-17 19:57 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, Brandon Williams, Jonathan Tan
On Fri, Dec 14, 2018 at 11:55:30AM +0100, Ævar Arnfjörð Bjarmason wrote:
> > An interesting implication of this at GitHub (and possibly other
> > hosters) is that it exposes objects from shared storage via unexpected
> > repos. If I fork torvalds/linux to peff/linux and push up object X, a v0
> > fetch will (by default) refuse to serve it to me. But v2 will happily
> > hand it over, which may confuse people into thinking that the object is
> > (or at least at some point was) in Linus's repository.
>
> More importantly this bypasses the security guarantee we've had with the
> default of uploadpack.allowAnySHA1InWant=false.
IMHO those security guarantees there are overrated (due to delta
guessing attacks, though things are not quite as bad if the attacker
can't actually push to the repo).
But I agree that people do assume it's the case. I was certainly
surprised by the v2 behavior, and I don't remember that aspect being
discussed.
(I suspect it was mostly because in the stateless world, the "fetch"
operation requires iterating all of the refs again. That's made even
harder by the fact that "ls-refs" takes arguments now, and might have
only advertised a subset of the refs. How can "fetch" know which ones
were advertised?).
> a) Subject to the "SECURITY" section of git-fetch, i.e. maybe this
> doesn't even work in the face of a determined attacker.
Yeah, this is the part I was thinking about above.
> b) Hosting sites like GitHub, GitLab, Gitweb etc. aren't doing
> reachability checks when you view /commit/<id>. If I delete my topic
> branch it'll be viewable until the next GC kicks in (which would
> also be the case with uploadpack.allowAnySHA1InWant=true).
Actually, we don't ever prune unless a user asks us to (and certainly
users do ask us to after accidentally pushing secret stuff). In GitHub's
case, though, we always tell people to consider anything pushed to a
public repo to be non-secret, even if it was exposed for only a few
seconds. My understanding is that there are literally clients watching
the public feeds and sucking down repo data looking for these kinds of
mistakes.
The /commit/<id> thing has been a frequent topic of discussion within
GitHub over the years, and we have looked at actually doing reachability
checks. They're expensive, though bitmaps help.
> So I wonder how much we need to care about this in practice, but for
> some configurations protocol v2 definitely breaks a security promise
> we've been making, now whether that promise was always BS due to "a)"
> above is another matter.
>
> I'm inclined to say that in the face of that "SECURITY" section we
> should just:
>
> * Turn on uploadpack.allowReachableSHA1InWant for v0/v1 by
> default. Make saying uploadpack.allowReachableSHA1InWant=false warn
> with "this won't work, see SECURITY...".
>
> * The uploadpack.allowTipSHA1InWant setting will also be turned on by
> default, and will be much faster, since it'll just degrade to
> uploadpack.allowReachableSHA1InWant=true and we won't need any
> reachability check. We'll also warn saying that setting it is
> useless.
No real argument from me. I have always thought those security
guarantees were BS.
However, I do think that's all orthogonal to the issue I was mentioning,
which is that it looks like repo A wants to serve you object X, even if
that repo never saw the object. That's unique to shared storage schemes,
though.
IMHO this is also a user education issue (it's a question of trust: refs
are the source of trust, and objects don't need to be trusted because
they're immutable). But it's pretty subtle for people who don't know the
inner workings of Git.
-Peff
^ permalink raw reply [flat|nested] 73+ messages in thread
* [PATCH] upload-pack: turn on uploadpack.allowAnySHA1InWant=true
2018-12-17 19:57 ` Jeff King
@ 2018-12-17 22:16 ` Ævar Arnfjörð Bjarmason
2018-12-17 22:34 ` David Turner
2018-12-17 23:14 ` [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2 Jonathan Nieder
1 sibling, 1 reply; 73+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-12-17 22:16 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Brandon Williams, Jonathan Tan,
Fredrik Medley, David Turner, Matt McCutchen,
Ævar Arnfjörð Bjarmason
Unconditionally turn on uploadpack.allowAnySHA1InWant=true which means
that the lesser uploadpack.allow{Tip,Reachable}SHA1InWant settings are
implicitly "true" as well.
This is because as mentioned in 235ec24352e ("doc: mention transfer
data leaks in more places", 2016-11-14) this has always been leaky,
and thus lulls users into a false sense of security. Better to not
make promises we're not confident we can keep.
Now setting any of uploadpack.allow{Tip,Reachable,Any}SHA1InWant will
warn, to e.g. point users who've explicitly set it to "false" to the
documentation. They're probably relying on behavior that was always
buggy. We may have users who trusted it to be "false" by default, but
warning if it's set seems like a good trade-off.
This change also has the effect of making any operation that needed
uploadpack.allowReachableSHA1InWant=true much faster, since we won't
need any reachability check.
This change doesn't do any of the hard work of extracting the now-dead
ALLOW_* code out of upload-pack.c and fetch-pack.c. Most of that was
changed or added in the following commits:
- 9f9fb57063a ("upload-pack: turn on uploadpack.allowAnySHA1InWant=true", 2018-12-17)
- 6e7b66eebd1 ("fetch: fetch objects by their exact SHA-1 object names", 2013-01-29)
- 7199c093ad4 ("upload-pack: prepare to extend allow-tip-sha1-in-want", 2015-05-21)
- 68ee628932c ("upload-pack: optionally allow fetching reachable sha1", 2015-05-21)
- f8edeaa05d8 ("upload-pack: optionally allow fetching any sha1", 2016-11-11)
That cleanup step can be done later. Starting with a change to switch
the default (and only) value and tweaking the docs makes it easier to
reason about this than fully ripping out the code right away.
See https://public-inbox.org/git/87pnu51kac.fsf@evledraar.gmail.com/
and related messages for further discussion.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
Documentation/config/uploadpack.txt | 48 +++++++++++++++----------
Documentation/transfer-data-leaks.txt | 8 +++++
t/t0410-partial-clone.sh | 2 --
t/t1090-sparse-checkout-scope.sh | 1 -
t/t5500-fetch-pack.sh | 10 +++---
t/t5512-ls-remote.sh | 14 ++++++++
t/t5516-fetch-push.sh | 50 +++++++++++++--------------
t/t5551-http-fetch-smart.sh | 4 ---
t/t5601-clone.sh | 3 --
t/t5616-partial-clone.sh | 8 +----
t/t5702-protocol-v2.sh | 1 -
t/t7406-submodule-update.sh | 5 +--
upload-pack.c | 18 +++-------
13 files changed, 87 insertions(+), 85 deletions(-)
diff --git a/Documentation/config/uploadpack.txt b/Documentation/config/uploadpack.txt
index ed1c835695c..854801bd6cc 100644
--- a/Documentation/config/uploadpack.txt
+++ b/Documentation/config/uploadpack.txt
@@ -1,31 +1,41 @@
uploadpack.hideRefs::
This variable is the same as `transfer.hideRefs`, but applies
only to `upload-pack` (and so affects only fetches, not pushes).
- An attempt to fetch a hidden ref by `git fetch` will fail. See
- also `uploadpack.allowTipSHA1InWant`.
++
+This can be used in some setups to reduce the size of the ref
+advertisement, see also `protocol.version` for using a protocol
+version that allows them to be filtered by the client.
++
+In earlier versions of Git attempts to fetch an object pointed to by a
+hidden ref would fail, this is no longer the case. See
+`uploadpack.allowAnySHA1InWant`.
uploadpack.allowTipSHA1InWant::
- When `uploadpack.hideRefs` is in effect, allow `upload-pack`
- to accept a fetch request that asks for an object at the tip
- of a hidden ref (by default, such a request is rejected).
- See also `uploadpack.hideRefs`. Even if this is false, a client
- may be able to steal objects via the techniques described in the
- "SECURITY" section of the linkgit:gitnamespaces[7] man page; it's
- best to keep private data in a separate repository.
+ Depreacted. Used to allow `upload-pack` to accept a fetch
+ request that asked for an object at the tip of a hidden
+ ref. Now always `true`. See `uploadpack.allowAnySHA1InWant`
+ below for details.
uploadpack.allowReachableSHA1InWant::
- Allow `upload-pack` to accept a fetch request that asks for an
- object that is reachable from any ref tip. However, note that
- calculating object reachability is computationally expensive.
- Defaults to `false`. Even if this is false, a client may be able
- to steal objects via the techniques described in the "SECURITY"
- section of the linkgit:gitnamespaces[7] man page; it's best to
- keep private data in a separate repository.
+ Depreacted. Used to allow `upload-pack` to accept a fetch
+ request that asked for an object that was reachable from any
+ ref tip. Now always `true`. See
+ `uploadpack.allowAnySHA1InWant` below for details.
uploadpack.allowAnySHA1InWant::
- Allow `upload-pack` to accept a fetch request that asks for any
- object at all.
- Defaults to `false`.
+ Deprecated. Before Git version 2.21 the
+ `uploadpack.allow{Tip,Any,Reachable}SHA1InWant` settings were
+ `false` by default. Now they're unconditionally `true`, which
+ means fetch request that ask for any object in the object
+ store will be accepted, regardless of whether such an object
+ is at the tip of a ref, reachable by no ref etc.
++
+The reason for the change in default is as noted in the "SECURITY"
+section of linkgit:git-fetch[1] a determined client can get to any
+previously forbidden objects the server has access to by manipulating
+the client/server dialog. If you host objects that should be kept
+secret from some clients it's best to keep them in a separate
+repository.
uploadpack.keepAlive::
When `upload-pack` has started `pack-objects`, there may be a
diff --git a/Documentation/transfer-data-leaks.txt b/Documentation/transfer-data-leaks.txt
index 914bacc39e0..f8beb334b84 100644
--- a/Documentation/transfer-data-leaks.txt
+++ b/Documentation/transfer-data-leaks.txt
@@ -28,3 +28,11 @@ The known attack vectors are as follows:
an object Y that the attacker already has, and the attacker falsely
claims to have X and not Y, so the victim sends Y as a delta against X.
The delta reveals regions of X that are similar to Y to the attacker.
+
+Before Git version 2.21 the
+`uploadpack.allow{Tip,Any,Reachable}SHA1InWant` allowed for tweaking
+linkgit:git-upload-pack[1] behavior to refuse to serve some refs. As
+noted above this served to lull users into a false sense of security,
+and the fetch protocol will now allow clients to fetch any object in
+the ref store. See `uploadpack.allowAnySHA1InWant` in
+linkgit:git-config[1].
diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
index ba3887f178b..a9d24a94d9f 100755
--- a/t/t0410-partial-clone.sh
+++ b/t/t0410-partial-clone.sh
@@ -192,7 +192,6 @@ test_expect_success 'fetching of missing blobs works' '
git hash-object repo/foo.t >blobhash &&
rm -rf repo/.git/objects/* &&
- git -C server config uploadpack.allowanysha1inwant 1 &&
git -C server config uploadpack.allowfilter 1 &&
git -C repo config core.repositoryformatversion 1 &&
git -C repo config extensions.partialclone "origin" &&
@@ -211,7 +210,6 @@ test_expect_success 'fetching of missing trees does not fetch blobs' '
git hash-object repo/foo.t >blobhash &&
rm -rf repo/.git/objects/* &&
- git -C server config uploadpack.allowanysha1inwant 1 &&
git -C server config uploadpack.allowfilter 1 &&
git -C repo config core.repositoryformatversion 1 &&
git -C repo config extensions.partialclone "origin" &&
diff --git a/t/t1090-sparse-checkout-scope.sh b/t/t1090-sparse-checkout-scope.sh
index 090b7fc3d35..4de2e2dd1bb 100755
--- a/t/t1090-sparse-checkout-scope.sh
+++ b/t/t1090-sparse-checkout-scope.sh
@@ -68,7 +68,6 @@ test_expect_success 'in partial clone, sparse checkout only fetches needed blobs
git clone "file://$(pwd)/server" client &&
test_config -C server uploadpack.allowfilter 1 &&
- test_config -C server uploadpack.allowanysha1inwant 1 &&
echo a >server/a &&
echo bb >server/b &&
mkdir server/c &&
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 086f2c40f68..b639629863d 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -592,8 +592,7 @@ test_expect_success 'fetch-pack can fetch a raw sha1' '
test_commit 1 &&
test_commit 2 &&
git update-ref refs/hidden/one HEAD^ &&
- git config transfer.hiderefs refs/hidden &&
- git config uploadpack.allowtipsha1inwant true
+ git config transfer.hiderefs refs/hidden
) &&
git fetch-pack hidden $(git -C hidden rev-parse refs/hidden/one)
'
@@ -619,7 +618,7 @@ test_expect_success 'fetch-pack can fetch a raw sha1 overlapping a named ref' '
$(git -C server rev-parse refs/tags/1) refs/tags/1
'
-test_expect_success 'fetch-pack cannot fetch a raw sha1 that is not advertised as a ref' '
+test_expect_success 'fetch-pack can fetch a raw sha1 that is not advertised as a ref' '
rm -rf server &&
git init server &&
@@ -628,9 +627,8 @@ test_expect_success 'fetch-pack cannot fetch a raw sha1 that is not advertised a
test_commit -C server 6 &&
git init client &&
- test_must_fail git -C client fetch-pack ../server \
- $(git -C server rev-parse refs/heads/master^) 2>err &&
- test_i18ngrep "Server does not allow request for unadvertised object" err
+ git -C client fetch-pack ../server \
+ $(git -C server rev-parse refs/heads/master^)
'
check_prot_path () {
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 32e722db2ed..0cd1aad9597 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -195,6 +195,20 @@ do
! grep refs/tags/magic/one actual
'
+
+ test_expect_success "fetching a divergent object hidden by $configsection.hiderefs" "
+ test_when_finished 'test_unconfig $configsection.hiderefs' &&
+ git config --add $configsection.hiderefs refs/tags &&
+ test_when_finished 'git tag -d magic/divergent' &&
+ git tag -a -m'a tag' magic/divergent &&
+ SHA1_TAG=\$(git rev-parse magic/divergent) &&
+ echo \$SHA1_TAG >expect &&
+ git init client &&
+ test_when_finished 'rm -r client' &&
+ git -C client fetch ../ \$SHA1_TAG:refs/tags/magic/divergent &&
+ git -C client rev-parse magic/divergent >actual &&
+ test_cmp expect actual
+ "
done
test_expect_success 'overrides work between mixed transfer/upload-pack hideRefs' '
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 37e8e80893d..8f2b12c17a5 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1147,26 +1147,14 @@ test_expect_success 'fetch exact SHA1' '
git prune &&
test_must_fail git cat-file -t $the_commit &&
- # fetching the hidden object should fail by default
- test_must_fail git fetch -v ../testrepo $the_commit:refs/heads/copy 2>err &&
- test_i18ngrep "Server does not allow request for unadvertised object" err &&
- test_must_fail git rev-parse --verify refs/heads/copy &&
+ # fetching the object should work
+ git fetch -v ../testrepo $the_commit:refs/heads/copy &&
+ git rev-parse --verify refs/heads/copy &&
- # the server side can allow it to succeed
- (
- cd ../testrepo &&
- git config uploadpack.allowtipsha1inwant true
- ) &&
-
- git fetch -v ../testrepo $the_commit:refs/heads/copy master:refs/heads/extra &&
cat >expect <<-EOF &&
$the_commit
- $the_first_commit
EOF
- {
- git rev-parse --verify refs/heads/copy &&
- git rev-parse --verify refs/heads/extra
- } >actual &&
+ git rev-parse --verify refs/heads/copy >actual &&
test_cmp expect actual
)
'
@@ -1190,6 +1178,25 @@ test_expect_success 'fetch exact SHA1 in protocol v2' '
git -C child fetch -v ../testrepo $the_commit:refs/heads/copy
'
+for state in true false
+do
+ test_expect_success "Deprecated uploadpack.*=$state settings warn" "
+ mk_empty deprecated-warnings &&
+ test_commit -C deprecated-warnings A &&
+ test_config -C deprecated-warnings uploadpack.allowTipSHA1InWant $state &&
+ test_config -C deprecated-warnings uploadpack.allowReachableSHA1InWant $state &&
+ test_config -C deprecated-warnings uploadpack.allowAnySHA1InWant $state &&
+ mk_empty target &&
+ (
+ cd target &&
+ git fetch ../deprecated-warnings/.git refs/tags/A:refs/tags/A 2>stderr &&
+ test_i18ngrep 'warning: .*uploadpack.allowTipSHA1InWant .*deprecated' stderr &&
+ test_i18ngrep 'warning: .*uploadpack.allowReachableSHA1InWant.* deprecated' stderr &&
+ test_i18ngrep 'warning: .*uploadpack.allowAnySHA1InWant.* deprecated' stderr
+ )
+ "
+done
+
for configallowtipsha1inwant in true false
do
test_expect_success "shallow fetch reachable SHA1 (but not a ref), allowtipsha1inwant=$configallowtipsha1inwant" '
@@ -1204,8 +1211,6 @@ do
mk_empty shallow &&
(
cd shallow &&
- test_must_fail git fetch --depth=1 ../testrepo/.git $SHA1 &&
- git --git-dir=../testrepo/.git config uploadpack.allowreachablesha1inwant true &&
git fetch --depth=1 ../testrepo/.git $SHA1 &&
git cat-file commit $SHA1
)
@@ -1232,15 +1237,10 @@ do
mk_empty shallow &&
(
cd shallow &&
- test_must_fail ok=sigpipe git fetch ../testrepo/.git $SHA1_3 &&
- test_must_fail ok=sigpipe git fetch ../testrepo/.git $SHA1_1 &&
- git --git-dir=../testrepo/.git config uploadpack.allowreachablesha1inwant true &&
+ git fetch ../testrepo/.git $SHA1_3 &&
git fetch ../testrepo/.git $SHA1_1 &&
git cat-file commit $SHA1_1 &&
- test_must_fail git cat-file commit $SHA1_2 &&
- git fetch ../testrepo/.git $SHA1_2 &&
- git cat-file commit $SHA1_2 &&
- test_must_fail ok=sigpipe git fetch ../testrepo/.git $SHA1_3
+ git cat-file commit $SHA1_2
)
'
done
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index 8630b0cc390..b8f7c0b5563 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -283,7 +283,6 @@ test_expect_success 'test allowreachablesha1inwant' '
test_when_finished "rm -rf test_reachable.git" &&
server="$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
master_sha=$(git -C "$server" rev-parse refs/heads/master) &&
- git -C "$server" config uploadpack.allowreachablesha1inwant 1 &&
git init --bare test_reachable.git &&
git -C test_reachable.git remote add origin "$HTTPD_URL/smart/repo.git" &&
@@ -302,7 +301,6 @@ test_expect_success 'test allowreachablesha1inwant with unreachable' '
server="$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
master_sha=$(git -C "$server" rev-parse refs/heads/master) &&
- git -C "$server" config uploadpack.allowreachablesha1inwant 1 &&
git init --bare test_reachable.git &&
git -C test_reachable.git remote add origin "$HTTPD_URL/smart/repo.git" &&
@@ -321,13 +319,11 @@ test_expect_success 'test allowanysha1inwant with unreachable' '
server="$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
master_sha=$(git -C "$server" rev-parse refs/heads/master) &&
- git -C "$server" config uploadpack.allowreachablesha1inwant 1 &&
git init --bare test_reachable.git &&
git -C test_reachable.git remote add origin "$HTTPD_URL/smart/repo.git" &&
test_must_fail git -C test_reachable.git fetch origin "$(git rev-parse HEAD)" &&
- git -C "$server" config uploadpack.allowanysha1inwant 1 &&
git -C test_reachable.git fetch origin "$(git rev-parse HEAD)"
'
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 8bbc7068acb..ec7a3c46528 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -646,7 +646,6 @@ partial_clone () {
test_commit -C "$SERVER" two &&
HASH2=$(git hash-object "$SERVER/two.t") &&
test_config -C "$SERVER" uploadpack.allowfilter 1 &&
- test_config -C "$SERVER" uploadpack.allowanysha1inwant 1 &&
git clone --filter=blob:limit=0 "$URL" client &&
@@ -689,7 +688,6 @@ test_expect_success 'batch missing blob request during checkout' '
git -C server commit -m x &&
test_config -C server uploadpack.allowfilter 1 &&
- test_config -C server uploadpack.allowanysha1inwant 1 &&
git clone --filter=blob:limit=0 "file://$(pwd)/server" client &&
@@ -720,7 +718,6 @@ test_expect_success 'batch missing blob request does not inadvertently try to fe
git -C server commit -m x &&
test_config -C server uploadpack.allowfilter 1 &&
- test_config -C server uploadpack.allowanysha1inwant 1 &&
# Make sure that it succeeds
git clone --filter=blob:limit=0 "file://$(pwd)/server" client
diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
index 336f02a41a6..213899a690d 100755
--- a/t/t5616-partial-clone.sh
+++ b/t/t5616-partial-clone.sh
@@ -25,8 +25,7 @@ test_expect_success 'setup normal src repo' '
# bare clone "src" giving "srv.bare" for use as our server.
test_expect_success 'setup bare clone for server' '
git clone --bare "file://$(pwd)/src" srv.bare &&
- git -C srv.bare config --local uploadpack.allowfilter 1 &&
- git -C srv.bare config --local uploadpack.allowanysha1inwant 1
+ git -C srv.bare config --local uploadpack.allowfilter 1
'
# do basic partial clone from "srv.bare"
@@ -159,7 +158,6 @@ test_expect_success 'partial clone with transfer.fsckobjects=1 uses index-pack -
git init src &&
test_commit -C src x &&
test_config -C src uploadpack.allowfilter 1 &&
- test_config -C src uploadpack.allowanysha1inwant 1 &&
GIT_TRACE="$(pwd)/trace" git -c transfer.fsckobjects=1 \
clone --filter="blob:none" "file://$(pwd)/src" dst &&
@@ -213,7 +211,6 @@ test_expect_success 'partial clone fetches blobs pointed to by refs even if norm
git init src &&
test_commit -C src x &&
test_config -C src uploadpack.allowfilter 1 &&
- test_config -C src uploadpack.allowanysha1inwant 1 &&
# Create a tag pointing to a blob.
BLOB=$(echo blob-contents | git -C src hash-object --stdin -w) &&
@@ -229,7 +226,6 @@ test_expect_success 'fetch what is specified on CLI even if already promised' '
git init src &&
test_commit -C src foo &&
test_config -C src uploadpack.allowfilter 1 &&
- test_config -C src uploadpack.allowanysha1inwant 1 &&
git hash-object --stdin <src/foo.t >blob &&
@@ -257,7 +253,6 @@ test_expect_success 'upon cloning, check that all refs point to objects' '
test_create_repo "$SERVER" &&
test_commit -C "$SERVER" foo &&
test_config -C "$SERVER" uploadpack.allowfilter 1 &&
- test_config -C "$SERVER" uploadpack.allowanysha1inwant 1 &&
# Create a tag pointing to a blob.
BLOB=$(echo blob-contents | git -C "$SERVER" hash-object --stdin -w) &&
@@ -293,7 +288,6 @@ test_expect_success 'when partial cloning, tolerate server not sending target of
test_create_repo "$SERVER" &&
test_commit -C "$SERVER" foo &&
test_config -C "$SERVER" uploadpack.allowfilter 1 &&
- test_config -C "$SERVER" uploadpack.allowanysha1inwant 1 &&
# Create an annotated tag pointing to a blob.
BLOB=$(echo blob-contents | git -C "$SERVER" hash-object --stdin -w) &&
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 0f2b09ebb8d..af16f139a4a 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -274,7 +274,6 @@ test_expect_success 'setup filter tests' '
test_commit -C server message2 a.txt &&
git -C server config protocol.version 2 &&
git -C server config uploadpack.allowfilter 1 &&
- git -C server config uploadpack.allowanysha1inwant 1 &&
git -C server config protocol.version 2
'
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index e87164aa8ff..cf5bda08690 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -943,10 +943,7 @@ test_expect_success 'submodule update clone shallow submodule outside of depth'
cd super3 &&
sed -e "s#url = ../#url = file://$pwd/#" <.gitmodules >.gitmodules.tmp &&
mv -f .gitmodules.tmp .gitmodules &&
- test_must_fail git submodule update --init --depth=1 2>actual &&
- test_i18ngrep "Direct fetching of that commit failed." actual &&
- git -C ../submodule config uploadpack.allowReachableSHA1InWant true &&
- git submodule update --init --depth=1 >actual &&
+ git submodule update --init --depth=1 &&
git -C submodule log --oneline >out &&
test_line_count = 1 out
)
diff --git a/upload-pack.c b/upload-pack.c
index 5e81f1ff24f..18e4c2d2452 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -54,7 +54,8 @@ static int no_progress, daemon_mode;
#define ALLOW_REACHABLE_SHA1 02
/* Allow request of any sha1. Implies ALLOW_TIP_SHA1 and ALLOW_REACHABLE_SHA1. */
#define ALLOW_ANY_SHA1 07
-static unsigned int allow_unadvertised_object_request;
+static unsigned int allow_unadvertised_object_request = (
+ ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1 | ALLOW_ANY_SHA1);
static int shallow_nr;
static struct object_array extra_edge_obj;
static unsigned int timeout;
@@ -1019,20 +1020,11 @@ static int find_symref(const char *refname, const struct object_id *oid,
static int upload_pack_config(const char *var, const char *value, void *unused)
{
if (!strcmp("uploadpack.allowtipsha1inwant", var)) {
- if (git_config_bool(var, value))
- allow_unadvertised_object_request |= ALLOW_TIP_SHA1;
- else
- allow_unadvertised_object_request &= ~ALLOW_TIP_SHA1;
+ warning(_("The uploadpack.allowTipSHA1InWant setting is deprecated, and now always 'true'"));
} else if (!strcmp("uploadpack.allowreachablesha1inwant", var)) {
- if (git_config_bool(var, value))
- allow_unadvertised_object_request |= ALLOW_REACHABLE_SHA1;
- else
- allow_unadvertised_object_request &= ~ALLOW_REACHABLE_SHA1;
+ warning(_("The uploadpack.allowReachableSHA1InWant setting is deprecated, and now always 'true'"));
} else if (!strcmp("uploadpack.allowanysha1inwant", var)) {
- if (git_config_bool(var, value))
- allow_unadvertised_object_request |= ALLOW_ANY_SHA1;
- else
- allow_unadvertised_object_request &= ~ALLOW_ANY_SHA1;
+ warning(_("The uploadpack.allowAnySHA1InWant setting is deprecated, and now always 'true'"));
} else if (!strcmp("uploadpack.keepalive", var)) {
keepalive = git_config_int(var, value);
if (!keepalive)
--
2.20.0.405.gbc1bbc6f85
^ permalink raw reply related [flat|nested] 73+ messages in thread
* Re: [PATCH] upload-pack: turn on uploadpack.allowAnySHA1InWant=true
2018-12-17 22:16 ` [PATCH] upload-pack: turn on uploadpack.allowAnySHA1InWant=true Ævar Arnfjörð Bjarmason
@ 2018-12-17 22:34 ` David Turner
2018-12-17 22:57 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 73+ messages in thread
From: David Turner @ 2018-12-17 22:34 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason, git
Cc: Junio C Hamano, Jeff King, Brandon Williams, Jonathan Tan,
Fredrik Medley, Matt McCutchen
Overall, I like this. One nit:
On December 17, 2018 5:16:25 PM EST, "Ævar Arnfjörð Bjarmason" <avarab@gmail.com> wrote:
>--- a/upload-pack.c
>+++ b/upload-pack.c
>@@ -54,7 +54,8 @@ static int no_progress, daemon_mode;
> #define ALLOW_REACHABLE_SHA1 02
>/* Allow request of any sha1. Implies ALLOW_TIP_SHA1 and
>ALLOW_REACHABLE_SHA1. */
> #define ALLOW_ANY_SHA1 07
>-static unsigned int allow_unadvertised_object_request;
>+static unsigned int allow_unadvertised_object_request = (
>+ ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1 | ALLOW_ANY_SHA1);
ALLOW_ANY_SHA1 already includes the other two.
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH] upload-pack: turn on uploadpack.allowAnySHA1InWant=true
2018-12-17 22:34 ` David Turner
@ 2018-12-17 22:57 ` Ævar Arnfjörð Bjarmason
2018-12-17 23:07 ` David Turner
0 siblings, 1 reply; 73+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-12-17 22:57 UTC (permalink / raw)
To: David Turner
Cc: git, Junio C Hamano, Jeff King, Brandon Williams, Jonathan Tan,
Fredrik Medley, Matt McCutchen
On Mon, Dec 17 2018, David Turner wrote:
> Overall, I like this. One nit:
Thanks for the review!
> On December 17, 2018 5:16:25 PM EST, "Ævar Arnfjörð Bjarmason" <avarab@gmail.com> wrote:
>>--- a/upload-pack.c
>>+++ b/upload-pack.c
>>@@ -54,7 +54,8 @@ static int no_progress, daemon_mode;
>> #define ALLOW_REACHABLE_SHA1 02
>>/* Allow request of any sha1. Implies ALLOW_TIP_SHA1 and
>>ALLOW_REACHABLE_SHA1. */
>> #define ALLOW_ANY_SHA1 07
>>-static unsigned int allow_unadvertised_object_request;
>>+static unsigned int allow_unadvertised_object_request = (
>>+ ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1 | ALLOW_ANY_SHA1);
>
> ALLOW_ANY_SHA1 already includes the other two.
FWIW (and maybe I made the wrong call, and have no preference realy) I
opted for this as part of "this change doesn't do any of the hard work
of extracting the now-dead ALLOW_[...]*.
I.e. the diff context I had doesn't show all the ALLOW_* values, and
with the context given it might be easier for reviewers to look no
further than "this is what you'd get before if all
uploadpack.allow.*SHA1InWant was true".
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH] upload-pack: turn on uploadpack.allowAnySHA1InWant=true
2018-12-17 22:57 ` Ævar Arnfjörð Bjarmason
@ 2018-12-17 23:07 ` David Turner
0 siblings, 0 replies; 73+ messages in thread
From: David Turner @ 2018-12-17 23:07 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, Jeff King, Brandon Williams, Jonathan Tan,
Fredrik Medley, Matt McCutchen
On December 17, 2018 5:57:50 PM EST, "Ævar Arnfjörð Bjarmason" <avarab@gmail.com> wrote:
>
>On Mon, Dec 17 2018, David Turner wrote:
>
>> Overall, I like this. One nit:
>
>Thanks for the review!
>
>> On December 17, 2018 5:16:25 PM EST, "Ævar Arnfjörð Bjarmason"
><avarab@gmail.com> wrote:
>>>--- a/upload-pack.c
>>>+++ b/upload-pack.c
>>>@@ -54,7 +54,8 @@ static int no_progress, daemon_mode;
>>> #define ALLOW_REACHABLE_SHA1 02
>>>/* Allow request of any sha1. Implies ALLOW_TIP_SHA1 and
>>>ALLOW_REACHABLE_SHA1. */
>>> #define ALLOW_ANY_SHA1 07
>>>-static unsigned int allow_unadvertised_object_request;
>>>+static unsigned int allow_unadvertised_object_request = (
>>>+ ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1 | ALLOW_ANY_SHA1);
>>
>> ALLOW_ANY_SHA1 already includes the other two.
>
>FWIW (and maybe I made the wrong call, and have no preference realy) I
>opted for this as part of "this change doesn't do any of the hard work
>of extracting the now-dead ALLOW_[...]*.
>
>I.e. the diff context I had doesn't show all the ALLOW_* values, and
>with the context given it might be easier for reviewers to look no
>further than "this is what you'd get before if all
>uploadpack.allow.*SHA1InWant was true".
The context I quoted said /* Allow request of any sha1. Implies ALLOW_TIP_SHA1 and
ALLOW_REACHABLE_SHA1. */
But up to you (or Junio).
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2
2018-12-17 19:57 ` Jeff King
2018-12-17 22:16 ` [PATCH] upload-pack: turn on uploadpack.allowAnySHA1InWant=true Ævar Arnfjörð Bjarmason
@ 2018-12-17 23:14 ` Jonathan Nieder
2018-12-17 23:36 ` Ævar Arnfjörð Bjarmason
2018-12-18 12:36 ` Jeff King
1 sibling, 2 replies; 73+ messages in thread
From: Jonathan Nieder @ 2018-12-17 23:14 UTC (permalink / raw)
To: Jeff King
Cc: Ævar Arnfjörð Bjarmason, git, Junio C Hamano,
Brandon Williams, Jonathan Tan
Hi,
Jeff King wrote:
> On Fri, Dec 14, 2018 at 11:55:30AM +0100, Ævar Arnfjörð Bjarmason wrote:
>> More importantly this bypasses the security guarantee we've had with the
>> default of uploadpack.allowAnySHA1InWant=false.
>
> IMHO those security guarantees there are overrated (due to delta
> guessing attacks, though things are not quite as bad if the attacker
> can't actually push to the repo).
Do you have a proof of concept for delta guessing? My understanding
was that without using a broken hash (e.g. uncorrected SHA-1), it is
not feasible to carry out.
JGit checks delta bases in received thin packs for reachability as
well.
> But I agree that people do assume it's the case. I was certainly
> surprised by the v2 behavior, and I don't remember that aspect being
> discussed.
IMHO it's a plain bug (either in implementation or documentation).
[...]
>> I'm inclined to say that in the face of that "SECURITY" section we
>> should just:
>>
>> * Turn on uploadpack.allowReachableSHA1InWant for v0/v1 by
>> default. Make saying uploadpack.allowReachableSHA1InWant=false warn
>> with "this won't work, see SECURITY...".
>>
>> * The uploadpack.allowTipSHA1InWant setting will also be turned on by
>> default, and will be much faster, since it'll just degrade to
>> uploadpack.allowReachableSHA1InWant=true and we won't need any
>> reachability check. We'll also warn saying that setting it is
>> useless.
>
> No real argument from me. I have always thought those security
> guarantees were BS.
This would make per-branch ACLs (as implemented both by Gerrit and
gitolite) an essentially useless feature, so please no.
I would be all for changing the default, but making turning off
allowReachableSHA1InWant an unsupported deprecated thing is a step too
far, in my opinion.
Is there somewhere that we can document these kinds of invariants or
goals so that we don't have to keep repeating the same discussions?
Thanks,
Jonathan
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2
2018-12-17 23:14 ` [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2 Jonathan Nieder
@ 2018-12-17 23:36 ` Ævar Arnfjörð Bjarmason
2018-12-18 0:02 ` Jonathan Nieder
2018-12-18 12:36 ` Jeff King
1 sibling, 1 reply; 73+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-12-17 23:36 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Jeff King, git, Junio C Hamano, Brandon Williams, Jonathan Tan
On Mon, Dec 17 2018, Jonathan Nieder wrote:
> Hi,
>
> Jeff King wrote:
>> On Fri, Dec 14, 2018 at 11:55:30AM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>>> More importantly this bypasses the security guarantee we've had with the
>>> default of uploadpack.allowAnySHA1InWant=false.
>>
>> IMHO those security guarantees there are overrated (due to delta
>> guessing attacks, though things are not quite as bad if the attacker
>> can't actually push to the repo).
>
> Do you have a proof of concept for delta guessing? My understanding
> was that without using a broken hash (e.g. uncorrected SHA-1), it is
> not feasible to carry out.
>
> JGit checks delta bases in received thin packs for reachability as
> well.
>
>> But I agree that people do assume it's the case. I was certainly
>> surprised by the v2 behavior, and I don't remember that aspect being
>> discussed.
>
> IMHO it's a plain bug (either in implementation or documentation).
>
> [...]
>>> I'm inclined to say that in the face of that "SECURITY" section we
>>> should just:
>>>
>>> * Turn on uploadpack.allowReachableSHA1InWant for v0/v1 by
>>> default. Make saying uploadpack.allowReachableSHA1InWant=false warn
>>> with "this won't work, see SECURITY...".
>>>
>>> * The uploadpack.allowTipSHA1InWant setting will also be turned on by
>>> default, and will be much faster, since it'll just degrade to
>>> uploadpack.allowReachableSHA1InWant=true and we won't need any
>>> reachability check. We'll also warn saying that setting it is
>>> useless.
>>
>> No real argument from me. I have always thought those security
>> guarantees were BS.
>
> This would make per-branch ACLs (as implemented both by Gerrit and
> gitolite) an essentially useless feature, so please no.
Doesn't Gerrit always use JGit?
The discussion upthread is about what we should do about git.git's
version of this feature since we document it doesn't do its job from a
security / ACL standpoint, but that doesn't preclude other server
implementations from having a working version of this.
But if gitolite is relying on this to hide refs, and if our security
docs are to be believed, then it's just providing security through
obscurity.
Like you I'm curious about a POC. The patch I submitted in
<20181217221625.1523-1-avarab@gmail.com> takes the "SECURITY" docs at
face value. I.e. I think it's dangerous to expose this as-is given the
scary non-promise they make, but perhaps we'll find that this actually
works well enough in some cases, or there's other non-security uses for
this (e.g. simply discouraging users from cloning certain things,
e.g. for cache performance purposes).
> I would be all for changing the default, but making turning off
> allowReachableSHA1InWant an unsupported deprecated thing is a step too
> far, in my opinion.
>
> Is there somewhere that we can document these kinds of invariants or
> goals so that we don't have to keep repeating the same discussions?
>
> Thanks,
> Jonathan
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2
2018-12-17 23:36 ` Ævar Arnfjörð Bjarmason
@ 2018-12-18 0:02 ` Jonathan Nieder
2018-12-18 9:28 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 73+ messages in thread
From: Jonathan Nieder @ 2018-12-18 0:02 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Jeff King, git, Junio C Hamano, Brandon Williams, Jonathan Tan
Ævar Arnfjörð Bjarmason wrote:
> On Mon, Dec 17 2018, Jonathan Nieder wrote:
>> This would make per-branch ACLs (as implemented both by Gerrit and
>> gitolite) an essentially useless feature, so please no.
>
> Doesn't Gerrit always use JGit?
>
> The discussion upthread is about what we should do about git.git's
> version of this feature since we document it doesn't do its job from a
> security / ACL standpoint, but that doesn't preclude other server
> implementations from having a working version of this.
>
> But if gitolite is relying on this to hide refs, and if our security
> docs are to be believed, then it's just providing security through
> obscurity.
Yes, Gerrit uses JGit. JGit shares configuration with Git, so if we
make that configuration in Git warn "This is just a placebo", then
people will naturally assume that it's just a placebo in JGit, too.
More importantly, if Git upstream stops caring about this use case,
then the protocol and other features can evolve in directions that
make it even harder to support. I am willing to take on some of the
burden of keeping it working, even when I do not run any servers that
use plain Git (I do interact with many).
> Like you I'm curious about a POC. The patch I submitted in
> <20181217221625.1523-1-avarab@gmail.com> takes the "SECURITY" docs at
> face value.
One of the difficulties that security engineers have to deal with is
living without absolutes. In other words, their experience is
constant risk/benefit tradeoffs: if you want 0% risk, then don't use a
computer. ;-)
If someone has thoughts on what would lead people to be comfortable
with removing the SECURITY notice, then I'm happy to listen. As
already mentioned, I am strongly against abandoning this use case.
Jonathan
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2
2018-12-18 0:02 ` Jonathan Nieder
@ 2018-12-18 9:28 ` Ævar Arnfjörð Bjarmason
2018-12-18 12:41 ` Jeff King
0 siblings, 1 reply; 73+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-12-18 9:28 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Jeff King, git, Junio C Hamano, Brandon Williams, Jonathan Tan
On Tue, Dec 18 2018, Jonathan Nieder wrote:
> Ævar Arnfjörð Bjarmason wrote:
>> On Mon, Dec 17 2018, Jonathan Nieder wrote:
>
>>> This would make per-branch ACLs (as implemented both by Gerrit and
>>> gitolite) an essentially useless feature, so please no.
>>
>> Doesn't Gerrit always use JGit?
>>
>> The discussion upthread is about what we should do about git.git's
>> version of this feature since we document it doesn't do its job from a
>> security / ACL standpoint, but that doesn't preclude other server
>> implementations from having a working version of this.
>>
>> But if gitolite is relying on this to hide refs, and if our security
>> docs are to be believed, then it's just providing security through
>> obscurity.
>
> Yes, Gerrit uses JGit. JGit shares configuration with Git, so if we
> make that configuration in Git warn "This is just a placebo", then
> people will naturally assume that it's just a placebo in JGit, too.
Aside from the merits of this change it sounds like a good idea to
document the server options JGit shares with us somewhere, or have a
test mode similar to what I added in (but didn't follow-up on
integrating) in
https://public-inbox.org/git/20170516203712.15921-1-avarab@gmail.com/
> More importantly, if Git upstream stops caring about this use case,
> then the protocol and other features can evolve in directions that
> make it even harder to support. I am willing to take on some of the
> burden of keeping it working, even when I do not run any servers that
> use plain Git (I do interact with many).
>
>> Like you I'm curious about a POC. The patch I submitted in
>> <20181217221625.1523-1-avarab@gmail.com> takes the "SECURITY" docs at
>> face value.
>
> One of the difficulties that security engineers have to deal with is
> living without absolutes. In other words, their experience is
> constant risk/benefit tradeoffs: if you want 0% risk, then don't use a
> computer. ;-)
>
> If someone has thoughts on what would lead people to be comfortable
> with removing the SECURITY notice, then I'm happy to listen. As
> already mentioned, I am strongly against abandoning this use case.
For me this would be docs backed up by tests (which can start as a ML
reply) showing a case where this actually works to hide data.
I.e. have a repo with "master" and a "root-password" branch, where the
"root-password" branch has content that's irresistible to "git repack"
for delta purposes, do we end up sending the "root-password" content
over on a fetch even when that branch isn't advertised & forbidden?
Or, if that fails are there ways to make it work? E.g. using hidden/* in
combination with delta islands?
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2
2018-12-18 9:28 ` Ævar Arnfjörð Bjarmason
@ 2018-12-18 12:41 ` Jeff King
0 siblings, 0 replies; 73+ messages in thread
From: Jeff King @ 2018-12-18 12:41 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Jonathan Nieder, git, Junio C Hamano, Brandon Williams,
Jonathan Tan
On Tue, Dec 18, 2018 at 10:28:27AM +0100, Ævar Arnfjörð Bjarmason wrote:
> I.e. have a repo with "master" and a "root-password" branch, where the
> "root-password" branch has content that's irresistible to "git repack"
> for delta purposes, do we end up sending the "root-password" content
> over on a fetch even when that branch isn't advertised & forbidden?
>
> Or, if that fails are there ways to make it work? E.g. using hidden/* in
> combination with delta islands?
Delta islands wouldn't generally help here. They only tell us not to
store on-disk deltas that fetching clients aren't likely to be able to
reuse (i.e, they want X but will generally not have Y, so don't make a
delta there).
In the attacks I mentioned in my previous email, the deltas would
usually be computed on the fly for each fetch. So the lack of on-disk
deltas across "security boundaries" wouldn't help.
You could disable on-the-fly delta compression, but the resulting packs
are much larger (and you'd think it would save some server CPU, but
experiments I've done show that it helps a lot less than you'd think,
since we often end up zlib-deflating more bytes).
-Peff
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2
2018-12-17 23:14 ` [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2 Jonathan Nieder
2018-12-17 23:36 ` Ævar Arnfjörð Bjarmason
@ 2018-12-18 12:36 ` Jeff King
2018-12-18 13:10 ` Ævar Arnfjörð Bjarmason
1 sibling, 1 reply; 73+ messages in thread
From: Jeff King @ 2018-12-18 12:36 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Ævar Arnfjörð Bjarmason, git, Junio C Hamano,
Brandon Williams, Jonathan Tan
On Mon, Dec 17, 2018 at 03:14:52PM -0800, Jonathan Nieder wrote:
> > IMHO those security guarantees there are overrated (due to delta
> > guessing attacks, though things are not quite as bad if the attacker
> > can't actually push to the repo).
>
> Do you have a proof of concept for delta guessing? My understanding
> was that without using a broken hash (e.g. uncorrected SHA-1), it is
> not feasible to carry out.
I think we may be talking about two different things. I mean an attack
where you want to know what is in object X, so you ask the server for
object Y and tell it that you already have X. If the sender generates a
delta against X, that tells you something about what's in X.
For a pure read-only server, you're restricted to the Y's that are
already in the repo. So how effective this is depends on what's in X,
and what Y's are available.
For a case where X is in a victim repo you cannot make arbitrary writes
to, but you _can_ make the victim repo aware of other objects (e.g., by
opening a pull request that creates a ref), then you can iteratively
provide many Y's, improving your guess about X in each iteration.
For a case where the victim repo has fully shared storage (GitHub, and
probably other hosts; I'm not sure if it's available yet, but GitLab is
clearly working on shared-storage too), you can probably skip all that
and just push a ref pointing to X with an empty pack (Git just cares
that it has all of the objects afterwards, not that you pushed them).
None of those care about the quality of the hash (they do assume you
know the hash of X already, but then so does fetching by object id).
And no, I've never written a proof-of-concept for that. It would depend
largely on the data you're trying to extract. E.g., if you think X
contains "root:XXXXXX", then you might hope to ask for "root:AXXXXX",
then "root:BXXXXX", etc. You know you've got a hit when the delta gets
smaller. So the complexity for guessing N bytes becomes 256*N, rather
than 256^N.
> > But I agree that people do assume it's the case. I was certainly
> > surprised by the v2 behavior, and I don't remember that aspect being
> > discussed.
>
> IMHO it's a plain bug (either in implementation or documentation).
Or both. :) The behavior and the documentation seem to agree.
> [...]
> >> I'm inclined to say that in the face of that "SECURITY" section we
> >> should just:
> >>
> >> * Turn on uploadpack.allowReachableSHA1InWant for v0/v1 by
> >> default. Make saying uploadpack.allowReachableSHA1InWant=false warn
> >> with "this won't work, see SECURITY...".
> >>
> >> * The uploadpack.allowTipSHA1InWant setting will also be turned on by
> >> default, and will be much faster, since it'll just degrade to
> >> uploadpack.allowReachableSHA1InWant=true and we won't need any
> >> reachability check. We'll also warn saying that setting it is
> >> useless.
> >
> > No real argument from me. I have always thought those security
> > guarantees were BS.
>
> This would make per-branch ACLs (as implemented both by Gerrit and
> gitolite) an essentially useless feature, so please no.
I think Ævar's argument is that those are providing a false sense of
security now (at least for read permissions).
Let me clarify my position a little.
I won't claim the existing scheme provides _no_ value (especially the
pure read-only case above is less dicey than the others). It's mostly
that the protections are very hand-wavy. I don't trust them _now_, and I
have little faith that other innocent-looking changes to the object
negotiation and the packing code will not significantly weaken them.
There's no security boundary expressed within Git's code, so there's a
very high chance of information leaking accidentally. A trustable system
would have boundaries built in from the ground up.
Enough people seem to believe otherwise (i.e., that the hand-waving
arguments are worth _something_) that I've never pushed to actually
change the default behavior. But if Ævar wants to try to do so, I'm not
going to stand in my way (hence my "no argument from me").
> I would be all for changing the default, but making turning off
> allowReachableSHA1InWant an unsupported deprecated thing is a step too
> far, in my opinion.
Yes, I agree if we were to go down this road, it probably makes sense to
flip the knobs and let them be "unflipped" if the user wants.
> Is there somewhere that we can document these kinds of invariants or
> goals so that we don't have to keep repeating the same discussions?
It's not clear to me that there's consensus on the invariants or goals.
;)
-Peff
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2
2018-12-18 12:36 ` Jeff King
@ 2018-12-18 13:10 ` Ævar Arnfjörð Bjarmason
2018-12-26 22:14 ` Junio C Hamano
0 siblings, 1 reply; 73+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-12-18 13:10 UTC (permalink / raw)
To: Jeff King
Cc: Jonathan Nieder, git, Junio C Hamano, Brandon Williams,
Jonathan Tan
On Tue, Dec 18 2018, Jeff King wrote:
> On Mon, Dec 17, 2018 at 03:14:52PM -0800, Jonathan Nieder wrote:
>
>> > IMHO those security guarantees there are overrated (due to delta
>> > guessing attacks, though things are not quite as bad if the attacker
>> > can't actually push to the repo).
>>
>> Do you have a proof of concept for delta guessing? My understanding
>> was that without using a broken hash (e.g. uncorrected SHA-1), it is
>> not feasible to carry out.
>
> I think we may be talking about two different things. I mean an attack
> where you want to know what is in object X, so you ask the server for
> object Y and tell it that you already have X. If the sender generates a
> delta against X, that tells you something about what's in X.
>
> For a pure read-only server, you're restricted to the Y's that are
> already in the repo. So how effective this is depends on what's in X,
> and what Y's are available.
>
> For a case where X is in a victim repo you cannot make arbitrary writes
> to, but you _can_ make the victim repo aware of other objects (e.g., by
> opening a pull request that creates a ref), then you can iteratively
> provide many Y's, improving your guess about X in each iteration.
>
> For a case where the victim repo has fully shared storage (GitHub, and
> probably other hosts; I'm not sure if it's available yet, but GitLab is
> clearly working on shared-storage too), you can probably skip all that
> and just push a ref pointing to X with an empty pack (Git just cares
> that it has all of the objects afterwards, not that you pushed them).
>
> None of those care about the quality of the hash (they do assume you
> know the hash of X already, but then so does fetching by object id).
>
> And no, I've never written a proof-of-concept for that. It would depend
> largely on the data you're trying to extract. E.g., if you think X
> contains "root:XXXXXX", then you might hope to ask for "root:AXXXXX",
> then "root:BXXXXX", etc. You know you've got a hit when the delta gets
> smaller. So the complexity for guessing N bytes becomes 256*N, rather
> than 256^N.
>
>> > But I agree that people do assume it's the case. I was certainly
>> > surprised by the v2 behavior, and I don't remember that aspect being
>> > discussed.
>>
>> IMHO it's a plain bug (either in implementation or documentation).
>
> Or both. :) The behavior and the documentation seem to agree.
>
>> [...]
>> >> I'm inclined to say that in the face of that "SECURITY" section we
>> >> should just:
>> >>
>> >> * Turn on uploadpack.allowReachableSHA1InWant for v0/v1 by
>> >> default. Make saying uploadpack.allowReachableSHA1InWant=false warn
>> >> with "this won't work, see SECURITY...".
>> >>
>> >> * The uploadpack.allowTipSHA1InWant setting will also be turned on by
>> >> default, and will be much faster, since it'll just degrade to
>> >> uploadpack.allowReachableSHA1InWant=true and we won't need any
>> >> reachability check. We'll also warn saying that setting it is
>> >> useless.
>> >
>> > No real argument from me. I have always thought those security
>> > guarantees were BS.
>>
>> This would make per-branch ACLs (as implemented both by Gerrit and
>> gitolite) an essentially useless feature, so please no.
>
> I think Ævar's argument is that those are providing a false sense of
> security now (at least for read permissions).
>
> Let me clarify my position a little.
>
> I won't claim the existing scheme provides _no_ value (especially the
> pure read-only case above is less dicey than the others). It's mostly
> that the protections are very hand-wavy. I don't trust them _now_, and I
> have little faith that other innocent-looking changes to the object
> negotiation and the packing code will not significantly weaken them.
> There's no security boundary expressed within Git's code, so there's a
> very high chance of information leaking accidentally. A trustable system
> would have boundaries built in from the ground up.
>
> Enough people seem to believe otherwise (i.e., that the hand-waving
> arguments are worth _something_) that I've never pushed to actually
> change the default behavior. But if Ævar wants to try to do so, I'm not
> going to stand in my way (hence my "no argument from me").
FWIW I don't really care about this, I don't rely on
uploadpack.allow{Tip,Reachable,Any}SHA1InWant=false I'm just on the
side-quest of:
1. Try protocol v2
2. Discover that v2 implictly has uploadpack.allowAnySHA1InWant=true
enabled
3. Some people (including Jonathan) can reasonable read our docs /
seem to have understood this to be a security mechanism
4. What are we going to do about that v1 & v2 discrepancy? [You are
here!]
The genreal ways I see forward from that are:
A) Say that v2 has a security issue and that this is a feature that
works in some circumstances, but given Jeff's explanation here we
should at least improve our "SECURITY" docs to be less handwaivy.
B) Improve security docs, turn uploadpack.allowAnySHA1InWant=true on by
default, allow people to turn it off.
C) Like B) but deprecate
uploadpack.allow{Tip,Reachable,Any}SHA1InWant=false. This is my
patch upthread
D-Z) ???
I'm not set on C), and yeah it's probably overzelous to just rip the
thing out, but then what should we do?
>> I would be all for changing the default, but making turning off
>> allowReachableSHA1InWant an unsupported deprecated thing is a step too
>> far, in my opinion.
>
> Yes, I agree if we were to go down this road, it probably makes sense to
> flip the knobs and let them be "unflipped" if the user wants.
>
>> Is there somewhere that we can document these kinds of invariants or
>> goals so that we don't have to keep repeating the same discussions?
>
> It's not clear to me that there's consensus on the invariants or goals.
> ;)
>
> -Peff
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2
2018-12-18 13:10 ` Ævar Arnfjörð Bjarmason
@ 2018-12-26 22:14 ` Junio C Hamano
2018-12-27 11:26 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 73+ messages in thread
From: Junio C Hamano @ 2018-12-26 22:14 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Jeff King, Jonathan Nieder, git, Brandon Williams, Jonathan Tan
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> The genreal ways I see forward from that are:
>
> A) Say that v2 has a security issue and that this is a feature that
> works in some circumstances, but given Jeff's explanation here we
> should at least improve our "SECURITY" docs to be less handwaivy.
>
> B) Improve security docs, turn uploadpack.allowAnySHA1InWant=true on by
> default, allow people to turn it off.
>
> C) Like B) but deprecate
> uploadpack.allow{Tip,Reachable,Any}SHA1InWant=false. This is my
> patch upthread
>
> D-Z) ???
>
>
> I'm not set on C), and yeah it's probably overzelous to just rip the
> thing out, but then what should we do?
Hmph. The other overzealous thing you could do is to strenthen A
and "fix" the security issue in v2? Which letter comes before A in
the alphabet? ;-)
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2
2018-12-26 22:14 ` Junio C Hamano
@ 2018-12-27 11:26 ` Ævar Arnfjörð Bjarmason
2018-12-27 17:10 ` Jonathan Nieder
0 siblings, 1 reply; 73+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-12-27 11:26 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, Jonathan Nieder, git, Brandon Williams, Jonathan Tan
On Wed, Dec 26 2018, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> The genreal ways I see forward from that are:
>>
>> A) Say that v2 has a security issue and that this is a feature that
>> works in some circumstances, but given Jeff's explanation here we
>> should at least improve our "SECURITY" docs to be less handwaivy.
>>
>> B) Improve security docs, turn uploadpack.allowAnySHA1InWant=true on by
>> default, allow people to turn it off.
>>
>> C) Like B) but deprecate
>> uploadpack.allow{Tip,Reachable,Any}SHA1InWant=false. This is my
>> patch upthread
>>
>> D-Z) ???
>>
>>
>> I'm not set on C), and yeah it's probably overzelous to just rip the
>> thing out, but then what should we do?
>
> Hmph. The other overzealous thing you could do is to strenthen A
> and "fix" the security issue in v2? Which letter comes before A in
> the alphabet? ;-)
Sure, but that being useful is predicated on this supposed security
mechanism being useful and not just security-through-obscurity, as noted
in side-threads I don't think we have a convincing argument either way
(and the one we do have is more on the "it's not secure" side).
Of course we had that with v1 all along, but now that v2 is in released
versions and in this insecure mode, we have a reason to closely look at
whether we need to be issuing security releases, or doubling down on the
"SECURITY" wording in git-fetch and then not carrying the mode forward.
^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2
2018-12-27 11:26 ` Ævar Arnfjörð Bjarmason
@ 2018-12-27 17:10 ` Jonathan Nieder
0 siblings, 0 replies; 73+ messages in thread
From: Jonathan Nieder @ 2018-12-27 17:10 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Junio C Hamano, Jeff King, git, Brandon Williams, Jonathan Tan
Hi,
Ævar Arnfjörð Bjarmason wrote:
> On Wed, Dec 26 2018, Junio C Hamano wrote:
>> Hmph. The other overzealous thing you could do is to strenthen A
>> and "fix" the security issue in v2? Which letter comes before A in
>> the alphabet? ;-)
Yes, agreed. This is what I was hinting at in [1] with "it's a plain
bug".
> Sure, but that being useful is predicated on this supposed security
> mechanism being useful and not just security-through-obscurity, as noted
> in side-threads I don't think we have a convincing argument either way
> (and the one we do have is more on the "it's not secure" side).
>
> Of course we had that with v1 all along, but now that v2 is in released
> versions and in this insecure mode, we have a reason to closely look at
> whether we need to be issuing security releases, or doubling down on the
> "SECURITY" wording in git-fetch and then not carrying the mode forward.
Just for the record, as I've already said, I would be strongly against
removing this feature. I know of multiple populations that make use
of it, and removing it would not serve them well.
Changing defaults and documentation is a separate story.
Sincerely,
Jonathan
[1] https://public-inbox.org/git/20181217231452.GA13835@google.com/
^ permalink raw reply [flat|nested] 73+ messages in thread