Re: [PATCH] repack: respect gc.pid lock
- Date: Thu, 13 Apr 2017 17:33:55 -0700
- From: Jacob Keller <jacob.keller@xxxxxxxxx>
- Subject: Re: [PATCH] repack: respect gc.pid lock
On Thu, Apr 13, 2017 at 1:27 PM, David Turner <dturner@xxxxxxxxxxxx> wrote:
> Git gc locks the repository (using a gc.pid file) so that other gcs
> don't run concurrently. Make git repack respect this lock.
> Now repack, by default, will refuse to run at the same time as a gc.
> This fixes a concurrency issue: a repack which deleted packs would
> make a concurrent gc sad when its packs were deleted out from under
> it. The gc would fail with: "fatal: ./objects/pack/pack-$sha.pack
> cannot be accessed". Then it would die, probably leaving a large temp
> pack hanging around.
> Git repack learns --no-lock, so that when run under git gc, it doesn't
> attempt to manage the lock itself.
> Martin Fick suggested just moving the lock into git repack, but this
> would leave parts of git gc (e.g. git prune) protected by only local
> locks. I worried that a prune (part of git gc) concurrent with a
> repack could confuse the repack, so I decided to go with this
The last paragraph could be reworded to be a bit less personal and
more as a direct statement of why moving the lock entirely to repack
is a bad idea.
> Signed-off-by: David Turner <dturner@xxxxxxxxxxxx>
> Documentation/git-repack.txt | 5 +++
> Makefile | 1 +
> builtin/gc.c | 72 ++----------------------------------
> builtin/repack.c | 13 +++++++
> repack.c | 88 ++++++++++++++++++++++++++++++++++++++++++++
> repack.h | 8 ++++
> t/t7700-repack.sh | 8 ++++
> 7 files changed, 127 insertions(+), 68 deletions(-)
> create mode 100644 repack.c
> create mode 100644 repack.h
> diff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt
> index 26afe6ed54..b347ff5c62 100644
> --- a/Documentation/git-repack.txt
> +++ b/Documentation/git-repack.txt
> @@ -143,6 +143,11 @@ other objects in that pack they already have locally.
> being removed. In addition, any unreachable loose objects will
> be packed (and their loose counterparts removed).
> + Do not lock the repository, and do not respect any existing lock.
> + Mostly useful for running repack within git gc. Do not use this
> + unless you know what you are doing.
I would have phrased this more like:
Used internally by git gc to call git repack while already holding
the lock. Do not use unless you know what you're doing.