Re: [PATCH v3 5/7] commit/revisions: bookkeeping before refactoring

[Jeff King <peff@peff.net>]

On Fri, Sep 21, 2018 at 10:39:33AM -0700, Derrick Stolee via GitGitGadget wrote:

> From: Derrick Stolee <dstolee@microsoft.com>
> 
> There are a few things that need to move around a little before
> making a big refactoring in the topo-order logic:
> 
> 1. We need access to record_author_date() and
>    compare_commits_by_author_date() in revision.c. These are used
>    currently by sort_in_topological_order() in commit.c.
> 
> 2. Moving these methods to commit.h requires adding the author_slab
>    definition to commit.h.

The overall goal makes sense. Do we really need to define the whole slab
in the header file? We're going to end up with multiple copies of the
functions, since they're declared static in each file that includes
commit.h.

From what's here, I think you could get away with just:

  struct author_date_slab;
  void record_author_date(struct author_date_slab *author_date,
                          struct commit *commit);

in the header file. But presumably callers would eventually want to
allocate their own author dates. If that's all we need, then these days
you can do:

  declare_commit_slab(author_date, timestamp_t);

to get the type declaration.

If they really do need the functions accessible outside of commit.c,
then perhaps:

  define_shared_commit_slab(author_date, timestamp_t);

in commit.h, and:

  implement_shared_commit_slab(author_date, timestamp_t);

in commit.c (the type repetition is not too bad, as the compiler would
catch any mistakes).

The only downside of this approach is that we're less likely to be able
to inline element access (though "peek" is big enough that I'm not sure
it ends up inlined anyway).

> 3. The add_parents_to_list() method in revision.c performs logic
>    around the UNINTERESTING flag and other special cases depending
>    on the struct rev_info. Allow this method to ignore a NULL 'list'
>    parameter, as we will not be populating the list for our walk.

So now you can add_parents_to_list() without a list? That sounds
confusing. :)

Is it possible to split the function into two? Some
handle_uninteresting_parents() logic, and then an add_parents_to_list()
that calls that, but also adds to the list?

A cursory look at the function suggests it's actually kind of tricky.
Perhaps as an alternative, add_parents_to_list() could just get a more
descriptive name?

> ---
>  commit.c   | 11 ++++-------
>  commit.h   |  8 ++++++++
>  revision.c |  6 ++++--
>  3 files changed, 16 insertions(+), 9 deletions(-)

The patch itself seems straight-forward based on those explanations.

-Peff