Re: [PATCH] use child_process_init() to initialize struct child_process variables

From: Jeff King <peff@peff.net>
Date: Nov 4, 2014, 23:32 UTC
Message-ID: <20141104233215.GA16091@peff.net>
In-Reply-To: <xmqq4muepr40.fsf@gitster.dls.corp.google.com>

On Tue, Nov 04, 2014 at 01:56:15PM -0800, Junio C Hamano wrote:

Show 14 quoted lines

> >>   2. Including two lines, like:
> >>
> >>         $sha1 HEAD\0symref=refs/heads/master
> >>         $sha1 HEAD
> >>
> >>      which JGit does the right thing with (and git.git seems to, as
> >>      well).
> >
> > Sounds sensible, even though it looks ugly X-<.
> 
> I have a mild preference for a syntax that is more similar to the
> on-wire protocol, so that connect.c::parse_feature_value() can be
> reused to parse it and possibly annotate_refs_with_symref_info() can
> also be reused by calling it from transport.c::get_refs_from_bundle().

Yeah, what I wrote above was the simplest thing that could work, and does not need to be the final form. I know that you already know what I'm about to describe below, Junio, but I want to expand on the situation for the benefit of onlookers (and potential implementers like Philip).

The online protocol is hampered by the "if you see something after a NUL, it is a capabilities string, and you must throw out the previous capabilities string and replace it with this one" historical rule. And that's why we cannot do:

  $sha1 refs/heads/master\0thin-pack side-band etc
  $sha1 HEAD\0symref=refs/heads/master

as it would throw out "thin-pack", "side-band", etc. Instead we do it more like:

  $sha1 refs/heads/master\0thin-pack side-band etc symref=HEAD:refs/heads/master
  $sha1 HEAD

to shove _all_ of the symref mappings into the capability string, rather than letting them ride along with their respective refs. The downside is that we are bounded in the number of symref mappings we can send (by the maximum length for a single pkt-line), and therefore send only the value of HEAD.

The bundle code is not bound by this historical legacy, and could do it in a different (and more efficient and flexible) way. But it is probably saner to just keep them identical. It makes the code simpler, and having bundle as the only transport which has the extra flexibility does not really buy us much (and probably just invites confusion).

-Peff