On Wed, May 18, 2022 at 05:12:47PM +0200, Omar Polo wrote:
> >  	space = memchr(buf, ' ', *elen);
> > -	if (space == NULL || space <= buf) {
> > -		err = got_error(GOT_ERR_BAD_OBJ_DATA);
> > -		free(*pte);
> > -		*pte = NULL;
> > -		return err;
> > -	}
> > -	(*pte)->mode = 0;
> 
> this was the same in the previous code too, but why check for space <=
> buf?

Yeah, it surprised me as well.

> > +	if (space == NULL || space <= buf)
> 
> `space' is either NULL or somewhere between [buf, buf+(*elen)), the <=
> is intended just to make sure buf doesn't start with a space right?

Yes, it is just in order to cover == so when the current position
is a space we trigger a parsing error.

The check is not incorrect but could be changed to == for clarity.
I didn't want to change the check as part of this patch though.

> > +static int
> > +pte_cmp(const void *pa, const void *pb)
> > +{
> > +	const struct got_parsed_tree_entry *a, *b;
> > +
> > +	a = (const struct got_parsed_tree_entry *)pa;
> > +	b = (const struct got_parsed_tree_entry *)pb;
> 
> are these cast needed?  It's just a matter of style, I'm just asking to
> be sure so future patches will be coherent with the rest of the code.  I
> have written some code in got_patch IIRC which uses the implicit cast
> from void *

Yes, we can rely on implicit cast from void instead.
Casts should only be needed if the array elements are themselves pointers.

I'll tweak this either before commit, or as a follow-up.