git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Eric Wong <e@80x24.org>
To: Jeff King <peff@peff.net>
Cc: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Janos Farkas" <chexum@gmail.com>,
	git@vger.kernel.org
Subject: [PATCH] repack: disable bitmaps-by-default if .keep files exist
Date: Fri, 28 Jun 2019 07:02:11 +0000	[thread overview]
Message-ID: <20190628070211.hfweqcons6c6gy52@dcvr> (raw)
In-Reply-To: <20190623224244.GB1100@sigill.intra.peff.net>

Jeff King <peff@peff.net> 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.  <snip>

--------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 <e@80x24.org>
Helped-by: Jeff King <peff@peff.net>
Reported-by: Janos Farkas <chexum@gmail.com>
---
 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
-- 
EW

  parent reply	other threads:[~2019-06-28  7:02 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-23 12:15 2.22.0 repack -a duplicating pack contents Janos Farkas
2019-06-23 14:54 ` Ævar Arnfjörð Bjarmason
2019-06-23 15:38   ` Janos Farkas
2019-06-23 18:02   ` Jeff King
2019-06-23 18:08     ` Eric Wong
2019-06-23 22:42       ` Jeff King
2019-06-24  9:30         ` Ævar Arnfjörð Bjarmason
2019-07-03 17:40           ` Jeff King
2019-06-28  7:02         ` Eric Wong [this message]
2019-06-28  7:21           ` [PATCH] repack: disable bitmaps-by-default if .keep files exist Ævar Arnfjörð Bjarmason
2019-06-29 19:16             ` [PATCH 2/1] repack: warn if bitmaps are explicitly enabled with keep files Eric Wong
2019-07-01 18:15               ` Junio C Hamano
2019-07-03 17:38                 ` Jeff King
2019-07-03 18:10                   ` Junio C Hamano
2019-07-03 18:37                     ` Junio C Hamano
2019-07-03 21:24                       ` Jeff King
2019-07-03 21:23                     ` Jeff King
2019-07-08 17:40                       ` Junio C Hamano
2019-06-29  8:03           ` [PATCH] repack: disable bitmaps-by-default if .keep files exist SZEDER Gábor
2019-06-29 19:13             ` [PATCH v2] " Eric Wong

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190628070211.hfweqcons6c6gy52@dcvr \
    --to=e@80x24.org \
    --cc=avarab@gmail.com \
    --cc=chexum@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).