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

From: Jeff King <peff@peff.net>
Date: Nov 22, 2022, 20:22 UTC
Message-ID: <Y30vf76mdrSv4We4@coredump.intra.peff.net>
In-Reply-To: <20221117230658.M516129@dcvr>

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

Show 7 quoted lines

> 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:

Show 12 quoted lines

>  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