From: David Turner Date: Tue, 17 May 2016 22:23:49 GMT Subject: Re: [PATCH 04/14] connect: rewrite feature parsing to work on string_list Message-ID: <1463523829.24478.78.camel@twopensource.com> In-Reply-To: On Wed, 2016-05-04 at 13:13 -0700, Junio C Hamano wrote: > Stefan Beller writes: > > > Later on when we introduce the version 2 transport protocol, the > > capabilities will not be transported in one lone string but each > > s/lone/long/, I think. > > > capability will be carried in its own pkt line. > > > > To reuse existing infrastructure we would either need to join the > > capabilities into a single string again later or refactor the > > current > > capability parsing to be using a data structure which fits both > > versions of the transport protocol. We chose to implement the > > later. > > > > Signed-off-by: Stefan Beller > > --- > > builtin/receive-pack.c | 15 ++++++--- > > connect.c | 82 +++++++++++++++++++++++--------------- > > ------------ > > connect.h | 2 +- > > upload-pack.c | 13 ++++++-- > > 4 files changed, 58 insertions(+), 54 deletions(-) > > I am not sure if the churn is make a right tradeoff here. > > A loop to concatenate each segment into a strbuf before passing it > to parse_feature_request would be at most 5 lines long, no? I started looking at this again briefly today, and I actually think that string manipulation of this sort is a mistake. String manipulation is error-prone, and having an interface defined in terms of strings makes it hard for a reader to see what sorts of input are valid and invalid. (It's also often somewhat less efficient). So I think it is a good idea to improve the interface while we're already changing code in this area.