Eric Wong <e@xxxxxxxxx> writes: > Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> wrote: >> I have the feedback I posted before this patch in >> https://public-inbox.org/git/874l4f8h4c.fsf@xxxxxxxxxxxxxxxxxxx/ >> >> In particular "b" there since "a" is clearly more work. I.e. shouldn't >> we at least in interactive mode on a "gc" print something about skipping >> what we'd otherwise do. >> >> Maybe that's tricky with the gc.log functionality, but I think we should >> at least document this before the next guy shows up with "sometimes my >> .bitmap files aren't generated...". > > I'm not sure if the warning should be present by default; > because we'll silently stop using bitmaps, now. But warning > if '-b' is specified seems right: Hmph... write_bitmaps can come either from the command line or from the configuration. When repack.writebitmaps configuration is set, and some .keep files exist, the user probably is not even aware of doing something that is unsupported. The keep_pack_list will not be empty only if the "--keep-pack" option is explicitly given from the command line, so making it and an explicit "-b" on the command line a condition to trigger the warning (or even "fatal: can't do both at the same time") is safe enough, I think. But anything more than that, I am not so sure. Also, is it safe to unconditionally emit strings to the standard error stream from the gc codepath these days? I recall that once we had trouble with the logic to skip repeatedly failing "gc --auto" vaguely, but maybe I am remembering something else. Thanks. > -------8<---------- > Subject: [PATCH] repack: warn if bitmaps are explicitly enabled with keep > files > > If a user explicitly enables bitmaps, we should warn if .keep > files exist or are specified via --keep-pack > > Signed-off-by: Eric Wong <e@xxxxxxxxx> > --- > builtin/repack.c | 8 ++++++++ > t/t7700-repack.sh | 16 ++++++++++++++++ > 2 files changed, 24 insertions(+) > > diff --git a/builtin/repack.c b/builtin/repack.c > index 73250b2431..b1eeee88a7 100644 > --- a/builtin/repack.c > +++ b/builtin/repack.c > @@ -359,7 +359,15 @@ int cmd_repack(int argc, const char **argv, const char *prefix) > is_bare_repository() && > keep_pack_list.nr == 0 && > !has_pack_keep_file(); > + } else if (write_bitmaps > 0) { > + if (keep_pack_list.nr) > + fprintf(stderr, > + _("WARNING: --keep-pack is incompatible with bitmaps\n")); > + if (has_pack_keep_file()) > + fprintf(stderr, > + _("WARNING: .keep files are incompatible with bitmaps\n")); > } > + > if (pack_kept_objects < 0) > pack_kept_objects = write_bitmaps; > > diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh > index 0e9af832c9..839484c7dc 100755 > --- a/t/t7700-repack.sh > +++ b/t/t7700-repack.sh > @@ -249,4 +249,20 @@ test_expect_success 'no bitmaps created if .keep files present' ' > test_must_be_empty actual > ' > > +test_expect_success '-b warns with .keep files present' ' > + pack=$(ls bare.git/objects/pack/*.pack) && > + test_path_is_file "$pack" && > + keep=${pack%.pack}.keep && > + >"$keep" && > + git -C bare.git repack -adb 2>err && > + test_i18ngrep -F ".keep files are incompatible" err && > + rm -f "$keep" > +' > + > +test_expect_success '-b warns with --keep-pack specified' ' > + keep=$(cd bare.git/objects/pack/ && ls *.pack) && > + git -C bare.git repack -adb --keep-pack="$keep" 2>err && > + test_i18ngrep -F "keep-pack is incompatible" err > +' > + > test_done
