Re: [PATCH v3 0/9] Compute and consume generation numbers

[Jakub Narebski <jnareb@gmail.com>]

Derrick Stolee <dstolee@microsoft.com> writes:

> -- >8 --
>
> This is the one of several "small" patches that follow the serialized
> Git commit graph patch (ds/commit-graph) and lazy-loading trees
> (ds/lazy-load-trees).
>
> As described in Documentation/technical/commit-graph.txt, the generation
> number of a commit is one more than the maximum generation number among
> its parents (trivially, a commit with no parents has generation number
> one). This section is expanded to describe the interaction with special
> generation numbers GENERATION_NUMBER_INFINITY (commits not in the commit-graph
> file) and *_ZERO (commits in a commit-graph file written before generation
> numbers were implemented).
>
> This series makes the computation of generation numbers part of the
> commit-graph write process.
>
> Finally, generation numbers are used to order commits in the priority
> queue in paint_down_to_common(). This allows a short-circuit mechanism
> to improve performance of `git branch --contains`.
>
> Further, use generation numbers for 'git tag --contains', providing a
> significant speedup (at least 95% for some cases).
>
> A more substantial refactoring of revision.c is required before making
> 'git log --graph' use generation numbers effectively.
>
> This patch series is build on ds/lazy-load-trees.
>
> Derrick Stolee (9):
>   commit: add generation number to struct commmit

Nice and short patch. Looks good to me.

>   commit-graph: compute generation numbers

Another quite easy to understand patch. LGTM.

>   commit: use generations in paint_down_to_common()

Nice and short patch; minor typo in comment in code.
Otherwise it looks good to me.

>   commit-graph.txt: update design document

I see that diagram got removed in this version; maybe it could be
replaced with relationship table?

Anyway, it looks good to me.

>   ref-filter: use generation number for --contains

A question: how performance looks like after the change if commit-graph
is not available?

>   commit: use generation numbers for in_merge_bases()

Possible typo in the commit message, and stylistic inconsistence in
in_merge_bases() - though actually more clear than existing code.

Short, simple, and gives good performance improvenebts.

>   commit: add short-circuit to paint_down_to_common()

Looks good to me; ignore [mostly] what I have written in response to the
patch in question.

>   commit-graph: always load commit-graph information

Looks all right; question: parameter or one more global variable.

>   merge: check config before loading commits

This looks good to me.

>
>  Documentation/technical/commit-graph.txt | 30 +++++--
>  alloc.c                                  |  1 +
>  builtin/merge.c                          |  5 +-
>  commit-graph.c                           | 99 +++++++++++++++++++-----
>  commit-graph.h                           |  8 ++
>  commit.c                                 | 54 +++++++++++--
>  commit.h                                 |  7 +-
>  object.c                                 |  2 +-
>  ref-filter.c                             | 23 +++++-
>  sha1_file.c                              |  2 +-
>  t/t5318-commit-graph.sh                  |  9 +++
>  11 files changed, 199 insertions(+), 41 deletions(-)
>
>
> base-commit: 7b8a21dba1bce44d64bd86427d3d92437adc4707