Re: [PATCH 2/3] receive-pack: verify push options in cert

[Jonathan Nieder <jrnieder@gmail.com>]

Hi,

Stefan Beller wrote:
> On Fri, May 5, 2017 at 4:46 PM, Jonathan Tan <jonathantanmy@google.com> wrote:

>> This sets in stone the requirement that send-pack redundantly send its
>> push options in 2 places, but I think that this is better than the
>> alternatives. Sending push options only within the cert is
>> backwards-incompatible with existing Git servers (which read push
>> options only from outside the cert), and sending push options only
>> outside the cert means that the push options are not signed for.
>
> As the combination of push certs and push options are broken
> (at least when using JGit as well, which are used in Gerrit
> installations), I would not feel too bad about actually going
> the backwards-incompatible way.

Per offline discussion, what you're proposing is to omit the second
copy of push options from the client request, so the server does not
have to see two copies.  I agree that that would be a better protocol,
but I think that ship has sailed.

Current versions of git the client and git the server are able to
interoperate without trouble (though the server-side bug is ugly in
terms of what the push certs mean).

Current versions of JGit the server *require* that the push options be
omitted from the push certificate.  I don't think there exists a
sensible way to interoperate with that, so we can ignore that JGit
server behavior as a bug (like you've said).

If we want to omit the second copy of the push certs (which sounds
like a reasonable thing to want), that would require a new capability
to be added to the protocol to do so.  That way, interoperability with
existing versions of git the client wouldn't be broken.  That could
happen on top of this series --- it is not needed for fixing the bug
that this series fixes.

To summarize:

 1. I agree that we shouldn't feel bad about breaking compatibility
    with the JGit server behavior at issue, since there is no
    reasonable way to be compatible with it.  And that's what this
    series does --- it breaks compatibility with the broken versions
    of JGit server and picks what the Git client has been doing
    instead.

 2. I don't think we should break new versions of Git's ability to
    interoperate with current versions of the server, even though I
    consider the current server behavior to be broken.

 3. If someone is interested in getting rid of the redundant second
    copy of push options, we have a way to do so, by introducing a
    new capability

Thanks,
Jonathan