Re: [PATCH] repack: respect gc.pid lock

From: Jacob Keller <jacob.keller@gmail.com>
Date: Apr 14, 2017, 00:33 UTC
Message-ID: <CA+P7+xomqvK=E0A_wPiyufzyF63yRLA=CQS3Sfec_Uub72DKrw@mail.gmail.com>
In-Reply-To: <20170413202712.22192-1-dturner@twosigma.com>

On Thu, Apr 13, 2017 at 1:27 PM, David Turner <dturner@twosigma.com> wrote:

Show 19 quoted lines

> 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
> solution.
>

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.

Show 26 quoted lines

> Signed-off-by: David Turner <dturner@twosigma.com>
> ---
>  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).
>
> +--no-lock::
> +       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.