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

From: Philip Oakley <philipoakley@iee.org>
Date: Nov 5, 2014, 12:44 UTC
Message-ID: <95BF305A6E8D485F9F63503ACE7FC2AB@PhilipOakley>
In-Reply-To: <20141104233215.GA16091@peff.net>

From: "Jeff King" <peff@peff.net>
Subject: Re: [PATCH] use child_process_init() to initialize struct 
child_process variables

Show 25 quoted lines

> On Tue, Nov 04, 2014 at 01:56:15PM -0800, Junio C Hamano wrote:
>
>> >>   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).

I think I'm keeping up ;-)

Show 37 quoted lines

>
> 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
>

Obviously bundles are always off-line, so it's reasonable to be cautious about using an on-line sideband method, though the re-use of a standard format is good.

Finding the right parsing method will be important, as well as ensuring there are no races from the update of unsorted refs.

Philip