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

Doug Kelly [off-list ref] writes:

I think the patches I sent (a bit prematurely) address the
remaining comments... I did find there was a relevant test in
t5304 already, so I added a new test in the same section (and
cleaned up some of the garbage it wasn't removing before).  I'm
not sure if it's poor form to move tests around like this, but I
figured it might be best to keep them logically grouped.

OK, will queue as I didn't spot anything glaringly wrong ;-)

I did wonder if we want to say anything about .bitmap files, though.
If there is one without matching .idx and .pack, shouldn't we report
just like we report .idx without .pack (or vice versa)?

Thanks.

On Wed, Nov 4, 2015 at 1:35 PM, Junio C Hamano [off-list ref] wrote:

Doug Kelly [off-list ref] writes:

quoted

I think the patches I sent (a bit prematurely) address the
remaining comments... I did find there was a relevant test in
t5304 already, so I added a new test in the same section (and
cleaned up some of the garbage it wasn't removing before).  I'm
not sure if it's poor form to move tests around like this, but I
figured it might be best to keep them logically grouped.

OK, will queue as I didn't spot anything glaringly wrong ;-)

I did wonder if we want to say anything about .bitmap files, though.
If there is one without matching .idx and .pack, shouldn't we report
just like we report .idx without .pack (or vice versa)?

Thanks.

I think you're right -- this would be something worth following up on.
At least, t5304 doesn't cover this case explicitly, but when I tried
adding an empty bitmap with a bogus name, I did see a "no
corresponding .idx or .pack" error, similar to the stale .keep file.

I'd trust your (and Jeff's) knowledge on this far more than my own,
but would it be a bad idea to clean up .keep and .bitmap files if the
.idx/.pack pair are missing?  I think we may have had a discussion
previously on how things along these lines might be racey -- but I
don't know what order the .keep file is created in relation to the
.idx/.pack.

On Wed, Nov 04, 2015 at 11:35:52AM -0800, Junio C Hamano wrote:

Doug Kelly [off-list ref] writes:

quoted

I think the patches I sent (a bit prematurely) address the
remaining comments... I did find there was a relevant test in
t5304 already, so I added a new test in the same section (and
cleaned up some of the garbage it wasn't removing before).  I'm
not sure if it's poor form to move tests around like this, but I
figured it might be best to keep them logically grouped.

OK, will queue as I didn't spot anything glaringly wrong ;-)

I did wonder if we want to say anything about .bitmap files, though.
If there is one without matching .idx and .pack, shouldn't we report
just like we report .idx without .pack (or vice versa)?

Yeah, I think so. The logic should really extend to anything without a
matching .pack. And I think the sane rule is probably:

  If we have pack-$sha.$ext, but not pack-$sha.pack, then:

    1. if $ext is known to us as a cache that can be regenerated from the
       .pack (i.e., .idx, .bitmap), then delete it

    2. if $ext is known to us as precious, do nothing (there is nothing
       in this category right now, though)

    3. if $ext is not known to us, warn but do not delete (in case a
       future version adds something precious)

The conservatism in (3) is the right thing to do, I think, but I doubt
it will ever matter, because we probably cannot ever add non-cache
auxiliary files to the pack. Old versions of git would not delete such
precious files, but nor would they carry them forward during a repack.
So short of a repo-version bump, I think we are effectively limited to
adding only caches which can be re-generated from an original .pack.

-Peff

On Wed, Nov 04, 2015 at 01:56:38PM -0600, Doug Kelly wrote:

quoted

I did wonder if we want to say anything about .bitmap files, though.
If there is one without matching .idx and .pack, shouldn't we report
just like we report .idx without .pack (or vice versa)?

I think you're right -- this would be something worth following up on.
At least, t5304 doesn't cover this case explicitly, but when I tried
adding an empty bitmap with a bogus name, I did see a "no
corresponding .idx or .pack" error, similar to the stale .keep file.

Yeah, that should be harmless warning (although note because the bitmap
code only really handles a single bitmap, it can prevent loading of the
"real" bitmap; so you'd want to clean it up, for sure).

I'd trust your (and Jeff's) knowledge on this far more than my own,
but would it be a bad idea to clean up .keep and .bitmap files if the
.idx/.pack pair are missing?  I think we may have had a discussion
previously on how things along these lines might be racey -- but I
don't know what order the .keep file is created in relation to the
.idx/.pack.

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.

-Peff

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

On Wed, Nov 04, 2015 at 01:56:38PM -0600, Doug Kelly wrote:

quoted

quoted

I did wonder if we want to say anything about .bitmap files, though.
If there is one without matching .idx and .pack, shouldn't we report
just like we report .idx without .pack (or vice versa)?

I think you're right -- this would be something worth following up on.
At least, t5304 doesn't cover this case explicitly, but when I tried
adding an empty bitmap with a bogus name, I did see a "no
corresponding .idx or .pack" error, similar to the stale .keep file.

Yeah, that should be harmless warning (although note because the bitmap
code only really handles a single bitmap, it can prevent loading of the
"real" bitmap; so you'd want to clean it up, for sure).

quoted

I'd trust your (and Jeff's) knowledge on this far more than my own,
but would it be a bad idea to clean up .keep and .bitmap files if the
.idx/.pack pair are missing?  I think we may have had a discussion
previously on how things along these lines might be racey -- but I
don't know what order the .keep file is created in relation to the
.idx/.pack.

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.

-Peff

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.

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

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'd hoped you could reuse the list of extensions found in
builtin/repack.c (e.g., see remove_redundant_pack). But I guess that is
not connected with the garbage-reporting code. And anyway, the simple
list probably does not carry sufficient information (it does not know
that ".keep" is potentially more precious than ".idx", for example).

-Peff

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

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

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