Re: [PATCH 2/2] connect: know that zero-ID is not a ref

From: Shawn Pearce <spearce@spearce.org>
Date: Sep 2, 2016, 19:39 UTC
Message-ID: <CAJo=hJssMohNq5zDU=wdb1gP4nW59m8pUmxP7eY4jnjr502z0w@mail.gmail.com>
In-Reply-To: <2bea354c6218a33db3972e42baa75676fdcbc598.1472836026.git.jonathantanmy@google.com>

On Fri, Sep 2, 2016 at 10:15 AM, Jonathan Tan <jonathantanmy@google.com> wrote:

>
> +               if (is_null_oid(&old_oid)) {
> +                       if (strcmp(name, "capabilities^{}"))

Its not the zero ID that is special, its the "capabilities^{}" name that is special when its the first entry in the stream. In the wire protocol a "x^{}" line is a modifier to a prior "x" line to add a peeled object to the prior line. But if we see "^{}" on the first line that is non-sense, there is no prior line to modify with this identifier.

Further ^{} is used here because its invalid in a name. A server really cannot have a reference that ends with the sequence ^{}. And a server should not have a reference named "capabilities" without a "refs/" prefix on it.

So the entire "capabilities^{}" on the first line is a bunch of contradictions that violate a number of things about the protocol, which is why clients should ignore it.

I think the test should be about:

  !*list && !strcmp(name, "capabilities^{}")

> +                               warning("zero object ID received that is not accompanied by a "
> +                                       "capability declaration, ignoring and continuing anyway");

Annoyingly a zero object ID is sort of possible; with a probability of 1/2^160 or something. Its just a very very unlikely value. Slightly stronger to test against the known invalid name.