Re: [PATCH 1/2] prepare_packed_git(): refactor garbage reporting in pack directory

On Wed, Dec 30, 2015 at 1:37 AM, Jeff King [off-list ref] wrote:

On Wed, Nov 04, 2015 at 02:08:21PM -0600, Doug Kelly wrote:

quoted

On Wed, Nov 4, 2015 at 2:02 PM, Jeff King [off-list ref] wrote:

quoted

Definitely cleaning up the .bitmap is sane and not racy (it's in the
same boat as the .idx, I think).

.keep files are more tricky. I'd have to go over the receive-pack code
to confirm, but I think they _are_ racy. That is, receive-pack will
create them as a lockfile before moving the pack into place. That's OK,
though, if we use mtimes to give ourselves a grace period (I haven't
looked at your series yet).

But moreover, .keep files can be created manually by the user. If the
pack they referenced goes away, they are not really serving any purpose.
But it's possible that the user would want to salvage the content of the
file, or know that it was there.

So I'd argue we should leave them. Or at least leave ones that do not
have the generic "{receive,fetch}-pack $pid on $host comment in them,
which were clearly created as lockfiles.

Currently there's no mtime-guarding logic (I dug up that conversation
earlier, though, but after I'd done the respin on this series)... OK,
in that case, I'll create a separate patch that tests/cleans up
.bitmap, but doesn't touch .keep.  This might be a small series since
I think the logic for finding pack garbage doesn't know anything about
.bitmap per-se, so it's looking like I'll extend that relevant code,
before adding the handling in gc and appropriate tests.

I happened to be looking over your series again, and I noticed that we
didn't end up with any mtime logic at all in what got merged.

I _think_ that is probably OK, because we always write the pack,
followed by the .idx, followed by the .bitmap (if any). And we don't
drop .keep files (though I think we would perhaps note them as possible
cruft?).

So I don't think there are any races introduced here, but I wonder if we
want to be a bit more conservative. Sorry to bring this up so much after
the fact; I completely forgot about it when reviewing the patches.

These changes are slated for the v2.7 release. Like I said, I don't
think it's buggy, so we don't necessarily need to address it before the
release. We could add an mtime check in the next cycle as a
belt-and-suspenders safety, rather than a fix.

-Peff

Yeah, I know I never got to adding the mtime logic, but for a simple (naive,
hard-coded) case, I did come up with a basic patch today.  I think this could
be extended to a configuration option(?) which would allow a default longer
than 10 seconds (an hour? a day?), then during the regression tests, we
could provide a shorter timeout to ensure the guarding both works and also
not wait forever for tests to complete.  Thoughts?

---
 builtin/gc.c     | 14 ++++++++++++--
 t/t5304-prune.sh |  2 ++
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 79e9886..a4ce616 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c

@@ -51,8 +51,18 @@ static struct string_list pack_garbage =

STRING_LIST_INIT_DUP;
 static void clean_pack_garbage(void)
 {
  int i;
- for (i = 0; i < pack_garbage.nr; i++)
- unlink_or_warn(pack_garbage.items[i].string);
+ /* Define a cutoff time for "new" garbage to prevent race conditions */
+ time_t cutoff = time(NULL) - 10;
+ for (i = 0; i < pack_garbage.nr; i++) {
+ struct stat s;
+ char *garbage = pack_garbage.items[i].string;
+ if (!stat(garbage, &s)) {
+ if (s.st_mtime < cutoff)
+ unlink_or_warn(garbage);
+ } else
+ fprintf(stderr, _("stat failed on pack garbage: %s"),
+ garbage);
+ }
  string_list_clear(&pack_garbage, 0);
 }

diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh
index cbcc0c0..7b4650f 100755
--- a/t/t5304-prune.sh
+++ b/t/t5304-prune.sh

@@ -272,6 +272,7 @@ test_expect_success 'clean pack garbage with gc' '
  : >.git/objects/pack/fake6.keep &&
  : >.git/objects/pack/fake6.bitmap &&
  : >.git/objects/pack/fake6.idx &&
+ sleep 10 &&
  git gc &&
  git count-objects -v 2>stderr &&
  grep "^warning:" stderr | sort >actual &&

@@ -291,6 +292,7 @@ test_expect_success 'ensure unknown garbage kept with gc' '
  : >.git/objects/pack/foo.keep &&
  : >.git/objects/pack/fake.pack &&
  : >.git/objects/pack/fake2.foo &&
+ sleep 10 &&
  git gc &&
  git count-objects -v 2>stderr &&
  grep "^warning:" stderr | sort >actual &&

-- 
2.6.1