From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS31976 209.132.180.0/23 X-Spam-Status: No, score=-4.0 required=3.0 tests=AWL,BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,RCVD_IN_DNSWL_HI shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by dcvr.yhbt.net (Postfix) with ESMTP id 784C820248 for ; Tue, 12 Mar 2019 10:50:21 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726109AbfCLKt5 (ORCPT ); Tue, 12 Mar 2019 06:49:57 -0400 Received: from cloud.peff.net ([104.130.231.41]:47024 "HELO cloud.peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1725811AbfCLKt4 (ORCPT ); Tue, 12 Mar 2019 06:49:56 -0400 Received: (qmail 25580 invoked by uid 109); 12 Mar 2019 10:49:56 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with SMTP; Tue, 12 Mar 2019 10:49:56 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 31283 invoked by uid 111); 12 Mar 2019 10:50:15 -0000 Received: from sigill.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.7) by peff.net (qpsmtpd/0.94) with (ECDHE-RSA-AES256-GCM-SHA384 encrypted) SMTP; Tue, 12 Mar 2019 06:50:15 -0400 Authentication-Results: peff.net; auth=none Received: by sigill.intra.peff.net (sSMTP sendmail emulation); Tue, 12 Mar 2019 06:49:54 -0400 Date: Tue, 12 Mar 2019 06:49:54 -0400 From: Jeff King To: Eric Wong Cc: git@vger.kernel.org Subject: Re: [PATCH] repack: enable bitmaps by default on bare repos Message-ID: <20190312104954.GA2023@sigill.intra.peff.net> References: <20190214043127.GA19019@sigill.intra.peff.net> <20190214043743.GB19183@sigill.intra.peff.net> <20190309024944.zcbwgvn52jsw2a2e@dcvr> <20190310233956.GB3059@sigill.intra.peff.net> <20190312031303.5tutut7zzvxne5dw@dcvr> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20190312031303.5tutut7zzvxne5dw@dcvr> Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org On Tue, Mar 12, 2019 at 03:13:03AM +0000, Eric Wong wrote: > > I do think they're a net win for people hosting git servers. But if > > that's the goal, I think at most you'd want to make bitmaps the default > > for bare repos. They're really not much help for normal end-user repos > > at this point. > > Fair enough, hopefully this can make life easier for admins > new to hosting git: > > ----------8<--------- > Subject: [PATCH] repack: enable bitmaps by default on bare repos > > A typical use case for bare repos is for serving clones and > fetches to clients. Enable bitmaps by default on bare repos to > make it easier for admins to host git repos in a performant way. OK. I still think of bitmaps as something that might need manual care and feeding, but I think that may be leftover superstition. I can't offhand think of any real downsides to this. > static int delta_base_offset = 1; > static int pack_kept_objects = -1; > -static int write_bitmaps; > +static int write_bitmaps = -1; So we'll have "-1" be "not decided yet". Makes sense. > @@ -343,11 +343,15 @@ 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 (!(pack_everything & ALL_INTO_ONE)) { > + if (write_bitmaps > 0) > + die(_(incremental_bitmap_conflict_error)); > + } else if (write_bitmaps < 0) { > + write_bitmaps = is_bare_repository(); > + } Might it be easier here to always resolve "-1" into a 0/1? I.e., like: if (write_bitmaps < 0) write_bitmaps = (pack_everything & ALL_INTO_ONE) && is_bare_repository(); and then the rest of the logic can stay the same, and does not need to be modified to handle "write_bitmaps < 0"? > +test_expect_success 'bitmaps are created by default in bare repos' ' > + git clone --bare .git bare.git && > + cd bare.git && Please don't "cd" outside of a subshell, since it impacts further tests that are added. > + mkdir old && > + mv objects/pack/* old && > + pack=$(ls old/*.pack) && Are we sure we have just done $pack here? Our repo came from a local-disk clone, which would have just hard-linked whatever was in the source repo. So we're subtly relying on the state that other tests have left. I'm not sure what we're trying to accomplish with this unpacking, though. Running "git repack -ad" should generate bitmaps whether the objects were already in a single pack or not. So I think this test can just be: git clone --bare . bare.git && git -C bare.git repack -ad && bitmap=$(ls objects/pack/*.bitmap) test_path_is_file "$bitmap" I do agree with Ævar it might also be worth testing that disabling bitmaps explicitly still works. And also that repacking _without_ "-a" (i.e., an incremental) does not complain about being unable to generate bitmaps. -Peff