| Message ID | 0fea7803d86ca17451af408e1bf93c32690edc44.1723743050.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 187504f9b2b2af89c77247100b246a704c68c884 |
| Headers | show |
| Series | pseudo-merge: avoid empty and non-closed pseudo-merge commits | expand |
On Thu, Aug 15, 2024 at 01:31:12PM -0400, Taylor Blau wrote: > Ordinarily, the pack-bitmap machinery will select some subset of > reachable commits to receive bitmaps. But when there are fewer than 100 > commits indexed in the first place, they will all receive bitmaps as a > special case. > > When this happens, pseudo-merges are not generated, making it impossible > to test pseudo-merge corner cases with fewer than 100 commits. > > Select pseudo-merges even for bitmaps with fewer than 100 commits to > make such testing easier. In practice, this should not make a difference > to non-testing bitmaps, as they are unlikely to be used when a > repository has so few commits to begin with. I think you could argue that if there are fewer than 100 commits in the history that pseudo-merge bitmaps are overkill, so it does not matter much either way. But I think being consistent with our behavior (i.e., generating them if asked) is important for testing and debugging. > diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c > index 346fb29513..923f793cec 100644 > --- a/pack-bitmap-write.c > +++ b/pack-bitmap-write.c > @@ -694,6 +694,10 @@ void bitmap_writer_select_commits(struct bitmap_writer *writer, > if (indexed_commits_nr < 100) { > for (i = 0; i < indexed_commits_nr; ++i) > bitmap_writer_push_commit(writer, indexed_commits[i], 0); > + > + select_pseudo_merges(writer, indexed_commits, > + indexed_commits_nr); > + I of course just posted a series which drops the latter two arguments from this function. It's a semantic conflict, not a textual one, so we'll have to fix it up when the two are merged (but the compiler will easily notice to remind us). -Peff
Jeff King <peff@peff.net> writes: >> Select pseudo-merges even for bitmaps with fewer than 100 commits to >> make such testing easier. In practice, this should not make a difference >> to non-testing bitmaps, as they are unlikely to be used when a >> repository has so few commits to begin with. > > I think you could argue that if there are fewer than 100 commits in the > history that pseudo-merge bitmaps are overkill, so it does not matter > much either way. But I think being consistent with our behavior (i.e., > generating them if asked) is important for testing and debugging. OK. I had an opposite reaction to this change, i.e. "eh, are we lifting the safety that protected the users from asking for a suboptimal way to pack, only to make it easier to write this test?", but I do buy "the user tells us to do the suboptimal thing, we do that thing."
On Sat, Aug 17, 2024 at 06:34:01AM -0400, Jeff King wrote: > On Thu, Aug 15, 2024 at 01:31:12PM -0400, Taylor Blau wrote: > > > Ordinarily, the pack-bitmap machinery will select some subset of > > reachable commits to receive bitmaps. But when there are fewer than 100 > > commits indexed in the first place, they will all receive bitmaps as a > > special case. > > > > When this happens, pseudo-merges are not generated, making it impossible > > to test pseudo-merge corner cases with fewer than 100 commits. > > > > Select pseudo-merges even for bitmaps with fewer than 100 commits to > > make such testing easier. In practice, this should not make a difference > > to non-testing bitmaps, as they are unlikely to be used when a > > repository has so few commits to begin with. > > I think you could argue that if there are fewer than 100 commits in the > history that pseudo-merge bitmaps are overkill, so it does not matter > much either way. But I think being consistent with our behavior (i.e., > generating them if asked) is important for testing and debugging. Oh, I think that argument is very fair and makes a lot of sense. The point of this patch wasn't that having pseudo-merge bitmaps for repositories that small (and which already have complete bitmap coverage!) is helpful for performance. Rather, it was to make it easier to test the pseudo-merge code path in small repositories without having to waste CPU cycles to generate 98 extra commits when 2 would do, etc. Thanks, Taylor
diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c index 346fb29513..923f793cec 100644 --- a/pack-bitmap-write.c +++ b/pack-bitmap-write.c @@ -694,6 +694,10 @@ void bitmap_writer_select_commits(struct bitmap_writer *writer, if (indexed_commits_nr < 100) { for (i = 0; i < indexed_commits_nr; ++i) bitmap_writer_push_commit(writer, indexed_commits[i], 0); + + select_pseudo_merges(writer, indexed_commits, + indexed_commits_nr); + return; }
Ordinarily, the pack-bitmap machinery will select some subset of reachable commits to receive bitmaps. But when there are fewer than 100 commits indexed in the first place, they will all receive bitmaps as a special case. When this happens, pseudo-merges are not generated, making it impossible to test pseudo-merge corner cases with fewer than 100 commits. Select pseudo-merges even for bitmaps with fewer than 100 commits to make such testing easier. In practice, this should not make a difference to non-testing bitmaps, as they are unlikely to be used when a repository has so few commits to begin with. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- pack-bitmap-write.c | 4 ++++ 1 file changed, 4 insertions(+)