Re: [PATCH v5 6/7] revision.c: generation-based topo-order algorithm

From: Derrick Stolee <stolee@gmail.com>
Date: Nov 11, 2019, 01:07 UTC
Message-ID: <ea07fd13-7d0a-4636-1aa4-453f6b837735@gmail.com>
In-Reply-To: <20191108025007.bphr7ynvskeoe6tb@glandium.org>

On 11/7/2019 9:50 PM, Mike Hommey wrote:

Show 13 quoted lines

> Replying to this old thread because I have questions regarding the
> patch, in the context of problems I had downstream, in git-cinnabar.
> 
> On Thu, Nov 01, 2018 at 01:46:22PM +0000, Derrick Stolee wrote:
>>  static void init_topo_walk(struct rev_info *revs)
>>  {
>>  	struct topo_walk_info *info;
>> +	struct commit_list *list;
>>  	revs->topo_walk_info = xmalloc(sizeof(struct topo_walk_info));
> 
> Not directly from this patch, but there's nothing that frees this memory
> AFAICS, but then, that's also true for most of the things in struct
> rev_info.

This is true, the 'struct rev_info' doesn't get cleaned up at the end. It is probably a lot of work to find all the consumers and get them to clean everything up, and the value is rather low. I believe the expectation is that each process will only run a revision walk at most once.

Show 16 quoted lines

>> diff --git a/revision.h b/revision.h
>> index fd4154ff75..b0b3bb8025 100644
>> --- a/revision.h
>> +++ b/revision.h
>> @@ -24,6 +24,8 @@
>>  #define USER_GIVEN	(1u<<25) /* given directly by the user */
>>  #define TRACK_LINEAR	(1u<<26)
>>  #define ALL_REV_FLAGS	(((1u<<11)-1) | USER_GIVEN | TRACK_LINEAR)
>> +#define TOPO_WALK_EXPLORED	(1u<<27)
>> +#define TOPO_WALK_INDEGREE	(1u<<28)
> 
> Should these two flags be included in ALL_REV_FLAGS?
> Should they be reset by reset_revision_walk?
> 
> At least for the latter, I'd say yes, otherwise you can end up with
> missing revs in a subsequent topo-order revwalk.

This is probably true. Sounds like a quick contribution could be in order?

Thanks, -Stolee