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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, 2016-05-04 at 13:13 -0700, Junio C Hamano wrote:
> Stefan Beller <sbeller@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> >  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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]