Re: [PATCH 0/3] some transport-helper "option object-format" confusion

From: Eric W. Biederman <ebiederm@gmail.com>
Date: Mar 20, 2024, 17:05 UTC
Message-ID: <87y1ac3kb6.fsf@gmail.froward.int.ebiederm.org>
In-Reply-To: <20240320093226.GA2445531@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 36 quoted lines

> 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