From: Jeff King <peff@peff.net>
To: "Eric W. Biederman" <ebiederm@gmail.com>
Cc: git@vger.kernel.org, "brian m. carlson" <sandals@crustytoothpaste.net>
Subject: Re: [PATCH 0/3] some transport-helper "option object-format" confusion
Date: Wed, 27 Mar 2024 05:48:40 -0400 [thread overview]
Message-ID: <20240327094840.GA857435@coredump.intra.peff.net> (raw)
In-Reply-To: <87y1ac3kb6.fsf@gmail.froward.int.ebiederm.org>
On Wed, Mar 20, 2024 at 12:05:49PM -0500, Eric W. Biederman wrote:
> Your sentence has what I was asking for backwards. It would be healthy
> if the code fails when "object-format" has been advertised by the
> remote, requested by the transport-helper, and the remote does not send
> ":object-format".
Ah, I see. That is probably reasonable, under the assumption that nobody
would have implemented "object-format" so far and _not_ sent it. It
might be worth clarifying the documentation at the same time.
> The implementation should just be:
>
> diff --git a/transport-helper.c b/transport-helper.c
> index b660b7942f9f..e648f136287d 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -1206,6 +1206,7 @@ static struct ref *get_refs_list_using_list(struct transport *transport,
> struct ref **tail = &ret;
> struct ref *posn;
> struct strbuf buf = STRBUF_INIT;
> + bool received_object_format = false;
>
> data->get_refs_list_called = 1;
> helper = get_helper(transport);
> @@ -1236,9 +1236,13 @@ static struct ref *get_refs_list_using_list(struct transport *transport,
> die(_("unsupported object format '%s'"),
> value);
> transport->hash_algo = &hash_algos[algo];
> + received_object_format = true;
> }
> continue;
> }
> + else if (data->object_format && !received_object_format) {
> + die(_("missing :object-format"));
> + }
>
> eov = strchr(buf.buf, ' ');
> if (!eov)
>
> Am I missing something that makes a bad implementation?
No, that seems right to me (modulo that we do not use C99 "bool" in our
code base).
> Hmm. I thought gitremote-helpers.txt said the key value pairs
> would precede everything else from a list command.
> gitremote-helpers.txt does not mention that. That looks like
> a Documentation oversight.
>
> However remote-curl.c in output_refs prints :object-format before
> anything else, and transport-helper.c will malfunction if :object-format
> is sent after any of the refs. As transport->hash_algop is used by
> get_oid_hex_algop is used to parse the oids of the refs.
Yeah, I think it is a natural consequence of "object-format", since it
is necessary for parsing the result. And since there aren't any other
keywords yet, we can surmise that nobody is doing the wrong thing yet.
So now is a good time to clarify the documentation.
I'm also not sure if we ever say explicitly in the documentation that
the keywords start with a colon. But maybe I am just missing it.
> diff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt
> index ed8da428c98b..b6ca29a245f3 100644
> --- a/Documentation/gitremote-helpers.txt
> +++ b/Documentation/gitremote-helpers.txt
> @@ -268,6 +268,8 @@ Support for this command is mandatory.
> ref. A space-separated list of attributes follows the name;
> unrecognized attributes are ignored. The list ends with a
> blank line.
> +
> + Keywords should precede everything else in the list.
> +
> See REF LIST ATTRIBUTES for a list of currently defined attributes.
> See REF LIST KEYWORDS for a list of currently defined keywords.
>
> I do agree that the sanity check can be added to your series, so if you
> would prefer I can do that.
Yeah, do you want to send some patches that can go on top of mine?
-Peff
prev parent reply other threads:[~2024-03-27 9:48 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-07 8:47 [RFC/PATCH 0/2] some transport-helper "option object-format" confusion Jeff King
2024-03-07 8:51 ` [PATCH 1/2] t5801: fix object-format handling in git-remote-testgit Jeff King
2024-03-07 8:56 ` [PATCH 2/2] doc/gitremote-helpers: match object-format option docs to code Jeff King
2024-03-07 22:20 ` brian m. carlson
2024-03-12 7:45 ` Jeff King
2024-03-13 21:11 ` brian m. carlson
2024-03-14 12:47 ` Eric W. Biederman
2024-03-14 21:21 ` brian m. carlson
2024-03-15 15:41 ` Eric W. Biederman
2024-03-16 6:04 ` Jeff King
2024-03-17 20:47 ` Eric W. Biederman
2024-03-18 8:49 ` Jeff King
2024-03-14 15:33 ` Junio C Hamano
2024-03-14 21:54 ` brian m. carlson
2024-03-20 9:32 ` [PATCH 0/3] some transport-helper "option object-format" confusion Jeff King
2024-03-20 9:34 ` [PATCH 1/3] transport-helper: use write helpers more consistently Jeff King
2024-03-20 9:37 ` [PATCH 2/3] transport-helper: drop "object-format <algo>" option Jeff King
2024-03-20 9:41 ` [PATCH 3/3] transport-helper: send "true" value for object-format option Jeff King
2024-03-20 17:23 ` Junio C Hamano
2024-03-20 17:05 ` [PATCH 0/3] some transport-helper "option object-format" confusion Eric W. Biederman
2024-03-27 9:48 ` Jeff King [this message]
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
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240327094840.GA857435@coredump.intra.peff.net \
--to=peff@peff.net \
--cc=ebiederm@gmail.com \
--cc=git@vger.kernel.org \
--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.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
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).