Re: [PATCH 2/2] doc/gitremote-helpers: match object-format option docs to code

[Jeff King <peff@peff.net>]

On Fri, Mar 15, 2024 at 10:41:24AM -0500, Eric W. Biederman wrote:

> I see you saying and a quick grep through the code supports that the
> object-format extension is implemented, and that the primary problem
> is that the Documentation varies slightly from what is implemented.
> 
> 
> Looking at the code I am left with the question:
>  Is the object-format extension properly implemented in all cases?
> 
> 
> If the object-format extension is properly implemented such that a
> client and server mismatch can be detected I am for just Documenting
> what is currently implemented and calling it good.
> 
> The reason for that is
> Documentation/technical/hash-function-transition.txt does not expect
> servers to support more than hash function.  I don't have a perspective
> that differs.  So detecting what the client and server support and
> failing if they differ should be good enough.

AFAIK the code all works correctly, and there are no cases where we fail
to notice a mismatch. The two code/doc inconsistencies (and bearing in
mind this is for the transport-helper protocol, not the v2 protocol
itself) are:

  - the docs say "object-format true", but the code just says
    "object-format". They're semantically equivalent, so it's just a
    minor syntax issue.

  - the docs say that Git may write "object-format sha256" to the
    helper, but the code will never do that.

So my big question is for the second case: is that something that we'll
need to be able to do (possibly to support interop, but possibly for
some other case)? If not, we should probably just fix the docs. If so,
then we need to either fix the code, or accept that we'll need to add a
new capability/extension later.

> I am concerned that the current code may not report it's hash function
> in all of the cases it needs to, to be able to detect a mismatch.
> 
> I look at commit 8b85ee4f47aa ("transport-helper: implement
> object-format extensions") and I don't see anything that generates
> ":object-format=" after it has been asked for except the code
> in remote-curl.c added in commit 7f60501775b2 ("remote-curl: implement
> object-format extensions").
> 
> Maybe I am mistaken but a name like remote-curl has me strongly
> suspecting that it does not cover all of the cases that git supports
> that implement protocol v2.

That all sounds right. We are talking just about the transport-helper
protocol here, where Git speaks to a separate program that actually
contacts the remote server. And the main helper we ship is remote-curl
(which handles https, http, etc). Everything else is linked directly and
does not need to use a separate process (we use a separate process to
avoid linking curl, openssl, etc into the main Git binary).

We do ship remote-fd and remote-ext, but they don't support most options
(and probably don't need to, because they're mostly pass-throughs that
just use the "connect" feature).

The other major helpers people tend to use are adapters to other version
control systems (e.g., remote-hg, cinnabar). We don't ship any of those
ourselves. They'll obviously need to learn about the transport-helper
object-format capability before they're ready to handle sha256 repos,
but I suspect that works has not really started.

> I think I see some omissions in updating the protocol v2 Documentation.

If you mean from the commits listed above, I don't think so; they are
just touching the transport-helper protocol, not the v2 wire protocol.

-Peff