Re: [PATCH 06/12] revision.c: use commit-slab for show_source

[Duy Nguyen <pclouds@gmail.com>]

On Sat, May 12, 2018 at 3:58 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>
>> On Sat, May 12, 2018 at 10:00:22AM +0200, Nguyễn Thái Ngọc Duy wrote:
>>
>>> diff --git a/revision.h b/revision.h
>>> index b8c47b98e2..72404e2599 100644
>>> --- a/revision.h
>>> +++ b/revision.h
>>> @@ -6,6 +6,7 @@
>>>  #include "notes.h"
>>>  #include "pretty.h"
>>>  #include "diff.h"
>>> +#include "commit-slab.h"
>>>
>>>  /* Remember to update object flag allocation in object.h */
>>>  #define SEEN                (1u<<0)
>>> @@ -29,6 +30,7 @@ struct rev_info;
>>>  struct log_info;
>>>  struct string_list;
>>>  struct saved_parents;
>>> +define_commit_slab(source_slab, char *);
>>
>> Since this one is a global, can we give it a name that ties it to the
>> revision machinery? Like "revision_source_slab" or something?
>
> 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.

> 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.
-- 
Duy