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

From: SZEDER Gábor <szeder.dev@gmail.com>
Date: Nov 18, 2019, 23:04 UTC
Message-ID: <20191118230431.GC23183@szeder.dev>
In-Reply-To: <ea07fd13-7d0a-4636-1aa4-453f6b837735@gmail.com>

On Sun, Nov 10, 2019 at 08:07:31PM -0500, Derrick Stolee wrote:

Show 19 quoted lines

> On 11/7/2019 9:50 PM, Mike Hommey wrote:
> > 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.

I don't think that's a valid expectation.

Several commands must do multiple revision walks in a single process, e.g. 'describe' or 'name-rev', but they tend to do so by rolling their own low-level revision walking (e.g. by putting all ~SEEN parents into a 'commit_list' and iterating until the list becomes empty) instead of a higher-level 'while ((commit = get_revision(revs)))' loop.

Alas, some of those commands are buggy, or at least 'git describe' is [1], and AFAICT the only way to fix that bug is to walk the history in topo-order. And of course we should not roll its own topo-order revision walk for each of those commands, but rather should convert them to use get_revision(), so they can all rely on the magic of the commit-graph-based on-the-fly topo-order, especially since the commit-graph is now enabled by default. However, all this means a lot of separate get_revision()-based revision walks in a single process.

[1]  https://public-inbox.org/git/20191008123156.GG11529@szeder.dev/