Re: [PATCH 06/12] revision.c: use commit-slab for show_source
- Date: Sat, 12 May 2018 15:06:44 -0400
- From: Jeff King <peff@xxxxxxxx>
- Subject: Re: [PATCH 06/12] revision.c: use commit-slab for show_source
On Sat, May 12, 2018 at 04:13:59PM +0200, Duy Nguyen wrote:
> > Should this one be global in the first place? Can we tie it to say
> > "struct rev_info" or something? I'd somehow anticipate a far future
> > where object flag bits used for traversal book-keeping would be moved
> > out of the objects themselves and multiple simultanous traversal
> > becomes reality.
> Yeah I thought about those 27 bits but discarded it because it was not
> that much. But now that you brought up the maintainability aspect of
> it, it might make sense to move those bits out (if we manage to make
> commit-slab (or a specialized version of it) manage bitmaps instead of
> primitive types, which is not impossible).
> I don't understand the tying to struct rev_info though. This is a
> struct definition and cannot really be tied to another struct. The
> pointer to struct source_slab _is_ tied to struct rev_info.
Right. We have a global type name and global functions, because this is
C. But the actual variable itself is inside struct rev_info (you used a
pointer to a caller-allocated struct, but I think that's fine, too).
> > Not limited to this particular one, but we'd need to think how the
> > commit_slab mechanism should interact with the_repository idea
> > Stefan has been having fun with. If the object pool is maintained
> > per in-core repository object, presumably we'd have more than one
> > in-core instances of the same commit object if we have more than one
> > repository objects, and decorating one with a slab data may not want
> > to decorate the other one at the same time.
> It should be ok. The slab is indexed by the "commit index" which is
> already per parsed_object_pool. Commit-slab user has to be careful to
> use the right slab with the right repo and free it at the right time,
> but that I think is outside with struct repository.
In theory somebody could misuse a "struct commit" from the wrong
repository and get nonsense results. I'd like to think that would be
pretty hard, but I guess it could be possible to make a mistake like
that at the interface where we call into a submodule-related function.
You could get around it by making the commit_index global across all
pools, but I don't think that's a good idea. Since it's an array index,
we want it to be compact and low.