Re: [PATCH 0/3] some transport-helper "option object-format" confusion
- From
Jeff King <peff@peff.net>
- Date
- Mar 27, 2024, 09:48 UTC
- Message-ID
- <20240327094840.GA857435@coredump.intra.peff.net>
- 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.
Show 30 quoted lines
> 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).
Show 9 quoted lines
> 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.
Show 16 quoted lines
> 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