Re: [PATCH 0/4] jk/version-string and google code

From: Junio C Hamano <gitster@pobox.com>
To: Jeff King <peff@peff.net>
Cc: "Shawn O. Pearce" <spearce@spearce.org>, git@vger.kernel.org
Subject: Re: [PATCH 0/4] jk/version-string and google code
Date: Fri, 10 Aug 2012 08:34:45 -0700	[thread overview]
Message-ID: <7vfw7ulq0a.fsf@alter.siamese.dyndns.org> (raw)
In-Reply-To: <20120810075342.GA30072@sigill.intra.peff.net> (Jeff King's message of "Fri, 10 Aug 2012 03:53:42 -0400")

Jeff King <peff@peff.net> writes:

> Ugh, the jk/version-string topic breaks fetching from Google Code. With
> my patch, the client unconditionally sends an "agent=foo" capability,
> but the server does not like seeing the unknown capability and ends the
> connection (I'm guessing with some kind of internal exception, since it
> spews "Internal server error" over the protocol channel).

The fix looks sane and the right thing to do.

I've been using 'next' and started seeing this breakage when pushing
to code.google.com only a few days ago.  My reflog tells that
ff5effd (include agent identifier in capability string, 2012-08-03)
was merged to my everyday-work branch on this Monday, which is more
or less consistent with what I am observing.

> This is the right thing to do according to protocol-capabilities.txt,
> which says:
>
>   Client will then send a space separated list of capabilities it wants
>   to be in effect. The client MUST NOT ask for capabilities the server
>   did not say it supports.
>
>   Server MUST diagnose and abort if capabilities it does not understand
>   was sent.  Server MUST NOT ignore capabilities that client requested
>   and server advertised.  As a consequence of these rules, server MUST
>   NOT advertise capabilities it does not understand.
>
> However, that is not how git-core behaves. Its server side will ignore
> an unknown capability coming from the client (so not only is it more
> lenient about what the client does, but it does not follow the "MUST"
> directives in the second paragraph).

Yeah, we probably should fix in the implementation to honor the
"MUST".  When various implementations of clients can start asking
something the server does not support and still yet expect the
request to cause the server do unusual things, it will lead to
chaos, and honoring that "MUST" is a good way to catch and help
diagnosing such a breakage early.

But that is a separate topic and a longer term direction item.

Thanks.