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
