[2/3] untracked-cache: simplify parsing by dropping "next"

Commit Message

Jeff King April 18, 2019, 9:17 p.m. UTC

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).

Signed-off-by: Jeff King <[e-mail]>
---
 dir.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

Comments

Junio C Hamano April 19, 2019, 5:33 a.m. UTC | #1

Jeff King <[e-mail]> 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 <[e-mail]>
> ---
>  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;

Patch

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;