| Message ID | 20190731054055.GC16941@sigill.intra.peff.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | handling warnings due to auto-enabled bitmaps | expand |
Jeff King <peff@peff.net> writes: > Commit 7328482253 (repack: disable bitmaps-by-default if .keep files > exist, 2019-06-29) taught repack to prefer disabling bitmaps to > duplicating objects (unless bitmaps were asked for explicitly). > > But there's an easier way to do this: if we keep passing the > --honor-pack-keep flag to pack-objects when auto-enabling bitmaps, then > pack-objects already makes the same decision (it will disable bitmaps > rather than duplicate). Better still, pack-objects can actually decide > to do so based not just on the presence of a .keep file, but on whether > that .keep file actually impacts the new pack we're making (so if we're > racing with a push or fetch, for example, their temporary .keep file > will not block us from generating bitmaps if they haven't yet updated > their refs). > > And because repack uses the --write-bitmap-index-quiet flag, we don't > have to worry about pack-objects generating confusing warnings when it > does see a .keep file. We can confirm this by tweaking the .keep test to > check repack's stderr. This change is a bit too dense so I'll need to think about it a bit longer, but in the meantime it is queued alongside the other two. Thanks. > > Signed-off-by: Jeff King <peff@peff.net> > --- > builtin/repack.c | 17 ++--------------- > t/t7700-repack.sh | 3 ++- > 2 files changed, 4 insertions(+), 16 deletions(-) > > diff --git a/builtin/repack.c b/builtin/repack.c > index db93ca3660..632c0c0a79 100644 > --- a/builtin/repack.c > +++ b/builtin/repack.c > @@ -89,17 +89,6 @@ static void remove_pack_on_signal(int signo) > raise(signo); > } > > -static int has_pack_keep_file(void) > -{ > - struct packed_git *p; > - > - for (p = get_all_packs(the_repository); p; p = p->next) { > - if (p->pack_keep) > - return 1; > - } > - return 0; > -} > - > /* > * Adds all packs hex strings to the fname list, which do not > * have a corresponding .keep file. These packs are not to > @@ -346,13 +335,11 @@ int cmd_repack(int argc, const char **argv, const char *prefix) > > if (write_bitmaps < 0) { > if (!(pack_everything & ALL_INTO_ONE) || > - !is_bare_repository() || > - keep_pack_list.nr != 0 || > - has_pack_keep_file()) > + !is_bare_repository()) > write_bitmaps = 0; > } > if (pack_kept_objects < 0) > - pack_kept_objects = !!write_bitmaps; > + pack_kept_objects = write_bitmaps > 0; > > if (write_bitmaps && !(pack_everything & ALL_INTO_ONE)) > die(_(incremental_bitmap_conflict_error)); > diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh > index 54f815b8b9..4e855bc21b 100755 > --- a/t/t7700-repack.sh > +++ b/t/t7700-repack.sh > @@ -245,7 +245,8 @@ test_expect_success 'no bitmaps created if .keep files present' ' > keep=${pack%.pack}.keep && > test_when_finished "rm -f \"\$keep\"" && > >"$keep" && > - git -C bare.git repack -ad && > + git -C bare.git repack -ad 2>stderr && > + test_must_be_empty stderr && > find bare.git/objects/pack/ -type f -name "*.bitmap" >actual && > test_must_be_empty actual > '
diff --git a/builtin/repack.c b/builtin/repack.c index db93ca3660..632c0c0a79 100644 --- a/builtin/repack.c +++ b/builtin/repack.c @@ -89,17 +89,6 @@ static void remove_pack_on_signal(int signo) raise(signo); } -static int has_pack_keep_file(void) -{ - struct packed_git *p; - - for (p = get_all_packs(the_repository); p; p = p->next) { - if (p->pack_keep) - return 1; - } - return 0; -} - /* * Adds all packs hex strings to the fname list, which do not * have a corresponding .keep file. These packs are not to @@ -346,13 +335,11 @@ int cmd_repack(int argc, const char **argv, const char *prefix) if (write_bitmaps < 0) { if (!(pack_everything & ALL_INTO_ONE) || - !is_bare_repository() || - keep_pack_list.nr != 0 || - has_pack_keep_file()) + !is_bare_repository()) write_bitmaps = 0; } if (pack_kept_objects < 0) - pack_kept_objects = !!write_bitmaps; + pack_kept_objects = write_bitmaps > 0; if (write_bitmaps && !(pack_everything & ALL_INTO_ONE)) die(_(incremental_bitmap_conflict_error)); diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh index 54f815b8b9..4e855bc21b 100755 --- a/t/t7700-repack.sh +++ b/t/t7700-repack.sh @@ -245,7 +245,8 @@ test_expect_success 'no bitmaps created if .keep files present' ' keep=${pack%.pack}.keep && test_when_finished "rm -f \"\$keep\"" && >"$keep" && - git -C bare.git repack -ad && + git -C bare.git repack -ad 2>stderr && + test_must_be_empty stderr && find bare.git/objects/pack/ -type f -name "*.bitmap" >actual && test_must_be_empty actual '
Commit 7328482253 (repack: disable bitmaps-by-default if .keep files exist, 2019-06-29) taught repack to prefer disabling bitmaps to duplicating objects (unless bitmaps were asked for explicitly). But there's an easier way to do this: if we keep passing the --honor-pack-keep flag to pack-objects when auto-enabling bitmaps, then pack-objects already makes the same decision (it will disable bitmaps rather than duplicate). Better still, pack-objects can actually decide to do so based not just on the presence of a .keep file, but on whether that .keep file actually impacts the new pack we're making (so if we're racing with a push or fetch, for example, their temporary .keep file will not block us from generating bitmaps if they haven't yet updated their refs). And because repack uses the --write-bitmap-index-quiet flag, we don't have to worry about pack-objects generating confusing warnings when it does see a .keep file. We can confirm this by tweaking the .keep test to check repack's stderr. Signed-off-by: Jeff King <peff@peff.net> --- builtin/repack.c | 17 ++--------------- t/t7700-repack.sh | 3 ++- 2 files changed, 4 insertions(+), 16 deletions(-)