| Message ID | 20240320093226.GA2445531@coredump.intra.peff.net (mailing list archive) |
|---|---|
| Headers | show |
| Series | some transport-helper "option object-format" confusion | expand |
Jeff King <peff@peff.net> writes: > On Thu, Mar 07, 2024 at 03:47:35AM -0500, Jeff King wrote: > >> I happened to be looking at the output of t5801 for an unrelated >> problem, and I noticed our git-remote-testgit spewing a bunch of shell >> errors. It turns out that its expectations do not quite match what the >> transport-helper code produces. >> >> This series brings the test and documentation in line with how the >> transport-helper code behaves. But I'm not sure if we should be going >> the other way (see the comments on patch 2 especially), and bringing the >> transport-helper code in line with the others. Hence the RFC. >> >> [1/2]: t5801: fix object-format handling in git-remote-testgit >> [2/2]: doc/gitremote-helpers: match object-format option docs to code > > Here's a non-RFC v2 based on the discussion thus far (thanks brian and > Eric). > > The big change is that instead of changing the docs to match true-less > "option object-format", the code is changed to match the docs. That > happens in patch 3 (which subsumes the original patch 1). We continue to > drop the documentation for the "option object-format sha256" form. But > now the commit message justifies it better, and we clean up the stale > code in remote-curl.c. > > Patch 1 is a small fix for debugging output that I noticed after getting > confused. :-/ It's not strictly related and could be taken separately. > > Eric mentioned having Git check that the helpers never say > ":object-format" unless it was negotiated. I stopped short of that. One, > it's a bit tricky to test (since Git will always ask for object-format, > you have to teach remote-testgit to optionally send broken output). And > two, I'm not sure that being strict has much value here. It keeps remote > helpers honest, but the real losers are old versions that do not > understand :object-format, which would fail against such a remote. So I > dunno. It isn't any harder to do it on top later if we want to. 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". The check is cheap and should prevent buggy remotes from appearing in the wild. I am probably biased but I rather want the information on what hash algorithm the remote is using when I ask for it. I can totally imagine someone during development forgetting to send :object-format and either not noticing something was wrong, or spending a fair amount of time debugging that they forgot to send it. It is the kind of bug I can imagine someone making when they are called away from the keyboard at the wrong moment. 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? 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. We can probably fix the Documentation like: 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. Eric
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