From: Jeff King <peff@peff.net>
To: "brian m. carlson" <sandals@crustytoothpaste.net>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
Adam Majer <adamm@zombino.com>
Subject: Re: [PATCH 1/2] http: advertise capabilities when cloning empty repos
Date: Thu, 27 Apr 2023 01:30:16 -0400 [thread overview]
Message-ID: <20230427053016.GD982277@coredump.intra.peff.net> (raw)
In-Reply-To: <20230426205324.326501-2-sandals@crustytoothpaste.net>
On Wed, Apr 26, 2023 at 08:53:23PM +0000, brian m. carlson wrote:
> From: "brian m. carlson" <bk2204@github.com>
>
> When cloning an empty repository, the HTTP protocol version 0 currently
> offers nothing but the header and flush packets for the /info/refs
> endpoint. This means that no capabilities are provided, so the client
> side doesn't know what capabilities are present.
Is this really an HTTP problem?
If I do:
git init --bare --object-format=sha256 remote.git
git -c protocol.version=0 clone --bare remote.git local.git
git -C local.git rev-parse --show-object-format
I will get sha1, which is wrong. Likewise with GIT_DEFAULT_HASH=sha256
on the clone (after Junio's recent patch), regardless of what the server
claims. This is really a git-protocol issue that affects all transports.
So I think in this hunk:
> @@ -1379,6 +1381,8 @@ void upload_pack(const int advertise_refs, const int stateless_rpc,
> data.no_done = 1;
> head_ref_namespaced(send_ref, &data);
> for_each_namespaced_ref(send_ref, &data);
> + if (!data.sent_capabilities && advertise_refs)
> + send_ref("capabilities^{}", null_oid(), 0, &data);
> /*
> * fflush stdout before calling advertise_shallow_grafts because send_ref
> * uses stdio.
you would want to drop the "&& advertise_refs" bit, after which both of
the cases above would yield a sha256 repository.
There is one other catch, though. Doing as I suggest results in a
failure in t5509, because the new code does not interact correctly with
namespaces. That is true of your version, as well; it's just that the
test suite does not cover the combination of namespaces, http, and empty
repos.
The issue is that send_ref() will try to strip the namespace, and end up
with NULL (which on my glibc system ends up with a ref named "(null)",
but obviously could segfault, too).
Something like this fixes it:
diff --git a/environment.c b/environment.c
index 8a96997539..37cd66b295 100644
--- a/environment.c
+++ b/environment.c
@@ -234,6 +234,8 @@ const char *get_git_namespace(void)
const char *strip_namespace(const char *namespaced_ref)
{
const char *out;
+ if (!strcmp(namespaced_ref, "capabilities^{}"))
+ return namespaced_ref; /* magic ref */
if (skip_prefix(namespaced_ref, get_git_namespace(), &out))
return out;
return NULL;
but I suspect it would be cleaner to refactor send_ref() to allow
sending a name more directly.
(As an aside, it feels like send_ref() is also wrong not to check for
NULL from strip_namespace(), but I guess in practice we do not feed
it names outside of the namespace. Might be a good candidate for a BUG()
check or other assertion).
> +test_expect_success 'clone empty SHA-256 repository with protocol v0' '
> + rm -fr sha256 &&
> + echo sha256 >expected &&
> + GIT_TRACE=1 GIT_TRACE_PACKET=1 git -c protocol.version=0 clone "$HTTPD_URL/smart/sha256.git" &&
> + git -C sha256 rev-parse --show-object-format >actual &&
> + test_cmp actual expected &&
> + git ls-remote "$HTTPD_URL/smart/sha256.git" >actual &&
> + test_must_be_empty actual
> +'
This looks reasonable, though I think if we do not need HTTP to
demonstrate the issue (and I don't think we do), then we should probably
avoid it, just to get test coverage on platforms that don't support
HTTP.
-Peff
next prev parent reply other threads:[~2023-04-27 5:30 UTC|newest]
Thread overview: 58+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-04-05 10:28 git clone of empty repositories doesn't preserve hash Adam Majer
2023-04-05 19:04 ` Junio C Hamano
2023-04-05 19:47 ` Adam Majer
2023-04-05 20:01 ` Jeff King
2023-04-05 20:40 ` Junio C Hamano
2023-04-05 21:15 ` Junio C Hamano
2023-04-05 21:26 ` Jeff King
2023-04-05 22:48 ` brian m. carlson
2023-04-06 13:11 ` Adam Majer
2023-04-25 21:35 ` brian m. carlson
2023-04-25 22:24 ` Junio C Hamano
2023-04-25 23:12 ` Junio C Hamano
2023-04-26 0:20 ` brian m. carlson
2023-04-26 11:25 ` Jeff King
2023-04-26 15:08 ` Junio C Hamano
2023-04-26 15:13 ` [PATCH] doc: GIT_DEFAULT_HASH is and will be ignored during "clone" Junio C Hamano
2023-04-26 21:06 ` brian m. carlson
2023-04-27 4:46 ` git clone of empty repositories doesn't preserve hash Jeff King
2023-04-26 10:51 ` Jeff King
2023-04-26 15:42 ` Junio C Hamano
2023-04-26 20:40 ` brian m. carlson
2023-04-26 20:53 ` [PATCH 0/2] Fix empty SHA-256 clones with v0 and v1 brian m. carlson
2023-04-26 20:53 ` [PATCH 1/2] http: advertise capabilities when cloning empty repos brian m. carlson
2023-04-26 21:14 ` Junio C Hamano
2023-04-26 21:28 ` brian m. carlson
2023-04-27 5:00 ` Jeff King
2023-04-27 5:30 ` Jeff King [this message]
2023-04-27 20:40 ` Junio C Hamano
2023-04-26 20:53 ` [PATCH 2/2] Honor GIT_DEFAULT_HASH for empty clones without remote algo brian m. carlson
2023-04-26 21:18 ` Junio C Hamano
2023-04-26 21:33 ` Junio C Hamano
2023-04-27 5:43 ` Jeff King
2023-05-02 23:46 ` Is GIT_DEFAULT_HASH flawed? Felipe Contreras
2023-05-03 9:03 ` Adam Majer
2023-05-03 15:44 ` Felipe Contreras
2023-05-03 17:21 ` Adam Majer
2023-05-08 0:34 ` Felipe Contreras
2023-05-03 9:09 ` demerphq
2023-05-03 18:20 ` Felipe Contreras
2023-05-03 22:54 ` brian m. carlson
2023-05-08 2:00 ` Felipe Contreras
2023-05-08 21:38 ` brian m. carlson
2023-05-09 10:32 ` Oswald Buddenhagen
2023-05-09 16:47 ` Junio C Hamano
2023-04-26 21:12 ` [PATCH 0/2] Fix empty SHA-256 clones with v0 and v1 Junio C Hamano
2023-04-27 4:56 ` git clone of empty repositories doesn't preserve hash Jeff King
2023-05-01 17:00 ` [PATCH v2 0/1] Fix empty SHA-256 clones with v0 and v1 brian m. carlson
2023-05-01 17:00 ` [PATCH v2 1/1] upload-pack: advertise capabilities when cloning empty repos brian m. carlson
2023-05-01 22:40 ` Jeff King
2023-05-01 22:51 ` Junio C Hamano
2023-05-01 17:37 ` [PATCH v2 0/1] Fix empty SHA-256 clones with v0 and v1 Junio C Hamano
2023-05-17 19:24 ` [PATCH v3 " brian m. carlson
2023-05-17 19:24 ` [PATCH v3 1/1] upload-pack: advertise capabilities when cloning empty repos brian m. carlson
2023-05-17 21:48 ` [PATCH v3 0/1] Fix empty SHA-256 clones with v0 and v1 Junio C Hamano
2023-05-17 22:28 ` brian m. carlson
2023-05-18 18:28 ` Jeff King
2023-05-19 15:32 ` brian m. carlson
2023-04-05 21:23 ` git clone of empty repositories doesn't preserve hash Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230427053016.GD982277@coredump.intra.peff.net \
--to=peff@peff.net \
--cc=adamm@zombino.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=sandals@crustytoothpaste.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).