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.

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