Omar Polo <op@omarpolo.com> wrote:
> 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

Thanks op and sorry for letting this sleep for so long!

I've committed these and applied your suggestions to the create_diff()
and match_packed_object() leaks.

I see what you mean about the f FILE pointer in create_diff() too. I'm a
bit later than expected tonight so will send a diff tomorrow; I just
didn't want to zzz before tending to this after seeing your ping on irc
today.

Regarding your below diff, yes ok. This is what I was flirting with in
my original mail but wasn't sure so wanted to float my thoughts, but I
do agree with your commentary above.

Thanks again op :)

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

ok for me

> 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


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