From: Taylor Blau <me@ttaylorr.com> To: "Jakub Narębski" <jnareb@gmail.com> Cc: Abhishek Kumar via GitGitGadget <gitgitgadget@gmail.com>, git@vger.kernel.org, Derrick Stolee <stolee@gmail>, Abhishek Kumar <abhishekkumar8222@gmail.com>, Taylor Blau <me@ttaylorr.com> Subject: Re: [PATCH 1/6] commit-graph: fix regression when computing bloom filter Date: Mon, 3 Aug 2020 20:56:58 -0400 Message-ID: <20200804005658.GB75662@syl.lan> (raw) In-Reply-To: <85eeonutj4.fsf@gmail.com> On Tue, Aug 04, 2020 at 02:46:55AM +0200, Jakub Narębski wrote: > "Abhishek Kumar via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > From: Abhishek Kumar <abhishekkumar8222@gmail.com> > > > > With 3d112755 (commit-graph: examine commits by generation number), Git > > knew to sort by generation number before examining the diff when not > > using pack order. c49c82aa (commit: move members graph_pos, generation > > to a slab, 2020-06-17) moved generation number into a slab and > > introduced a helper which returns GENERATION_NUMBER_INFINITY when > > writing the graph. Sorting is no longer useful and essentially reverts > > the earlier commit. > > > > Let's fix this by accessing generation number directly through the slab. > > It looks like unfortunate and unforeseen consequence of putting together > graph position and generation number in the commit_graph_data struct. > During writing of the commit-graph file generation number is computed, > but graph position is undefined (yet), and commit_graph_generation() > uses graph_pos field to find if the data for commit is initialized; > in this case wrongly. > > Anyway, when writing the commit graph we first compute generation > number, then (if requested) the changed-paths Bloom filter. Skipping > the unnecessary check is a good thing... assuming that commit_gen_cmp() > is used only when writing the commit graph, and not when traversing it > (because then some commits may not have generation number set, and maybe > even do not have any data on the commit slab) - which is the case. > > > > > Signed-off-by: Abhishek Kumar <abhishekkumar8222@gmail.com> > > --- > > commit-graph.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/commit-graph.c b/commit-graph.c > > index 1af68c297d..5d3c9bd23c 100644 > > --- a/commit-graph.c > > +++ b/commit-graph.c > > We might want to add function comment either here or in the header that > this comparisonn function is to be used only for `git commit-graph > write`, and not for graph traversal (even if similar funnction exists in > other modules). I think that probably within the function is just fine, and that we can avoid touching commit-graph.h here. > > > @@ -144,8 +144,9 @@ static int commit_gen_cmp(const void *va, const void *vb) > > const struct commit *a = *(const struct commit **)va; > > const struct commit *b = *(const struct commit **)vb; Maybe something like: /* * Access the generation number directly with * 'commit_graph_data_at(...)->generation' instead of going through * the slab as usual to avoid accessing a yet-uncomputed value. */ Folks that are curious for more can blame this commit and read there. I'd err on the side of being brief in the code comment and verbose in the commit message than the other way around ;). > > > > - uint32_t generation_a = commit_graph_generation(a); > > - uint32_t generation_b = commit_graph_generation(b); > > + uint32_t generation_a = commit_graph_data_at(a)->generation; > > + uint32_t generation_b = commit_graph_data_at(b)->generation; > > + > > /* lower generation commits first */ > > if (generation_a < generation_b) > > return -1; > > Best, > -- > Jakub Narębski Thanks, Taylor
next prev parent reply other threads:[~2020-08-04 0:57 UTC|newest] Thread overview: 189+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-07-28 9:13 [PATCH 0/6] [GSoC] Implement Corrected Commit Date Abhishek Kumar via GitGitGadget 2020-07-28 9:13 ` [PATCH 1/6] commit-graph: fix regression when computing bloom filter Abhishek Kumar via GitGitGadget 2020-07-28 15:28 ` Taylor Blau 2020-07-30 5:24 ` Abhishek Kumar 2020-08-04 0:46 ` Jakub Narębski 2020-08-04 0:56 ` Taylor Blau [this message] 2020-08-04 10:10 ` Jakub Narębski 2020-08-04 7:55 ` Jakub Narębski 2020-07-28 9:13 ` [PATCH 2/6] revision: parse parent in indegree_walk_step() Abhishek Kumar via GitGitGadget 2020-07-28 13:00 ` Derrick Stolee 2020-07-28 15:30 ` Taylor Blau 2020-08-05 23:16 ` Jakub Narębski 2020-07-28 9:13 ` [PATCH 3/6] commit-graph: consolidate fill_commit_graph_info Abhishek Kumar via GitGitGadget 2020-07-28 13:14 ` Derrick Stolee 2020-07-28 15:19 ` René Scharfe 2020-07-28 15:58 ` Derrick Stolee 2020-07-28 16:01 ` Taylor Blau 2020-07-30 6:07 ` Abhishek Kumar 2020-07-28 9:13 ` [PATCH 4/6] commit-graph: consolidate compare_commits_by_gen Abhishek Kumar via GitGitGadget 2020-07-28 16:03 ` Taylor Blau 2020-07-28 9:13 ` [PATCH 5/6] commit-graph: implement generation data chunk Abhishek Kumar via GitGitGadget 2020-07-28 16:12 ` Taylor Blau 2020-07-30 6:52 ` Abhishek Kumar 2020-07-28 9:13 ` [PATCH 6/6] commit-graph: implement corrected commit date offset Abhishek Kumar via GitGitGadget 2020-07-28 15:55 ` Derrick Stolee 2020-07-28 16:23 ` Taylor Blau 2020-07-30 7:27 ` Abhishek Kumar 2020-07-28 14:54 ` [PATCH 0/6] [GSoC] Implement Corrected Commit Date Taylor Blau 2020-07-30 7:47 ` Abhishek Kumar 2020-07-28 16:35 ` Derrick Stolee 2020-08-09 2:53 ` [PATCH v2 00/10] " Abhishek Kumar via GitGitGadget 2020-08-09 2:53 ` [PATCH v2 01/10] commit-graph: fix regression when computing bloom filter Abhishek Kumar via GitGitGadget 2020-08-09 2:53 ` [PATCH v2 02/10] revision: parse parent in indegree_walk_step() Abhishek Kumar via GitGitGadget 2020-08-09 2:53 ` [PATCH v2 03/10] commit-graph: consolidate fill_commit_graph_info Abhishek Kumar via GitGitGadget 2020-08-09 2:53 ` [PATCH v2 04/10] commit-graph: consolidate compare_commits_by_gen Abhishek Kumar via GitGitGadget 2020-08-09 2:53 ` [PATCH v2 05/10] commit-graph: implement generation data chunk Abhishek Kumar via GitGitGadget 2020-08-10 16:28 ` Derrick Stolee 2020-08-11 11:03 ` Abhishek Kumar 2020-08-11 12:27 ` Derrick Stolee 2020-08-11 18:58 ` Taylor Blau 2020-08-09 2:53 ` [PATCH v2 06/10] commit-graph: return 64-bit generation number Abhishek Kumar via GitGitGadget 2020-08-09 2:53 ` [PATCH v2 07/10] commit-graph: implement corrected commit date Abhishek Kumar via GitGitGadget 2020-08-10 14:23 ` Derrick Stolee 2020-08-14 4:59 ` Abhishek Kumar 2020-08-14 12:24 ` Derrick Stolee 2020-08-09 2:53 ` [PATCH v2 08/10] commit-graph: handle mixed generation commit chains Abhishek Kumar via GitGitGadget 2020-08-10 16:42 ` Derrick Stolee 2020-08-11 11:36 ` Abhishek Kumar 2020-08-11 12:43 ` Derrick Stolee 2020-08-09 2:53 ` [PATCH v2 09/10] commit-reach: use corrected commit dates in paint_down_to_common() Abhishek Kumar via GitGitGadget 2020-08-09 2:53 ` [PATCH v2 10/10] doc: add corrected commit date info Abhishek Kumar via GitGitGadget 2020-08-10 16:47 ` [PATCH v2 00/10] [GSoC] Implement Corrected Commit Date Derrick Stolee 2020-08-15 16:39 ` [PATCH v3 00/11] " Abhishek Kumar via GitGitGadget 2020-08-15 16:39 ` [PATCH v3 01/11] commit-graph: fix regression when computing bloom filter Abhishek Kumar via GitGitGadget 2020-08-17 22:30 ` Jakub Narębski 2020-08-15 16:39 ` [PATCH v3 02/11] revision: parse parent in indegree_walk_step() Abhishek Kumar via GitGitGadget 2020-08-18 14:18 ` Jakub Narębski 2020-08-15 16:39 ` [PATCH v3 03/11] commit-graph: consolidate fill_commit_graph_info Abhishek Kumar via GitGitGadget 2020-08-19 17:54 ` Jakub Narębski 2020-08-21 4:11 ` Abhishek Kumar 2020-08-25 11:11 ` Jakub Narębski 2020-09-01 11:35 ` Abhishek Kumar 2020-08-15 16:39 ` [PATCH v3 04/11] commit-graph: consolidate compare_commits_by_gen Abhishek Kumar via GitGitGadget 2020-08-17 13:22 ` Derrick Stolee 2020-08-21 11:05 ` Jakub Narębski 2020-08-15 16:39 ` [PATCH v3 05/11] commit-graph: return 64-bit generation number Abhishek Kumar via GitGitGadget 2020-08-21 13:14 ` Jakub Narębski 2020-08-25 5:04 ` Abhishek Kumar 2020-08-25 12:18 ` Jakub Narębski 2020-09-01 12:06 ` Abhishek Kumar 2020-09-03 13:42 ` Jakub Narębski 2020-09-05 17:21 ` Abhishek Kumar 2020-09-13 15:39 ` Jakub Narębski 2020-09-28 21:48 ` Jakub Narębski 2020-10-05 5:25 ` Abhishek Kumar 2020-08-15 16:39 ` [PATCH v3 06/11] commit-graph: add a slab to store topological levels Abhishek Kumar via GitGitGadget 2020-08-21 18:43 ` Jakub Narębski 2020-08-25 6:14 ` Abhishek Kumar 2020-08-25 7:33 ` Jakub Narębski 2020-08-25 7:56 ` Jakub Narębski 2020-09-01 10:26 ` Abhishek Kumar 2020-09-03 9:25 ` Jakub Narębski 2020-08-15 16:39 ` [PATCH v3 07/11] commit-graph: implement corrected commit date Abhishek Kumar via GitGitGadget 2020-08-22 0:05 ` Jakub Narębski 2020-08-25 6:49 ` Abhishek Kumar 2020-08-25 10:07 ` Jakub Narębski 2020-09-01 11:01 ` Abhishek Kumar 2020-08-15 16:39 ` [PATCH v3 08/11] commit-graph: implement generation data chunk Abhishek Kumar via GitGitGadget 2020-08-22 13:09 ` Jakub Narębski 2020-08-15 16:39 ` [PATCH v3 09/11] commit-graph: use generation v2 only if entire chain does Abhishek Kumar via GitGitGadget 2020-08-22 17:14 ` Jakub Narębski 2020-08-26 7:15 ` Abhishek Kumar 2020-08-26 10:38 ` Jakub Narębski 2020-08-15 16:39 ` [PATCH v3 10/11] commit-reach: use corrected commit dates in paint_down_to_common() Abhishek Kumar via GitGitGadget 2020-08-22 19:09 ` Jakub Narębski 2020-09-01 10:08 ` Abhishek Kumar 2020-09-03 19:11 ` Jakub Narębski 2020-08-15 16:39 ` [PATCH v3 11/11] doc: add corrected commit date info Abhishek Kumar via GitGitGadget 2020-08-22 22:20 ` Jakub Narębski 2020-08-27 6:39 ` Abhishek Kumar 2020-08-27 12:43 ` Jakub Narębski 2020-08-27 13:15 ` Derrick Stolee 2020-09-01 13:01 ` Abhishek Kumar 2020-08-17 0:13 ` [PATCH v3 00/11] [GSoC] Implement Corrected Commit Date Jakub Narębski [not found] ` <CANQwDwdKp7oKy9BeKdvKhwPUiq0R5MS8TCw-eWGCYCoMGv=G-g@mail.gmail.com> 2020-08-17 1:32 ` Fwd: " Taylor Blau 2020-08-17 7:56 ` Jakub Narębski 2020-08-18 6:12 ` Abhishek Kumar 2020-08-23 15:27 ` Jakub Narębski 2020-08-24 2:49 ` Abhishek Kumar 2020-10-07 14:09 ` [PATCH v4 00/10] " Abhishek Kumar via GitGitGadget 2020-10-07 14:09 ` [PATCH v4 01/10] commit-graph: fix regression when computing Bloom filters Abhishek Kumar via GitGitGadget 2020-10-24 23:16 ` Jakub Narębski 2020-10-25 20:58 ` Taylor Blau 2020-11-03 5:36 ` Abhishek Kumar 2020-10-07 14:09 ` [PATCH v4 02/10] revision: parse parent in indegree_walk_step() Abhishek Kumar via GitGitGadget 2020-10-24 23:41 ` Jakub Narębski 2020-10-07 14:09 ` [PATCH v4 03/10] commit-graph: consolidate fill_commit_graph_info Abhishek Kumar via GitGitGadget 2020-10-25 10:52 ` Jakub Narębski 2020-10-27 6:33 ` Abhishek Kumar 2020-10-07 14:09 ` [PATCH v4 04/10] commit-graph: return 64-bit generation number Abhishek Kumar via GitGitGadget 2020-10-25 13:48 ` Jakub Narębski 2020-11-03 6:40 ` Abhishek Kumar 2020-10-07 14:09 ` [PATCH v4 05/10] commit-graph: add a slab to store topological levels Abhishek Kumar via GitGitGadget 2020-10-25 22:17 ` Jakub Narębski 2020-10-07 14:09 ` [PATCH v4 06/10] commit-graph: implement corrected commit date Abhishek Kumar via GitGitGadget 2020-10-27 18:53 ` Jakub Narębski 2020-11-03 11:44 ` Abhishek Kumar 2020-11-04 16:45 ` Jakub Narębski 2020-11-05 14:05 ` Philip Oakley 2020-11-05 18:22 ` Junio C Hamano 2020-11-06 18:26 ` Extending and updating gitglossary (was: Re: [PATCH v4 06/10] commit-graph: implement corrected commit date) Jakub Narębski 2020-11-06 19:33 ` Extending and updating gitglossary Junio C Hamano 2020-11-08 17:23 ` Extending and updating gitglossary (was: Re: [PATCH v4 06/10] commit-graph: implement corrected commit date) Philip Oakley 2020-11-10 1:35 ` Extending and updating gitglossary Jakub Narębski 2020-11-10 14:04 ` Philip Oakley 2020-11-10 23:52 ` Jakub Narębski 2020-10-07 14:09 ` [PATCH v4 07/10] commit-graph: implement generation data chunk Abhishek Kumar via GitGitGadget 2020-10-30 12:45 ` Jakub Narębski 2020-11-06 11:25 ` Abhishek Kumar 2020-11-06 17:56 ` Jakub Narębski 2020-10-07 14:09 ` [PATCH v4 08/10] commit-graph: use generation v2 only if entire chain does Abhishek Kumar via GitGitGadget 2020-11-01 0:55 ` Jakub Narębski 2020-11-12 10:01 ` Abhishek Kumar 2020-11-13 9:59 ` Jakub Narębski 2020-10-07 14:09 ` [PATCH v4 09/10] commit-reach: use corrected commit dates in paint_down_to_common() Abhishek Kumar via GitGitGadget 2020-11-03 17:59 ` Jakub Narębski 2020-11-03 18:19 ` Junio C Hamano 2020-11-20 10:33 ` Abhishek Kumar 2020-10-07 14:09 ` [PATCH v4 10/10] doc: add corrected commit date info Abhishek Kumar via GitGitGadget 2020-11-04 1:37 ` Jakub Narębski 2020-11-21 6:30 ` Abhishek Kumar 2020-11-04 23:37 ` [PATCH v4 00/10] [GSoC] Implement Corrected Commit Date Jakub Narębski 2020-11-22 5:31 ` Abhishek Kumar 2020-12-28 11:15 ` [PATCH v5 00/11] " Abhishek Kumar via GitGitGadget 2020-12-28 11:15 ` [PATCH v5 01/11] commit-graph: fix regression when computing Bloom filters Abhishek Kumar via GitGitGadget 2020-12-30 1:35 ` Derrick Stolee 2021-01-08 5:45 ` Abhishek Kumar 2021-01-05 9:45 ` SZEDER Gábor 2021-01-05 9:47 ` SZEDER Gábor 2021-01-08 5:51 ` Abhishek Kumar 2020-12-28 11:15 ` [PATCH v5 02/11] revision: parse parent in indegree_walk_step() Abhishek Kumar via GitGitGadget 2020-12-28 11:16 ` [PATCH v5 03/11] commit-graph: consolidate fill_commit_graph_info Abhishek Kumar via GitGitGadget 2020-12-28 11:16 ` [PATCH v5 04/11] t6600-test-reach: generalize *_three_modes Abhishek Kumar via GitGitGadget 2020-12-28 11:16 ` [PATCH v5 05/11] commit-graph: add a slab to store topological levels Abhishek Kumar via GitGitGadget 2020-12-28 11:16 ` [PATCH v5 06/11] commit-graph: return 64-bit generation number Abhishek Kumar via GitGitGadget 2020-12-28 11:16 ` [PATCH v5 07/11] commit-graph: implement corrected commit date Abhishek Kumar via GitGitGadget 2020-12-30 1:53 ` Derrick Stolee 2021-01-10 12:21 ` Abhishek Kumar 2020-12-28 11:16 ` [PATCH v5 08/11] commit-graph: implement generation data chunk Abhishek Kumar via GitGitGadget 2020-12-28 11:16 ` [PATCH v5 09/11] commit-graph: use generation v2 only if entire chain does Abhishek Kumar via GitGitGadget 2020-12-30 3:23 ` Derrick Stolee 2021-01-10 13:13 ` Abhishek Kumar 2021-01-11 12:43 ` Derrick Stolee 2020-12-28 11:16 ` [PATCH v5 10/11] commit-reach: use corrected commit dates in paint_down_to_common() Abhishek Kumar via GitGitGadget 2020-12-28 11:16 ` [PATCH v5 11/11] doc: add corrected commit date info Abhishek Kumar via GitGitGadget 2020-12-30 4:35 ` [PATCH v5 00/11] [GSoC] Implement Corrected Commit Date Derrick Stolee 2021-01-10 14:06 ` Abhishek Kumar 2021-01-16 18:11 ` [PATCH v6 " Abhishek Kumar via GitGitGadget 2021-01-16 18:11 ` [PATCH v6 01/11] commit-graph: fix regression when computing Bloom filters Abhishek Kumar via GitGitGadget 2021-01-16 18:11 ` [PATCH v6 02/11] revision: parse parent in indegree_walk_step() Abhishek Kumar via GitGitGadget 2021-01-16 18:11 ` [PATCH v6 03/11] commit-graph: consolidate fill_commit_graph_info Abhishek Kumar via GitGitGadget 2021-01-16 18:11 ` [PATCH v6 04/11] t6600-test-reach: generalize *_three_modes Abhishek Kumar via GitGitGadget 2021-01-16 18:11 ` [PATCH v6 05/11] commit-graph: add a slab to store topological levels Abhishek Kumar via GitGitGadget 2021-01-16 18:11 ` [PATCH v6 06/11] commit-graph: return 64-bit generation number Abhishek Kumar via GitGitGadget 2021-01-16 18:11 ` [PATCH v6 07/11] commit-graph: implement corrected commit date Abhishek Kumar via GitGitGadget 2021-01-16 18:11 ` [PATCH v6 08/11] commit-graph: implement generation data chunk Abhishek Kumar via GitGitGadget 2021-01-16 18:11 ` [PATCH v6 09/11] commit-graph: use generation v2 only if entire chain does Abhishek Kumar via GitGitGadget 2021-01-16 18:11 ` [PATCH v6 10/11] commit-reach: use corrected commit dates in paint_down_to_common() Abhishek Kumar via GitGitGadget 2021-01-16 18:11 ` [PATCH v6 11/11] doc: add corrected commit date info Abhishek Kumar via GitGitGadget
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style List information: http://vger.kernel.org/majordomo-info.html * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20200804005658.GB75662@syl.lan \ --to=me@ttaylorr.com \ --cc=abhishekkumar8222@gmail.com \ --cc=git@vger.kernel.org \ --cc=gitgitgadget@gmail.com \ --cc=jnareb@gmail.com \ --cc=stolee@gmail \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
git@vger.kernel.org list mirror (unofficial, one of many) This inbox may be cloned and mirrored by anyone: git clone --mirror https://public-inbox.org/git git clone --mirror http://ou63pmih66umazou.onion/git git clone --mirror http://czquwvybam4bgbro.onion/git git clone --mirror http://hjrcffqmbrq6wope.onion/git # If you have public-inbox 1.1+ installed, you may # initialize and index your mirror using the following commands: public-inbox-init -V1 git git/ https://public-inbox.org/git \ git@vger.kernel.org public-inbox-index git Example config snippet for mirrors. Newsgroups are available over NNTP: nntp://news.public-inbox.org/inbox.comp.version-control.git nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git nntp://news.gmane.io/gmane.comp.version-control.git note: .onion URLs require Tor: https://www.torproject.org/ code repositories for the project(s) associated with this inbox: https://80x24.org/mirrors/git.git AGPL code for this site: git clone https://public-inbox.org/public-inbox.git