From patchwork Fri Jun 28 07:02:11 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Eric Wong X-Patchwork-Id: 11021481 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 1D69614C0 for ; Fri, 28 Jun 2019 07:02:15 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 0C23C28468 for ; Fri, 28 Jun 2019 07:02:15 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id F1EFB28723; Fri, 28 Jun 2019 07:02:14 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.9 required=2.0 tests=BAYES_00,MAILING_LIST_MULTI, RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 12A9128468 for ; Fri, 28 Jun 2019 07:02:14 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727247AbfF1HCN (ORCPT ); Fri, 28 Jun 2019 03:02:13 -0400 Received: from dcvr.yhbt.net ([64.71.152.64]:55218 "EHLO dcvr.yhbt.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726749AbfF1HCM (ORCPT ); Fri, 28 Jun 2019 03:02:12 -0400 Received: from localhost (dcvr.yhbt.net [127.0.0.1]) by dcvr.yhbt.net (Postfix) with ESMTP id CC58C1F461; Fri, 28 Jun 2019 07:02:11 +0000 (UTC) Date: Fri, 28 Jun 2019 07:02:11 +0000 From: Eric Wong To: Jeff King Cc: =?utf-8?b?w4Z2YXIgQXJuZmrDtnLDsA==?= Bjarmason , Janos Farkas , git@vger.kernel.org Subject: [PATCH] repack: disable bitmaps-by-default if .keep files exist Message-ID: <20190628070211.hfweqcons6c6gy52@dcvr> References: <875zow8i85.fsf@evledraar.gmail.com> <20190623180226.GA1100@sigill.intra.peff.net> <20190623180825.3ospajjgat3clwiu@dcvr> <20190623224244.GB1100@sigill.intra.peff.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20190623224244.GB1100@sigill.intra.peff.net> Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Jeff King wrote: > On Sun, Jun 23, 2019 at 06:08:25PM +0000, Eric Wong wrote: > > > > I'm not sure of the right solution. For maximal backwards-compatibility, > > > the default for bitmaps could become "if not bare and if there are no > > > .keep files". But that would mean bitmaps sometimes not getting > > > generated because of the problems that ee34a2bead was trying to solve. > > > > > > That's probably OK, though; you can always flip the bitmap config to > > > "true" yourself if you _must_ have bitmaps. > > > > What about something like this? Needs tests but I need to leave, now. > > Yeah, I think that's the right direction. OK. I have a real patch with one additional test, below. (don't have a lot of time for hacking) > Though... > > > +static int has_pack_keep_file(void) > > +{ > > + DIR *dir; > > + struct dirent *e; > > + int found = 0; > > + > > + if (!(dir = opendir(packdir))) > > + return found; > > + > > + while ((e = readdir(dir)) != NULL) { > > + if (ends_with(e->d_name, ".keep")) { > > + found = 1; > > + break; > > + } > > + } > > + closedir(dir); > > + return found; > > +} > > I think this can be replaced with just checking p->pack_keep for each > item in the packed_git list. Good point, I tend to forget git C API internals as soon as I learn them :x > That's racy, but then so is your code here, since it's really the child > pack-objects which is going to deal with the .keep. I don't think we > need to care much about the race, though. Either: Agreed. --------8<------- Subject: [PATCH] repack: disable bitmaps-by-default if .keep files exist Bitmaps aren't useful with multiple packs, and users with .keep files ended up with redundant packs when bitmaps got enabled by default in bare repos. So detect when .keep files exist and stop enabling bitmaps by default in that case. Wasteful (but otherwise harmless) race conditions with .keep files documented by Jeff King still apply and there's a chance we'd still end up with redundant data on the FS: https://public-inbox.org/git/20190623224244.GB1100@sigill.intra.peff.net/ Fixes: 36eba0323d3288a8 ("repack: enable bitmaps by default on bare repos") Signed-off-by: Eric Wong Helped-by: Jeff King Reported-by: Janos Farkas --- builtin/repack.c | 18 ++++++++++++++++-- t/t7700-repack.sh | 10 ++++++++++ 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/builtin/repack.c b/builtin/repack.c index caca113927..a9529d1afc 100644 --- a/builtin/repack.c +++ b/builtin/repack.c @@ -89,6 +89,17 @@ static void remove_pack_on_signal(int signo) raise(signo); } +static int has_pack_keep_file(void) +{ + struct packed_git *p; + + for (p = get_packed_git(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 @@ -343,9 +354,12 @@ int cmd_repack(int argc, const char **argv, const char *prefix) (unpack_unreachable || (pack_everything & LOOSEN_UNREACHABLE))) die(_("--keep-unreachable and -A are incompatible")); - if (write_bitmaps < 0) + if (write_bitmaps < 0) { write_bitmaps = (pack_everything & ALL_INTO_ONE) && - is_bare_repository(); + is_bare_repository() && + keep_pack_list.nr == 0 && + !has_pack_keep_file(); + } if (pack_kept_objects < 0) pack_kept_objects = write_bitmaps; diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh index 86d05160a3..0acde3b1f8 100755 --- a/t/t7700-repack.sh +++ b/t/t7700-repack.sh @@ -239,4 +239,14 @@ test_expect_success 'bitmaps can be disabled on bare repos' ' test -z "$bitmap" ' +test_expect_success 'no bitmaps created if .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 -ad && + bitmap=$(ls bare.git/objects/pack/*.bitmap 2>/dev/null || :) && + test -z "$bitmap" +' + test_done From patchwork Sat Jun 29 19:16:00 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Eric Wong X-Patchwork-Id: 11024091 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id E5DA913BD for ; Sat, 29 Jun 2019 19:16:02 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id D0550287CD for ; Sat, 29 Jun 2019 19:16:02 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id BFF91287D1; Sat, 29 Jun 2019 19:16:02 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.9 required=2.0 tests=BAYES_00,MAILING_LIST_MULTI, RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 5FE7B287CD for ; Sat, 29 Jun 2019 19:16:02 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726903AbfF2TQB (ORCPT ); Sat, 29 Jun 2019 15:16:01 -0400 Received: from dcvr.yhbt.net ([64.71.152.64]:34860 "EHLO dcvr.yhbt.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726882AbfF2TQB (ORCPT ); Sat, 29 Jun 2019 15:16:01 -0400 Received: from localhost (dcvr.yhbt.net [127.0.0.1]) by dcvr.yhbt.net (Postfix) with ESMTP id BFBF51F461; Sat, 29 Jun 2019 19:16:00 +0000 (UTC) Date: Sat, 29 Jun 2019 19:16:00 +0000 From: Eric Wong To: =?utf-8?b?w4Z2YXIgQXJuZmrDtnLDsA==?= Bjarmason Cc: Jeff King , Janos Farkas , git@vger.kernel.org Subject: [PATCH 2/1] repack: warn if bitmaps are explicitly enabled with keep files Message-ID: <20190629191600.nipp2ut37xd3mx56@dcvr> References: <875zow8i85.fsf@evledraar.gmail.com> <20190623180226.GA1100@sigill.intra.peff.net> <20190623180825.3ospajjgat3clwiu@dcvr> <20190623224244.GB1100@sigill.intra.peff.net> <20190628070211.hfweqcons6c6gy52@dcvr> <87zhm26uq9.fsf@evledraar.gmail.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <87zhm26uq9.fsf@evledraar.gmail.com> Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Ævar Arnfjörð Bjarmason wrote: > I have the feedback I posted before this patch in > https://public-inbox.org/git/874l4f8h4c.fsf@evledraar.gmail.com/ > > 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: -------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 Signed-off-by: Eric Wong Reported-by: Janos Farkas Signed-off-by: Junio C Hamano --- 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