Re: [PATCH 2/2] revision: free topo_walk_info before creating a new one in init_topo_walk

From: Derrick Stolee <stolee@gmail.com>
Date: Nov 27, 2019, 14:25 UTC
Message-ID: <46c47a21-d0e1-296f-5563-ae29597f995c@gmail.com>
In-Reply-To: <20191122083704.29267-2-mh@glandium.org>

On 11/22/2019 3:37 AM, Mike Hommey wrote:> diff --git a/revision.c b/revision.c

Show 15 quoted lines

> index 765a56ae33..7e23c5ed08 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -3211,10 +3211,26 @@ static void compute_indegrees_to_depth(struct rev_info *revs,
>  		indegree_walk_step(revs);
>  }
>  
> +static void reset_topo_walk(struct rev_info *revs)
> +{
> +	struct topo_walk_info *info = revs->topo_walk_info;
> +
> +	clear_prio_queue(&info->explore_queue);
> +	clear_prio_queue(&info->indegree_queue);
> +	clear_prio_queue(&info->topo_queue);
> +	clear_indegree_slab(&info->indegree);

In general I like this change. I'm happy that this was split into a method instead of crammed into the block of the "if" below.

> +	clear_author_date_slab(&info->author_date);

The only issue I have is that the author_date slab should not be cleared. That is used by more than the topo-walk AND the values for author dates will not change between subsequent revision walks. Just drop that line and we should be good to go!

Show 14 quoted lines

> +
> +	FREE_AND_NULL(revs->topo_walk_info);
> +}
> +
>  static void init_topo_walk(struct rev_info *revs)
>  {
>  	struct topo_walk_info *info;
>  	struct commit_list *list;
> +	if (revs->topo_walk_info)
> +		reset_topo_walk(revs);
> +
>  	revs->topo_walk_info = xmalloc(sizeof(struct topo_walk_info));
>  	info = revs->topo_walk_info;
>  	memset(info, 0, sizeof(struct topo_walk_info));

Thanks, -Stolee