Web lists-archives.com

Re: [PATCH v2 01/24] walker: convert to struct object_id




Hi,

brian m. carlson wrote:

> Signed-off-by: brian m. carlson <sandals@xxxxxxxxxxxxxxxxxxxx>
> ---
>  walker.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)

An egrep for 'sha1|unsigned char' finds nothing left in the file after
this change.

[...]
> --- a/walker.c
> +++ b/walker.c
> @@ -7,7 +7,7 @@
>  #include "blob.h"
>  #include "refs.h"
>  
> -static unsigned char current_commit_sha1[20];
> +static struct object_id current_commit_oid;

Yay!

nit, not suggesting changing anything: I would have been tempted to
call this current_commit_id, or even just current_commmit.

[...]
> @@ -259,7 +259,7 @@ int walker_fetch(struct walker *walker, int targets, char **target,
>  	struct strbuf refname = STRBUF_INIT;
>  	struct strbuf err = STRBUF_INIT;
>  	struct ref_transaction *transaction = NULL;
> -	unsigned char *sha1 = xmalloc(targets * 20);
> +	struct object_id *oids = xmalloc(targets * sizeof(struct object_id));

Not new in this patch, just noticing while we're here: can this
multiplication overflow?

E.g. in remote-curl, it looks like nr_heads gets incremented once per
"fetch" line passed to the remote helper, making it unbounded.

This could use st_mult or ALLOC_ARRAY to protect against that.  The
caller remote-curl.c::parse_fetch also needs an overflow check to
avoid overflowing its "int".  Likewise, walker_targets_stdin needs an
overflow check to avoid overflowing its "int".

[...]
> @@ -321,7 +321,7 @@ int walker_fetch(struct walker *walker, int targets, char **target,
>  done:
>  	ref_transaction_free(transaction);
>  	free(msg);
> -	free(sha1);
> +	free(oids);

Reviewed-by: Jonathan Nieder <jrnieder@xxxxxxxxx>