Jeff King <[email protected]> writes: > When we parse an on-disk untracked cache, we have two pointers, "data" > and "next". As we parse, we point "next" to the end of an element, and > then later update "data" to match. > > But we actually don't need two pointers. Each parsing step can just > update "data" directly from other variables we hold (and we don't have > to worry about bailing in an intermediate state, since any parsing > failure causes us to immediately discard "data" and return).
;-) My first reaction was "you can do so now you have introduced eos--why didn't you do that in the previous step?", but losing 'next' from the varint parsing step would certainly have been possible even before that change. So I agree that it makes much more sense to do this step separately from the previous one. The code after the patch certainly reads easier and simpler. Thanks. > Signed-off-by: Jeff King <[email protected]> > --- > dir.c | 20 +++++++------------- > 1 file changed, 7 insertions(+), 13 deletions(-) > > diff --git a/dir.c b/dir.c > index 7b0513c476..17865f44df 100644 > --- a/dir.c > +++ b/dir.c > @@ -2732,50 +2732,44 @@ static int read_one_dir(struct untracked_cache_dir > **untracked_, > struct read_data *rd) > { > struct untracked_cache_dir ud, *untracked; > - const unsigned char *next, *data = rd->data, *end = rd->end; > + const unsigned char *data = rd->data, *end = rd->end; > const unsigned char *eos; > unsigned int value; > int i, len; > > memset(&ud, 0, sizeof(ud)); > > - next = data; > - value = decode_varint(&next); > - if (next > end) > + value = decode_varint(&data); > + if (data > end) > return -1; > ud.recurse = 1; > ud.untracked_alloc = value; > ud.untracked_nr = value; > if (ud.untracked_nr) > ALLOC_ARRAY(ud.untracked, ud.untracked_nr); > - data = next; > > - next = data; > - ud.dirs_alloc = ud.dirs_nr = decode_varint(&next); > - if (next > end) > + ud.dirs_alloc = ud.dirs_nr = decode_varint(&data); > + if (data > end) > return -1; > ALLOC_ARRAY(ud.dirs, ud.dirs_nr); > - data = next; > > eos = memchr(data, '\0', end - data); > if (!eos || eos == end) > return -1; > len = eos - data; > - next = eos + 1; > > *untracked_ = untracked = xmalloc(st_add3(sizeof(*untracked), len, 1)); > memcpy(untracked, &ud, sizeof(ud)); > memcpy(untracked->name, data, len + 1); > - data = next; > + data = eos + 1; > > for (i = 0; i < untracked->untracked_nr; i++) { > eos = memchr(data, '\0', end - data); > if (!eos || eos == end) > return -1; > len = eos - data; > - next = eos + 1; > untracked->untracked[i] = xmemdupz(data, len); > - data = next; > + data = eos + 1; > } > > rd->ucd[rd->index++] = untracked;
