On 2023/12/10 02:00:45 +1100, Mark Jamsek <mark@jamsek.com> wrote:
> [...]
> The matched_ids leak (5th commit) in repository.c:match_packed_object()
> is because we initialise the potentially populated matched_ids list upon
> entering pack.c:got_packidx_match_id_str_prefix(). We could zap that
> STAILQ_INIT(matched_ids) call instead (final XX-prepended diff) but then
> we would repeatedly loop over the same matched ids with each index file
> as we'd be appending unique ids to the list from each index file, which
> we are not presently doing because of that STAILQ_INIT(matched_ids) call
> (although I'm not sure if that was the original intent?). So rather than
> only free the list when the pack directory modification time changes,
> free it after each index file iteration instead.

I think both options are fine, and I would actually do both! :)

Freeing the stailq in match_packed_object() is correct and what I would
expect.  maybe I'd just do it always at the start of the foreach loop
instead of the conditional at the end, but both ways are fine with me.

Then, on to got_packidx_match_id_str_prefix(), as it takes a stailq as
argument I'd expect it to append elements to it, moving the ownership of
the items to the caller.  If it fails, it should the caller to deal with
the content of the stailq.  IMHO it shouldn't re-initialize the stailq
nor call got_object_id_queue_free().

(not a big deal since match_packed_object() is the only caller, but still...)

OKs and comments below

> -----------------------------------------------
> commit b6fe41fdfb6219024a9afaea3da2065274a582ac
> from: Mark Jamsek <mark@jamsek.dev>
> date: Sat Dec  9 10:07:53 2023 UTC
>  
>  tog: plug colors memleak in log view

OK op@

> -----------------------------------------------
> commit 3c33690882bfa409292b75db9719c655b9402515
> from: Mark Jamsek <mark@jamsek.dev>
> date: Sat Dec  9 10:10:31 2023 UTC
>  
>  tog: plug commit object leak in 'tog tree'

OK op@

> -----------------------------------------------
> commit 5dd5fa1db32f0c250d6ad41baeebdcb4b2a5795d
> from: Mark Jamsek <mark@jamsek.dev>
> date: Sat Dec  9 10:15:03 2023 UTC
>  
>  tog: plug object id leak in diff view

OK op@

> -----------------------------------------------
> commit 69e758a9c1e5024d68eac9094dc7691838af58db
> from: Mark Jamsek <mark@jamsek.dev>
> date: Sat Dec  9 11:42:11 2023 UTC
>  
>  plug leak of commit object in 'tog diff' error path
>  
>  Reduce the scope of the commit object too.

OK op@

although I would find easier to move commit2 to the function scope,
initialize it to NULL, and always attempt to free it after done.  Less
headaches when coming back to this chunk of code.

by the way, create_diff() has a similar problem with f not being closed
if the second got_opentemp() or fclose() fail.  A similar treatment
could be applied to it too.

> -----------------------------------------------
> commit 557f71d8d65ad007ecbe02338778f4911e49daa2
> from: Mark Jamsek <mark@jamsek.dev>
> date: Sat Dec  9 11:48:32 2023 UTC
>  
>  plug object id queue leak when iterating pack index files
>  
>  We need to free the matched object id queue on each pack index iteration--not
>  only when the pack file modification time has changed--otherwise the ids are
>  leaked when we reinitialise the queue in got_packidx_match_id_str_prefix().

OK op@

although as said before I'd find more easy to move the
got_object_id_queue_free() call at the start of the loop, more or less
after the goto retry if or right before the
got_packidx_match_id_str_prefix() call.

> -----------------------------------------------
> commit 828e5074bf59310d99fdc5be7e6c781ea4c8ba1a (main)
> from: Mark Jamsek <mark@jamsek.dev>
> date: Sat Dec  9 11:48:52 2023 UTC
>  
>  whitespace

whitespaces diff don't need OKs :P

> XXX Alternative fix to the matched_ids leak that _changes_ the current
> behaviour!

I like it.  Actually, as described before, I think we can be a bit more
aggressive:

diff /home/op/w/got
commit - 976ccb693d1bc5f0f91152b31e2d92d6e9064581
path + /home/op/w/got
blob - 09136af9ef6031bc1ba4935ddaba53108806ba54
file + lib/pack.c
--- lib/pack.c
+++ lib/pack.c
@@ -675,8 +675,6 @@ got_packidx_match_id_str_prefix(struct got_object_id_q
 	struct got_packidx_object_id *oid;
 	uint32_t i = 0;
 
-	STAILQ_INIT(matched_ids);
-
 	if (prefix_len < 2)
 		return got_error_path(id_str_prefix, GOT_ERR_BAD_OBJ_ID_STR);
 
@@ -706,16 +704,14 @@ got_packidx_match_id_str_prefix(struct got_object_id_q
 
 		err = got_object_qid_alloc_partial(&qid);
 		if (err)
-			break;
+			return err;
 		memcpy(qid->id.sha1, oid->sha1, SHA1_DIGEST_LENGTH);
 		STAILQ_INSERT_TAIL(matched_ids, qid, entry);
 
 		oid = &packidx->hdr.sorted_ids[++i];
 	}
 
-	if (err)
-		got_object_id_queue_free(matched_ids);
-	return err;
+	return NULL;
 }
 
 static void