From: Derrick Stolee Date: Mon, 23 Apr 2018 14:54:07 GMT Subject: Re: [PATCH v3 0/9] Compute and consume generation numbers Message-ID: In-Reply-To: <86y3hkf4d9.fsf@gmail.com> On 4/18/2018 8:04 PM, Jakub Narebski wrote: > Derrick Stolee 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. The diagrams and tables seemed to cause more confusion than clarity. I think the reader should create their own mental model from the definitions and description and we should avoid trying to make a summary. > >> ref-filter: use generation number for --contains > A question: how performance looks like after the change if commit-graph > is not available? The performance issue is minor, but will be fixed in v4. > >> 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. I responded to say that the global variable approach is incorrect. Parameter is important to functionality and performance. > >> 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