Re: [PATCH v2] lockfile: add PID file for debugging stale locks

From: Patrick Steinhardt <ps@pks.im>
Date: Dec 18, 2025, 08:07 UTC
Message-ID: <aUO2Pb5JQ678ELCf@pks.im>
In-Reply-To: <pull.2011.v2.git.1765997966593.gitgitgadget@gmail.com>

On Wed, Dec 17, 2025 at 06:59:26PM +0000, Paulo Casaretto via GitGitGadget wrote:

Show 16 quoted lines

> diff --git a/lockfile.c b/lockfile.c
> index 1d5ed01682..4ee215374a 100644
> --- a/lockfile.c
> +++ b/lockfile.c
> @@ -71,19 +74,117 @@ static void resolve_symlink(struct strbuf *path)
>  	strbuf_reset(&link);
>  }
>  
> +/*
> + * Lock PID file functions - write PID to a foo-pid.lock file alongside
> + * the lock file for debugging stale locks. The PID file is registered
> + * as a tempfile so it gets cleaned up by signal/atexit handlers.
> + *
> + * Naming: For "foo.lock", the PID file is "foo-pid.lock" (not "foo.lock.pid").
> + * This avoids collision with the refs namespace.
> + */

This neatly solves the issue that alternative implementations of Git wouldn't know to handle this new PID file. They already ignore every file that ends with ".lock", so of course they would also ignore these new PID files.

But unfortunately this doesn't solve the other issue, which is that we now have a new restriction: you now cannot have two references "foo" and "foo-pid" and modify them in the same transaction. Granted, this is a very specific situation, and I doubt that 99.9% of all users would ever hit this restriction. But regardless of that I'm worried about the 0.1% that _do_ hit this restriction now. So I'm afraid that this proposed naming schema isn't going to work, which raises the question whether there are any other alternative naming schemas that could.

We could (I'm not saying we should!) do some gross stuff here. For example, refnames are not allowed to contain some specific characters in their names: ":?[\\^~ \t*" are all characters that are forbidden to exist in a refname. So in theory, we could call the lockfile for example "foo:pid.lock":

  - We know that no reference "foo:pid" should exist because it contains
    a forbidden character.

  - We know that all alternative implementations should ignore it due to
    the ".lock" suffix.

Note the "should" in both cases. I'd consider any implementation that doesn't honor these "shoulds" to be buggy, but that doesn't mean that there are no buggy implementations.

In any case though, this may be a possible way forward if we really want to also cover loose references. Whether we should is a different question though, and I'm not too sure about it myself:

  - It will regress performance of the "files" reference backend as
    every ref update now needs to write two files instead of one.
    Writing many references is already the worst-case scenario for this
    backend because each reference requires a separate file, and we make
    it even worse by making it two files.

  - We now have not only one file that would block future updates, but
    two. This increases the likelihood that a crash or forced shutdown
    will leave behind stale garbage.

That being said, I also see the potential upside, which is that it now becomes a bit easier to recover from crashes/forced shutdowns. But in any case I think that this needs to be opt-in, not opt-out.

Patrick