Re: [PATCH 04/14] connect: rewrite feature parsing to work on string_list

[David Turner <dturner@twopensource.com>]

On Wed, 2016-05-04 at 13:13 -0700, Junio C Hamano wrote:
> Stefan Beller <sbeller@google.com> 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 <sbeller@google.com>
> > ---
> >  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.