From: Mark Jamsek <mark@jamsek.com>
Subject: Re: simplify send.c for target refnames
To: Omar Polo <op@omarpolo.com>
Cc: Josh Rickmar <jrick@zettaport.com>, gameoftrees@openbsd.org
Date: Fri, 07 Jul 2023 12:43:59 +1000

Omar Polo <op@omarpolo.com> wrote:
> On 2023/07/05 10:46:38 -0400, Josh Rickmar <jrick@zettaport.com> wrote:
> > On Wed, Jul 05, 2023 at 10:40:43AM -0400, Josh Rickmar wrote:
> > > On Wed, Jul 05, 2023 at 08:38:09AM -0400, Josh Rickmar wrote:
> > > > got_send_pack in send.c stuffed into a single reflist only to
> > > > 
> > > > 1) resolve these references multiple times over
> > > > 2) convert the damn thing to a pathlist in the end because the reflist isn't
> > > >    linked into the privsep handlers.
> > > > 
> > > > Building the pathlist directly seems like the saner approach.  This also
> > > > moves the validation that the reference is 'sendable' (by got policy rules)
> > > > to insert_ref.
> > > 
> > > Updated with feedback.  Also changed their_ids to borrow the object
> > > ids from their_refs so the cleanup is the same for both arrays.
> > 
> > tb@ found a leak of the asprintf/strdup if insert_sendable_ref fails.
> 
> sorry for the delay, i've managed to read this only now.  i think
> there's a leak in insert_sendable_ref(), we should close the ref
> regardless of any error occurred since we open it in that function and
> we don't move it around.
> 
> while here also tweak a comment that was moved.

Good catch! I don't know how I missed it. I read that change a few
times, too, because the early return before the done label made me
look askance and I still missed it. Thanks for the save :)

ok

> diff /home/op/w/got
> commit - 96fc93d02a405c26b746df83e7f892bc8546109d
> path + /home/op/w/got
> blob - bfb0e6f7f0b590d525fca551e9d76d8d7d6abd04
> file + lib/send.c
> --- lib/send.c
> +++ lib/send.c
> @@ -180,11 +180,10 @@ done:
>  
>  	err = got_pathlist_insert(NULL, refs, refname, id);
>  done:
> -	if (err != NULL) {
> -		if (ref)
> -			got_ref_close(ref);
> +	if (ref)
> +		got_ref_close(ref);
> +	if (err)
>  		free(id);
> -	}
>  	return err;
>  }
>  
> @@ -441,7 +440,7 @@ got_send_pack(const char *remote_name, struct got_path
>  	}
>  
>  	/*
> -	 * Also prepare the array of our object IDs which
> +	 * Prepare the array of our object IDs which
>  	 * will be needed for generating a pack file.
>  	 */
>  	TAILQ_FOREACH(pe, &have_refs, entry) {


-- 
Mark Jamsek <https://bsdbox.org>
GPG: F2FF 13DE 6A06 C471 CA80  E6E2 2930 DC66 86EE CF68