Re: [RFC/WIP PATCH 04/11] upload-pack-2: Implement the version 2 of upload-pack

From: Jeff King <peff@peff.net>
Date: May 27, 2015, 20:14 UTC
Message-ID: <20150527201433.GB14309@peff.net>
In-Reply-To: <CAPig+cSFEN+V0668FPDM1jY2KdW_nVaEn7+AOWJj_KwUU_UVPw@mail.gmail.com>

On Wed, May 27, 2015 at 01:30:28PM -0400, Eric Sunshine wrote:

Show 15 quoted lines

> > Like Eric, I find the whole next_capability thing a little ugly. His
> > suggestion to pass in the parsing state is an improvement, but I wonder
> > why we need to parse at all. Can we keep the capabilities as:
> >
> >   const char *capabilities[] = {
> >         "multi_ack",
> >         "thin-pack",
> >         ... etc ...
> >   };
> >
> > and then loop over the array?
> 
> Yes, that would be much nicer. I also had this in mind but didn't know
> if there was a strong reason for the capabilities to be shoehorned
> into a single string as they are currently.

I don't think there is a good reason, beyond it being the simplest thing for the current code to work. But as you can see from the existing packet_write() in upload-pack, it's already going through some contortions to handle optional capabilities (i.e., "capabilities" is by no means the full list anymore).

Doing it item by item will mean we can't use a single packet_write() in the v1 code, but it's OK to format into a buffer here (we already need such a buffer for format_symref_info anyway).

-Peff