On Sun, Nov 10, 2019 at 08:07:31PM -0500, Derrick Stolee wrote:
> 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@xxxxxxxxxx/
