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

[Patrick Steinhardt <ps@pks.im>]

On Wed, Dec 17, 2025 at 06:59:26PM +0000, Paulo Casaretto via GitGitGadget wrote:
> 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