Re: [PATCH v2] delta-islands: free island-related data after use

[Jeff King <peff@peff.net>]

On Thu, Nov 17, 2022 at 11:06:58PM +0000, Eric Wong wrote:

> Aside from that, v2 below still frees the regex memory early on
> in the hopes deduplicate_islands() can reuse some of the freed
> regexp memory.
> 
> Anyways, here's v2, which seems to work.  I'm still trying to
> figure out SATA errors+resets after replacing a CMOS battery,
> but I really hope this patch isn't the cause.

This looks OK to me, though I think it would have been easier to review
split into two patches (one pushing the globals into local variables and
the other adding the freeing).

Two small notes:

>  void load_delta_islands(struct repository *r, int progress)
>  {
> +	struct island_load_data ild = { 0 };
> +
>  	island_marks = kh_init_oid_map();
> -	remote_islands = kh_init_str();
>  
> -	git_config(island_config_callback, NULL);
> -	for_each_ref(find_island_for_ref, NULL);
> -	deduplicate_islands(r);
> +	git_config(island_config_callback, &ild);
> +	ild.remote_islands = kh_init_str();

The initialization of the remote_islands khash is now moved after we
read the config. That's OK, because our callback doesn't read it, but
it's not immediately obvious without going back to check the callback.

Splitting it into:

  struct island_load_data {
	kh_str_t *remote_islands;
	struct island_regexes regexes {
		regex_t *rx;
		size_t nr;
		size_t alloc;
	};
  };

lets you pass:

  git_config(island_config_callback, &ild.regexes);

which makes it clear that the khash part isn't touched. But you still
get to pass the whole &ild around later. Of course that's all going
through a void pointer, so you're praying that the callback expects the
right type anyway. ;)

And with your code we'd hopefully notice the problem right away since
the khash pointer is NULL. So it might not be that big a deal.

> +	for_each_ref(find_island_for_ref, &ild);
> +	free_config_regexes(&ild);
> +	deduplicate_islands(ild.remote_islands, r);
> +	free_remote_islands(ild.remote_islands);

Here we free the regexes, but they're pointing to garbage memory still.
But since we pass just the remote_islands part of the struct to those
functions, we know they can't look at the garbage regexes. Good.

I'd have said it ought to be two separate variables, but the
for_each_ref() callback forces your hand there.

-Peff