patch, 2 partsrevision: free topo_walk_info before creating a new one in init_topo_walk

> 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);

> +
> +	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));

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

> Derrick Stolee <stolee@gmail.com> writes:
> 
>> 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!
> 
> Hmph, isn't this merely a performance thing, or would a slab that
> was once cleared never repopulate upon its second use (i.e.
> affecting correctness)?

> On 12/1/2019 11:22 AM, Junio C Hamano wrote:
>> Derrick Stolee <stolee@gmail.com> writes:
>> 
>>> 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!
>> 
>> Hmph, isn't this merely a performance thing, or would a slab that
>> was once cleared never repopulate upon its second use (i.e.
>> affecting correctness)?
>
> Yes, this is only a performance thing. If you think it is safest to
> clear it here, then it can stay.

> Not doing so can lead to wrong topo-walks when using the revision walk API
> consecutively.
> 
> Signed-off-by: Mike Hommey <mh@glandium.org>
> ---
>  revision.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> As mentioned in 20191108025007.bphr7ynvskeoe6tb@glandium.org, it feels
> like maybe TOPO_WALK_EXPLORED and TOPO_WALK_INDEGREE should be in
> ALL_REV_FLAGS too?
> 
> diff --git a/revision.c b/revision.c
> index 0e39b2b8a5..765a56ae33 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -3098,7 +3098,7 @@ static void set_children(struct rev_info *revs)
>  
>  void reset_revision_walk(void)
>  {
> -	clear_object_flags(SEEN | ADDED | SHOWN);
> +	clear_object_flags(SEEN | ADDED | SHOWN | TOPO_WALK_EXPLORED | TOPO_WALK_INDEGREE);