Jeff King <peff@xxxxxxxx> 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 <peff@xxxxxxxx>
> ---
> 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;
