Re: [PATCH 05/12] http: simplify parsing of remote objects/info/packs

From: Jeff King <peff@peff.net>
To: "René Scharfe" <l.s.r@web.de>
Cc: git@vger.kernel.org
Subject: Re: [PATCH 05/12] http: simplify parsing of remote objects/info/packs
Date: Fri, 5 Apr 2019 14:11:32 -0400	[thread overview]
Message-ID: <20190405181132.GA32401@sigill.intra.peff.net> (raw)
In-Reply-To: <83129937-dcd0-f16e-c8aa-97eceec9769a@web.de>

On Fri, Apr 05, 2019 at 12:41:27PM +0200, René Scharfe wrote:

> Am 05.04.2019 um 01:27 schrieb Jeff King:
> > We can use skip_prefix() and parse_oid_hex() to continuously increment
> > our pointer, rather than dealing with magic numbers. This also fixes a
> > few small shortcomings:
> >
> >   - if we see a 'P' line that does not match our expectations, we'll
> >     leave our "i" counter in the middle of the line. So we'll parse:
> >     "P P P pack-1234.pack" as if there was just one "P" which was not
> >     intentional (though probably not that harmful).
> 
> How so?  The default case, which we'd fall through to, skips the rest
> of such a line, doesn't it?

Oh, you're right. I didn't notice the fall-through, which is quite
subtle (I'm actually surprised -Wimplicit-fallthrough doesn't complain
about this).

I'll fix up the commit message (the cleanup is still very much worth it
for the garbage-oid issue, IMHO).

> > +	while (*data) {
> > +		if (skip_prefix(data, "P pack-", &data) &&
> > +		    !parse_oid_hex(data, &oid, &data) &&
> > +		    skip_prefix(data, ".pack", &data) &&
> > +		    (*data == '\n' || *data == '\0')) {
> > +			fetch_and_setup_pack_index(packs_head, oid.hash, base_url);
> > +		} else {
> > +			data = strchrnul(data, '\n');
> >  		}
> > -		i++;
> > +		if (*data)
> > +			data++; /* skip past newline */
> 
> So much simpler, *and* converted to object_id -- I like it!
> 
> Parsing "P" and "pack-" together crosses logical token boundaries,
> but that I don't mind it here.

Yeah, I was tempted to write:

  if (skip_prefix(data, "P ", &data) &&
      skip_prefix(data, "pack-", &data) &&
      ...

but that felt a little silly. I dunno. I guess it is probably not any
less efficient, because we'd expect skip_prefix() and its loop to get
inlined here anyway.

-Peff