lockfile: add holder info file for debugging stale locks #2011
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
hcmaATshopify
left a comment
hcmaATshopify
left a comment
There was a problem hiding this comment.
I would argue for using pid instead of holder everywhere.
Pros of pid
- Precise: The only payload in the file is the PID. Using “pid” makes it 100% clear and avoids ambiguity.
- Conventional: Many tools use
.pidfiles or similar conventions, so the meaning is familiar. - Unambiguous Code: Variables like
pid_fileorread_lock_pid()are explicitly about PIDs, not other info.
lockfile.c
Outdated
| return NULL; | ||
|
|
||
| strbuf_addf(&holder_path, "%s%s", lock_path, LOCK_HOLDER_SUFFIX); | ||
| fd = open(holder_path.buf, O_WRONLY | O_CREAT | O_TRUNC, mode); |
There was a problem hiding this comment.
Please make sure mode is restrictive enough (match whatever the .lock has and is readable by the creator).
Author
There was a problem hiding this comment.
It does use the same mode parameter as the lock file:
// In lock_file():
lk->tempfile = create_tempfile_mode(filename.buf, mode); // lock file
lk->pid_tempfile = create_lock_pid_file(filename.buf, mode); // PID file - same mode
lockfile.c
Outdated
|
|
||
| if (lock_holder_info_enabled() && | ||
| !read_lock_holder_pid(lock_path.buf, &holder_pid)) | ||
| strbuf_addf(buf, _("Lock is held by process %"PRIuMAX".\n\n"), |
There was a problem hiding this comment.
Lock (acquired via file '%s') is being held by process .
Author
There was a problem hiding this comment.
Given this will print after the "Unable to create '/path/to/.git/index.lock': File exists." message, I think it would be redundant.
lockfile.c
Outdated
| strbuf_addf(buf, _("Lock is held by process %"PRIuMAX".\n\n"), | ||
| holder_pid); | ||
|
|
||
| strbuf_addstr(buf, _("Another git process seems to be running in this repository, e.g.\n" |
There was a problem hiding this comment.
If the feature is enabled, we can actually improve this now and be assertive (for instance, if the pid is no longer around, we can say this "lock file is stale and can be removed" or "lock file is valid and held by process "
Author
There was a problem hiding this comment.
turns out kill was already implement and cross platform. Added a liveness check!
| return 0; | ||
| } | ||
|
|
||
| int rollback_lock_file(struct lock_file *lk) |
There was a problem hiding this comment.
I recommend renaming rollback_lock_file to abort_lock_file for clarity and consistency with Git’s conventions. abort_lock_file is likely the best fit for the Git codebase, as this matches the standard “commit/abort” terminology for transactional semantics.
Author
There was a problem hiding this comment.
This function is not new, I just needed to change it from being an inline function to add new functionality.
lockfile.c
Outdated
| strbuf_addf(&holder_path, "%s%s", lock_path, LOCK_HOLDER_SUFFIX); | ||
| fd = open(holder_path.buf, O_WRONLY | O_CREAT | O_TRUNC, mode); | ||
| if (fd >= 0) { | ||
| strbuf_addf(&content, "%"PRIuMAX"\n", (uintmax_t)getpid()); |
There was a problem hiding this comment.
maybe strbuf_addf(&content, "%ld\n", (long)getpid()); since pid_t is generally int or long; cast to long for portability
Are we sure this will work for non-Unix platforms that might be supported by git? In other words, do we need to ifdef this whole thing so we don't break non-Unix builds?
Author
There was a problem hiding this comment.
Looking around the codebase it seems (uintmax_t)getpid() + PRIuMAX pattern is the established Git convention for portable PID handling:
Examples:
-
daemon.c:1458- writes PID to file:write_file(pid_file, "%"PRIuMAX, (uintmax_t) getpid());
-
builtin/gc.c:789-790- writes PID to lockfile:strbuf_addf(&sb, "%"PRIuMAX" %s", (uintmax_t) getpid(), my_host);
In terms of cross platform compatibility, I think I got lucky because it was already a first class citizen.
Did a little research and it works across the board.
- getpid() is available on all Git-supported platforms (provided by libc on Unix, emulated on Windows)
- pid_t is defined everywhere (native on Unix, typedef int on Windows via mingw-posix.h)
pcasaretto
force-pushed
the
pid-holder-file
branch
from
5452ae9 to
51dbf28
Compare
|
Thanks for doing this @pcasaretto - LGTM! |
pcasaretto
force-pushed
the
pid-holder-file
branch
from
51dbf28 to
7068ec0
Compare
Author
|
/preview |
|
Preview email sent as pull.2011.git.1764687711531.gitgitgadget@gmail.com |
Author
|
/submit |
|
Submitted as pull.2011.git.1764688047077.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, "D. Ben Knoble" wrote (reply to this): On Tue, Dec 2, 2025 at 10:07 AM Paulo Casaretto via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Paulo Casaretto <pcasaretto@gmail.com>
>
> When a lock file is held, it can be helpful to know which process owns
> it, especially when debugging stale locks left behind by crashed
> processes. Add an optional feature that creates a companion .lock.pid
> file alongside each lock file, containing the PID of the lock holder.
>
> The .lock.pid file is created when a lock is acquired (if enabled), and
> automatically cleaned up when the lock is released (via commit or
> rollback). The file is registered as a tempfile so it gets cleaned up
> by signal and atexit handlers if the process terminates abnormally.
>
> When a lock conflict occurs, the code checks if the PID from the .pid
> file is still running using kill(pid, 0). This allows providing
> context-aware error messages. With PID info enabled:
>
> Lock is held by process 12345. Wait for it to finish, or remove
> the lock file to continue.
>
> Or for a stale lock:
>
> Lock was held by process 12345, which is no longer running.
> Remove the stale lock file to continue.
>
> Without PID info (default):
>
> Another git process seems to be running in this repository.
> Wait for it to finish, or remove the lock file to continue.
>
> The feature is opt-in via GIT_LOCK_PID_INFO=1 environment variable.
>
> Signed-off-by: Paulo Casaretto <pcasaretto@gmail.com>
Sounds interesting. I think by the time I wish I knew what else was
using the lockfile, it's too late for me to alter my environment.
Perhaps (in addition to allowing the environment opt-in) we could
opt-in via configuration? Or is this really only useful, say, on the
server side where the environment is carefully controlled? I don't
relish putting this variable into my environment to take advantage of
something that looks very useful.
Are there downsides that make it necessary to be opt-in? I also
imagine this could be a useful default; occasionally folks at work hit
something similar and ask "what's up with that?"
Only other thing is: just because a process X is running doesn't mean
it was the one holding the lock, right? Since PIDs can be reused.
--
D. Ben Knoble |
|
User |
|
On the Git mailing list, Torsten Bögershausen wrote (reply to this): On Tue, Dec 02, 2025 at 03:07:27PM +0000, Paulo Casaretto via GitGitGadget wrote:
> From: Paulo Casaretto <pcasaretto@gmail.com>
>
> When a lock file is held, it can be helpful to know which process owns
> it, especially when debugging stale locks left behind by crashed
> processes. Add an optional feature that creates a companion .lock.pid
> file alongside each lock file, containing the PID of the lock holder.
>
> The .lock.pid file is created when a lock is acquired (if enabled), and
> automatically cleaned up when the lock is released (via commit or
> rollback). The file is registered as a tempfile so it gets cleaned up
> by signal and atexit handlers if the process terminates abnormally.
>
> When a lock conflict occurs, the code checks if the PID from the .pid
> file is still running using kill(pid, 0). This allows providing
> context-aware error messages. With PID info enabled:
>
> Lock is held by process 12345. Wait for it to finish, or remove
> the lock file to continue.
>
> Or for a stale lock:
>
> Lock was held by process 12345, which is no longer running.
> Remove the stale lock file to continue.
>
> Without PID info (default):
>
> Another git process seems to be running in this repository.
> Wait for it to finish, or remove the lock file to continue.
>
> The feature is opt-in via GIT_LOCK_PID_INFO=1 environment variable.
[]
I think that this makes sense.
However, as a frequent user of Git repos hosted on an NFS server
(without any problems in my setup):
Does it make sense to add the hostname here ?
We already have xgethostname() in Git, so that we can diagnose
who/which machine really left a lock.
[] |
|
User |
|
On the Git mailing list, Jeff King wrote (reply to this): On Tue, Dec 02, 2025 at 03:07:27PM +0000, Paulo Casaretto via GitGitGadget wrote:
> The .lock.pid file is created when a lock is acquired (if enabled), and
> automatically cleaned up when the lock is released (via commit or
> rollback). The file is registered as a tempfile so it gets cleaned up
> by signal and atexit handlers if the process terminates abnormally.
I'm sympathetic to the goal of this series, and the implementation looks
cleanly done. But I wonder if there might be some system-level side
effects that make these .pid files awkward.
Temporarily having an extra .git/index.lock.pid file is probably not a
big deal. But for other namespaces, like refs, we're colliding with
names that have other meanings. So if we want to update refs/heads/foo,
for example, we'll create refs/heads/foo.lock now. And after your patch,
also refs/heads/foo.lock.pid.
The ".lock" suffix is special, in that we disallow it in a refname and
know to skip it when iterating over loose refs. But for the ".pid"
variant, we run the risk of colliding with a real branch named
"foo.lock.pid", both for reading and writing.
On the writing side, creating foo.lock.pid may accidentally overwrite an
existing branch. This can be mitigated by using O_EXCL when creating the
pid file.
But we can see the writes in the opposite order, which I think can also
lead to data loss. Something like:
- process A wants to write branch "foo", so it holds
refs/heads/foo.lock and now also the matching foo.lock.pid
- process B wants to write branch "foo.lock.pid", so it holds
refs/heads/foo.lock.pid.lock (and the matching pid)
- process B finishes its write and atomically renames
foo.lock.pid.lock into foo.lock.pid. It's expected to overwrite
the existing file there, so now process A's pid file is gone.
- process A finishes its write and tries to clean up its pid file. So
it deletes foo.lock.pid, which contains the actual data from process
B's write.
And now process B's write has been lost (and maybe even the entire
existence of the foo.lock.pid branch, if it was not also present in the
packed-refs file).
On the reading side, anybody iterating refs/heads/ may racily see the
foo.lock.pid file and think it is supposed to be an actual ref. So they
open it, find it contains garbage, and then flag it as an error.
I think both could be mitigated if we disallowed ".lock.pid" as a suffix
in refnames, but that is a big user-facing change.
In the long run, alternate ref stores like reftable won't suffer from
this issue. It's possible there are other spots where we use lockfiles
alongside arbitrarily-named files, though I couldn't think of any.
So I dunno what that means for your patch. I notice that the user has to
enable the feature manually. But it feels more like it should be
selective based on which subsystem is using the lockfile (so refs would
never want it, but other lockfiles/tempfiles might).
-Peff |
|
User |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Jeff King <peff@peff.net> writes:
> So I dunno what that means for your patch. I notice that the user has to
> enable the feature manually. But it feels more like it should be
> selective based on which subsystem is using the lockfile (so refs would
> never want it, but other lockfiles/tempfiles might).
Or perhaps the way to opt into the feature is to create an empty
file $GIT_DIR/lockfile-audit, and the lockfile subsystem will append
to it every time a lock is taken? We need to ensure that a PID and
pathname formatted into a single record is small enough and O_APPEND
would relieve us from worrying about multi writer races, which may
introduce different kind of complications, though.
|
|
On the Git mailing list, Jeff King wrote (reply to this): On Wed, Dec 03, 2025 at 02:21:11PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > So I dunno what that means for your patch. I notice that the user has to
> > enable the feature manually. But it feels more like it should be
> > selective based on which subsystem is using the lockfile (so refs would
> > never want it, but other lockfiles/tempfiles might).
>
> Or perhaps the way to opt into the feature is to create an empty
> file $GIT_DIR/lockfile-audit, and the lockfile subsystem will append
> to it every time a lock is taken? We need to ensure that a PID and
> pathname formatted into a single record is small enough and O_APPEND
> would relieve us from worrying about multi writer races, which may
> introduce different kind of complications, though.
I like a single log much better from a management perspective. I agree
that atomicity is a potential issue, though. I think that even if we
kept it small, network filesystems like NFS do not provide great
guarantees for atomic appends. Something like flock() can work there,
but that's not something we've relied on before.
It also raises questions about reading (do we find pid files in the log
in order to provide more directed advice?) and maintenance (do we ever
clean it up, or just let it grow without bound?).
-Peff |
|
On the Git mailing list, Taylor Blau wrote (reply to this): On Wed, Dec 03, 2025 at 04:16:10PM -0500, Jeff King wrote:
> On Tue, Dec 02, 2025 at 03:07:27PM +0000, Paulo Casaretto via GitGitGadget wrote:
>
> > The .lock.pid file is created when a lock is acquired (if enabled), and
> > automatically cleaned up when the lock is released (via commit or
> > rollback). The file is registered as a tempfile so it gets cleaned up
> > by signal and atexit handlers if the process terminates abnormally.
>
> I'm sympathetic to the goal of this series, and the implementation looks
> cleanly done. But I wonder if there might be some system-level side
> effects that make these .pid files awkward.
>
> Temporarily having an extra .git/index.lock.pid file is probably not a
> big deal. But for other namespaces, like refs, we're colliding with
> names that have other meanings. So if we want to update refs/heads/foo,
> for example, we'll create refs/heads/foo.lock now. And after your patch,
> also refs/heads/foo.lock.pid.
>
> The ".lock" suffix is special, in that we disallow it in a refname and
> know to skip it when iterating over loose refs. But for the ".pid"
> variant, we run the risk of colliding with a real branch named
> "foo.lock.pid", both for reading and writing.
Good point. I don't have a strong opinion on whether or not we should
use an append-only log of which PIDs grabbed which lockfiles when versus
tracking them on a per-lock basis. But I wonder if this would be
mitigated by either:
- Keeping the ".lock" suffix as-is, so that holding a lockfile at path
"$GIT_DIR/index.lock" would create "$GIT_DIR/index-pid.lock" or
something similar.
- Introducing a new reference name constraint that treats ".lock.pid"
as a reserved in a manner identical to how we currently treat
".lock".
Between the two, I vastly prefer the former, but see below for more on
why.
> But we can see the writes in the opposite order, which I think can also
> lead to data loss. Something like:
>
> - process A wants to write branch "foo", so it holds
> refs/heads/foo.lock and now also the matching foo.lock.pid
>
> - process B wants to write branch "foo.lock.pid", so it holds
> refs/heads/foo.lock.pid.lock (and the matching pid)
Changing the naming scheme as above would cause us to hold
"foo.pid.lock" in addition to "foo.lock". That would allow process B
here to write branch "foo.lock.pid" (as is the case today). But if the
scenario were instead "process B wants to write branch foo.pid.lock", it
would fail immediately since the ".lock" suffix is reserved.
> I think both could be mitigated if we disallowed ".lock.pid" as a suffix
> in refnames, but that is a big user-facing change.
Yeah, I don't think that we should change the refname constraints here,
especially in a world where reftable deployments are more common. In
that world I think we should err on the side of removing constraints,
not adding them ;-).
> So I dunno what that means for your patch. I notice that the user has to
> enable the feature manually. But it feels more like it should be
> selective based on which subsystem is using the lockfile (so refs would
> never want it, but other lockfiles/tempfiles might).
Yeah, I think that something similar to the "which files do we fsync()
and how?" configuration we have today would be a nice complement here.
(As an aside, I wonder if that interface, too, could be slightly
improved. Right now we have a comma-separated list of values in the
"core.fsync" configuration for listing different "components", and then
a global core.fsyncMethod to either issue a fsync(), or a pagecache
writeback, or writeout-only flushes in batches. It might be nice to have
something like:
[fsync "loose-object"]
method = fsync
[fsync "packfile"]
method = writeout
, so the analog here would be something like:
[lockfile "refs"]
pidfile = false
[lockfile "index"]
pidfile = true
or similar. That could also be represented as core.lockfile=index,
omitting "refs" to avoid tracking it. It may be that people don't really
care to ever use different fsync methods for different fsync-able
components, so perhaps the analogy doesn't hold up perfectly.)
Thanks,
Taylor |
|
On the Git mailing list, Taylor Blau wrote (reply to this): Hi Paulo,
On Tue, Dec 02, 2025 at 03:07:27PM +0000, Paulo Casaretto via GitGitGadget wrote:
> The .lock.pid file is created when a lock is acquired (if enabled), and
> automatically cleaned up when the lock is released (via commit or
> rollback). The file is registered as a tempfile so it gets cleaned up
> by signal and atexit handlers if the process terminates abnormally.
(For the purposes of this review, I'll ignore the naming conventions
that are discussed elsewhere in the thread, which I think can be
resolved separately of any technical concerns.)
> diff --git a/Documentation/git.adoc b/Documentation/git.adoc
> index ce099e78b8..6fdd509d34 100644
> --- a/Documentation/git.adoc
> +++ b/Documentation/git.adoc
> @@ -1010,6 +1010,16 @@ be in the future).
> the background which do not want to cause lock contention with
> other operations on the repository. Defaults to `1`.
>
> +`GIT_LOCK_PID_INFO`::
> + If this Boolean environment variable is set to `1`, Git will create
> + a `.lock.pid` file alongside each lock file containing the PID of the
> + process that created the lock. This information is displayed in error
> + messages when a lock conflict occurs, making it easier to identify
> + stale locks or debug locking issues. The PID files are automatically
> + cleaned up via signal and atexit handlers; however, if a process is
> + terminated abnormally (e.g., SIGKILL), the file may remain as a stale
> + indicator. Disabled by default.
Regardless of whether or not we expose this functionality behind an
environment variable or configuration, I think it would be nice to be
able to turn PID tracking on and off for different components (e.g., for
scenarios where you care about who is holding open, say, $GIT_DIR/index,
but not who is creating a lock during ref creation).
If we determined this through an environment variable, I think it would
be reasonable to adopt the convention from the "core.fsync"
configuration and use a comma-separated list. Alternatively, we could
adopt that same convention for a configuration variable, say,
"core.lockfile".
But I think that having this exposed as a per-component setting via
configuration is preferable than a global switch, since callers don't
have to remember to set this variable in their environment to get the
desired effect. Callers that want to opt-in or out of this feature on a
one-off basis can still override the configuration via the top-level
"-c" flag.
> `GIT_REDIRECT_STDIN`::
> `GIT_REDIRECT_STDOUT`::
> `GIT_REDIRECT_STDERR`::
> diff --git a/lockfile.c b/lockfile.c
> index 1d5ed01682..4a694b9c3d 100644
> --- a/lockfile.c
> +++ b/lockfile.c
> @@ -6,6 +6,9 @@
> #include "abspath.h"
> #include "gettext.h"
> #include "lockfile.h"
> +#include "parse.h"
> +#include "strbuf.h"
> +#include "wrapper.h"
>
> /*
> * path = absolute or relative path name
> @@ -71,6 +74,62 @@ static void resolve_symlink(struct strbuf *path)
> strbuf_reset(&link);
> }
>
> +/*
> + * Lock PID file functions - write PID to a .lock.pid 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.
> + */
> +
> +static int lock_pid_info_enabled(void)
> +{
With the above suggestion, I think this function would get a little more
complicated to instead take a component argument. That's not a huge deal
in and of itself, but callers that *create* a lockfile will have to
somehow pass in the component name when acquiring the lock.
> + return git_env_bool(GIT_LOCK_PID_INFO_ENVIRONMENT, 0);
> +}
> +
> +static struct tempfile *create_lock_pid_file(const char *lock_path, int mode)
> +{
> + struct strbuf pid_path = STRBUF_INIT;
> + struct strbuf content = STRBUF_INIT;
> + struct tempfile *pid_tempfile = NULL;
> + int fd;
> +
> + if (!lock_pid_info_enabled())
> + return NULL;
> +
> + strbuf_addf(&pid_path, "%s%s", lock_path, LOCK_PID_SUFFIX);
> + fd = open(pid_path.buf, O_WRONLY | O_CREAT | O_TRUNC, mode);
I'm not sure whether or not we should pass O_EXCL here, but I think it
depends on the naming convention we pick.
> + if (fd >= 0) {
This is a style preference, but I'd suggest handling the happy path
outside of this conditional if possible by inverting it. Perhaps
something like:
if (fd < 0)
goto out;
strbuf_addf(&content, ...);
if (write_in_full(fd, content.buf, content.len) < 0)
warning_errno(...);
close(fd);
pid_tempfile = register_tempfile(pid_path.buf);
out:
strbuf_release(...);
return pid_tempfile;
> +static int read_lock_pid(const char *lock_path, uintmax_t *pid_out)
> +{
> + struct strbuf pid_path = STRBUF_INIT;
> + struct strbuf content = STRBUF_INIT;
> + int ret = -1;
> +
> + strbuf_addf(&pid_path, "%s%s", lock_path, LOCK_PID_SUFFIX);
> + if (strbuf_read_file(&content, pid_path.buf, LOCK_PID_MAXLEN) > 0) {
> + char *endptr;
> + *pid_out = strtoumax(content.buf, &endptr, 10);
> + if (*pid_out > 0 && (*endptr == '\n' || *endptr == '\0'))
> + ret = 0;
Same note here. I'd suggest setting "ret = 0" during initialization, and
inverting this conditional to:
if (*pid_out <= 0 || (*endptr != '\n' && *endptr != '\0')) {
warning(...);
ret = -1;
goto out;
}
> + else
> + warning(_("malformed lock pid file '%s'"), pid_path.buf);
> + }
> + strbuf_release(&pid_path);
> + strbuf_release(&content);
> + return ret;
> +}
> +
> /* Make sure errno contains a meaningful value on error */
> static int lock_file(struct lock_file *lk, const char *path, int flags,
> int mode)
> @@ -80,9 +139,12 @@ static int lock_file(struct lock_file *lk, const char *path, int flags,
> strbuf_addstr(&filename, path);
> if (!(flags & LOCK_NO_DEREF))
> resolve_symlink(&filename);
> -
> strbuf_addstr(&filename, LOCK_SUFFIX);
> +
These look like stray whitespace changes that were left behind from
development.
> + if (pid_status == 1)
> + strbuf_addf(buf, _("Lock is held by process %"PRIuMAX". "
> + "Wait for it to finish, or remove the lock file to continue"),
> + pid);
> + else if (pid_status == -1)
> + strbuf_addf(buf, _("Lock was held by process %"PRIuMAX", "
> + "which is no longer running. Remove the stale lock file to continue"),
> + pid);
> + else
> + strbuf_addstr(buf, _("Another git process seems to be running in this repository. "
> + "Wait for it to finish, or remove the lock file to continue"));
On one hand I prefer the new "Another git process" message for when we
don't have a PID lockfile. But on the other hand, I think the "If it
still fails, a git process may have crashed..." message is useful for
users who may not be immediately aware of the consequences of simply
removing the lockfile to continue.
I do think the original message is somewhat verbose, so maybe the change
here in the non-PID case is worth doing. What are your thoughts?
> +
> + strbuf_release(&lock_path);
> } else
> strbuf_addf(buf, _("Unable to create '%s.lock': %s"),
> absolute_path(path), strerror(err));
> @@ -207,6 +292,8 @@ int commit_lock_file(struct lock_file *lk)
> {
> char *result_path = get_locked_file_path(lk);
>
> + delete_tempfile(&lk->pid_tempfile);
> +
Do we want to wait to delete the PID file until after we know that we
successfully committed the lockfile?
> +/* Maximum length for PID file content */
> +#define LOCK_PID_MAXLEN 32
Makes sense ;-). This should be plenty of space, since on my system
maximum PID value is 2^22:
$ cat /proc/sys/kernel/pid_max
4194304
> +test_expect_success 'PID info file cleaned up on successful operation when enabled' '
> + git init repo4 &&
> + (
> + cd repo4 &&
> + echo content >file &&
> + env GIT_LOCK_PID_INFO=1 git add file &&
> + # After successful add, no lock or PID files should exist
> + ! test -f .git/index.lock &&
> + ! test -f .git/index.lock.pid
These should be:
test_path_is_missing .git/index.lock &&
test_path_is_missing .git/index.lock.pid
instead of bare "! test -f"'s.
Thanks,
Taylor |
Author
|
/preview |
|
Preview email sent as pull.2011.v2.git.1764871702690.gitgitgadget@gmail.com |
Author
|
/submit |
|
Error: 7068ec0 was already submitted |
pcasaretto
force-pushed
the
pid-holder-file
branch
from
7068ec0 to
cfbf3a8
Compare
|
On the Git mailing list, Patrick Steinhardt wrote (reply to this): On Wed, Dec 03, 2025 at 06:19:46PM -0500, Taylor Blau wrote:
> On Wed, Dec 03, 2025 at 04:16:10PM -0500, Jeff King wrote:
> > On Tue, Dec 02, 2025 at 03:07:27PM +0000, Paulo Casaretto via GitGitGadget wrote:
> >
> > > The .lock.pid file is created when a lock is acquired (if enabled), and
> > > automatically cleaned up when the lock is released (via commit or
> > > rollback). The file is registered as a tempfile so it gets cleaned up
> > > by signal and atexit handlers if the process terminates abnormally.
> >
> > I'm sympathetic to the goal of this series, and the implementation looks
> > cleanly done. But I wonder if there might be some system-level side
> > effects that make these .pid files awkward.
> >
> > Temporarily having an extra .git/index.lock.pid file is probably not a
> > big deal. But for other namespaces, like refs, we're colliding with
> > names that have other meanings. So if we want to update refs/heads/foo,
> > for example, we'll create refs/heads/foo.lock now. And after your patch,
> > also refs/heads/foo.lock.pid.
> >
> > The ".lock" suffix is special, in that we disallow it in a refname and
> > know to skip it when iterating over loose refs. But for the ".pid"
> > variant, we run the risk of colliding with a real branch named
> > "foo.lock.pid", both for reading and writing.
>
> Good point. I don't have a strong opinion on whether or not we should
> use an append-only log of which PIDs grabbed which lockfiles when versus
> tracking them on a per-lock basis. But I wonder if this would be
> mitigated by either:
>
> - Keeping the ".lock" suffix as-is, so that holding a lockfile at path
> "$GIT_DIR/index.lock" would create "$GIT_DIR/index-pid.lock" or
> something similar.
>
> - Introducing a new reference name constraint that treats ".lock.pid"
> as a reserved in a manner identical to how we currently treat
> ".lock".
>
> Between the two, I vastly prefer the former, but see below for more on
> why.
Yeah, I agree that this is not really a feasible change for the "files"
reference backend. Besides all the issues mentioned in this thread, we
also have to consider alternative implementations of Git and old
versions. Those wouldn't know that the ".lock.pid" files are special, so
they will misbehave if Git started to write them now.
We could of course work around that issue by introducing a new
repository extension. But I doubt that this is something we want to
pursue in this context. The provided benefit just isn't high enough.
The overall idea still has merit though. So if we still want to pursue
it we can likely work around the issue by introducing a toggle that
allows specific callers to opt out of creating the PID files.
That'd raise the question though when we are most likely to need the PID
files for debugging stuff. From my own experience I only ever had issues
with stale locks in the ref subsystem, so if we disable the mechanism in
exactly that subsystem it may be way less useful.
If references are the main culprit one could also think about a slightly
ugly alternative approach: loose refs handle it just fine if they
contain additional lines. So in theory, we could write a second line for
each lock file that contains the PID. We do have an fsck check that
warns about this, but in theory this should just work. Whether we want
to go there is a different question though.
Patrick |
|
User |
|
On the Git mailing list, Jeff King wrote (reply to this): On Wed, Dec 03, 2025 at 06:19:46PM -0500, Taylor Blau wrote:
> Changing the naming scheme as above would cause us to hold
> "foo.pid.lock" in addition to "foo.lock". That would allow process B
> here to write branch "foo.lock.pid" (as is the case today). But if the
> scenario were instead "process B wants to write branch foo.pid.lock", it
> would fail immediately since the ".lock" suffix is reserved.
I agree that this gets rid of any corruption or race issues, since all
versions understand how to handle ".lock" specially (and assuming we
create foo.pid.lock with O_EXCL). But there is still a namespace
collision; I cannot write refs "foo" and "foo.pid" at the same time.
> > So I dunno what that means for your patch. I notice that the user has to
> > enable the feature manually. But it feels more like it should be
> > selective based on which subsystem is using the lockfile (so refs would
> > never want it, but other lockfiles/tempfiles might).
>
> Yeah, I think that something similar to the "which files do we fsync()
> and how?" configuration we have today would be a nice complement here.
I'm not sure it makes sense for the user to configure this. I more meant
that the ref files-backend code would set a flag for "no, do not create
a pid file for me ever" (or inversely, other bits of the code would
add a flag for "yes, it's OK to do so").
-Peff |
pcasaretto
force-pushed
the
pid-holder-file
branch
from
cfbf3a8 to
6ff07d7
Compare
|
Submitted as pull.2011.v4.git.1767804355831.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "Paulo Casaretto via GitGitGadget" <gitgitgadget@gmail.com> writes:
> For a lock file "foo.lock", the PID file is named "foo~pid.lock". The
> tilde character is forbidden in refnames and allowed in Windows
> filenames, which guarantees no collision with the refs namespace
> (e.g., refs "foo" and "foo~pid" cannot both exist). The file contains
> a single line in the format "pid <value>" followed by a newline.
For a lockfile used for manipulating refs, that would be good, but
it would be more assuring to enumerate what other things do we use
lockfiles for, especially things whose filenames are under end-user
control (e.g., $GIT_DIR/index using index.lock and index~pid.lock
would probably be OK as the "index" part is unlikely to be renamed.
Is the ref-files the only thing that needs to address names
controlled by the end-user?).
> The PID file is created when a lock is acquired (if enabled), and
> automatically cleaned up when the lock is released (via commit or
> rollback). The file is registered as a tempfile so it gets cleaned up
> by signal and atexit handlers if the process terminates abnormally.
>
> When a lock conflict occurs, the code checks for an existing PID file
> and, if found, uses kill(pid, 0) to determine if the process is still
> running. This allows providing context-aware error messages:
>
> Lock is held by process 12345. Wait for it to finish, or remove
> the lock file to continue.
>
> Or for a stale lock:
>
> Lock was held by process 12345, which is no longer running.
> Remove the stale lock file to continue.
Two additional thoughts.
* 12345 may be running as your kill(12345,0) did not fail, but that
may be a process different from the one that held the lock and
then died without cleaning up.
* 12345 may be running and may have the lock. You may decide to
break the lock and take the risk of breaking your repository, or
you may wait until that process finishes what it is doing. If
you go the former route, however, would it make more sense to
kill the process so that it does not any harm interfering with
your new attempt? "remove the lock file to continue" sounds like
a fairly irresponsible suggestion. Not that "kill the process,
remove the lock file and then do whatever you wanted to do" is
any better.
> The feature is controlled via core.lockfilePid configuration (boolean).
> Defaults to false. When enabled, PID files are created for all lock
> operations.
>
> Existing PID files are always read when displaying lock errors,
> regardless of the core.lockfilePid setting. This ensures helpful
> diagnostics even when the feature was previously enabled and later
> disabled.
This makes it even more easily triggerable to notice a running
process that happens to be reusing a process ID an old process used
to have (i.e. the first "thought" above---the phrasing of the
message needs to be carefully thought out).
> Changes in v4:
>
> * Simplified core.lockfilePid from per-component configuration to a
> simple boolean (true/false), as suggested by Jeff and Patrick. The
> per-subsystem granularity added complexity without a clear use case -
> if someone wants PID files for debugging, they likely want them
> everywhere.
Very nice.
> * Removed the enum lockfile_pid_component and all LOCKFILE_PID_*
> constants
> * Removed the component parameter from hold_lock_file_for_update*()
> functions, restoring their original signatures
> * Simplified config parsing to use git_config_bool() instead of the
> fsync-style component parser
> * Fixed error handling in create_lock_pid_file() to unlink the file on
> write failure, as noted by Junio
> * Clarified documentation to describe the file format directly ("pid "
> followed by a newline) rather than referencing "Git object headers"
> * Updated tests and documentation to reflect the boolean config
Nicely enumerated changes.
> Documentation/config/core.adoc | 11 +++
> builtin/commit.c | 3 +-
> builtin/gc.c | 6 +-
> compat/mingw.c | 10 ++
> environment.c | 6 ++
> lockfile.c | 170 ++++++++++++++++++++++++++++++---
> lockfile.h | 43 ++++++---
> t/meson.build | 1 +
> t/t0031-lockfile-pid.sh | 105 ++++++++++++++++++++
> 9 files changed, 320 insertions(+), 35 deletions(-)
> create mode 100755 t/t0031-lockfile-pid.sh
>
> diff --git a/Documentation/config/core.adoc b/Documentation/config/core.adoc
> index 01202da7cd..5c4bc9206f 100644
> --- a/Documentation/config/core.adoc
> +++ b/Documentation/config/core.adoc
> @@ -348,6 +348,17 @@ confusion unless you know what you are doing (e.g. you are creating a
> read-only snapshot of the same index to a location different from the
> repository's usual working tree).
>
> +core.lockfilePid::
> + If true, Git will create a PID file alongside lock files. When a
> + lock acquisition fails and a PID file exists, Git can provide
Is it "Git can provide", or "Git provides"? I thought that I read
in the proposed log message that this configuration variable does
not control what happens when we see the pid file?
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 0243f17d53..4378256fa5 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -539,8 +539,7 @@ static const char *prepare_index(const char **argv, const char *prefix,
>
> path = repo_git_path(the_repository, "next-index-%"PRIuMAX,
> (uintmax_t) getpid());
> - hold_lock_file_for_update(&false_lock, path,
> - LOCK_DIE_ON_ERROR);
> + hold_lock_file_for_update(&false_lock, path, LOCK_DIE_ON_ERROR);
Is this change necessary to achieve the goal of your topic?
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 92c6e7b954..1dcc8dd550 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -748,8 +748,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
> xsnprintf(my_host, sizeof(my_host), "unknown");
>
> pidfile_path = repo_git_path(the_repository, "gc.pid");
> - fd = hold_lock_file_for_update(&lock, pidfile_path,
> - LOCK_DIE_ON_ERROR);
> + fd = hold_lock_file_for_update(&lock, pidfile_path, LOCK_DIE_ON_ERROR);
Ditto.
> @@ -1016,8 +1015,7 @@ int cmd_gc(int argc,
>
> if (daemonized) {
> char *path = repo_git_path(the_repository, "gc.log");
> - hold_lock_file_for_update(&log_lock, path,
> - LOCK_DIE_ON_ERROR);
> + hold_lock_file_for_update(&log_lock, path, LOCK_DIE_ON_ERROR);
Ditto.
> diff --git a/compat/mingw.c b/compat/mingw.c
> index 939f938fe2..146b2585ce 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -1972,6 +1972,16 @@ int mingw_kill(pid_t pid, int sig)
> CloseHandle(h);
> return 0;
> }
> + /*
> + * OpenProcess returns ERROR_INVALID_PARAMETER for
> + * non-existent PIDs. Map this to ESRCH for POSIX
> + * compatibility with kill(pid, 0).
> + */
> + if (GetLastError() == ERROR_INVALID_PARAMETER)
> + errno = ESRCH;
> + else
> + errno = err_win_to_posix(GetLastError());
> + return -1;
> }
>
> errno = EINVAL;
> diff --git a/environment.c b/environment.c
> index a770b5921d..4adcce8606 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -21,6 +21,7 @@
> #include "gettext.h"
> #include "git-zlib.h"
> #include "ident.h"
> +#include "lockfile.h"
> #include "mailmap.h"
> #include "object-name.h"
> #include "repository.h"
> @@ -532,6 +533,11 @@ static int git_default_core_config(const char *var, const char *value,
> return 0;
> }
>
> + if (!strcmp(var, "core.lockfilepid")) {
> + lockfile_pid_enabled = git_config_bool(var, value);
> + return 0;
> + }
> +
> if (!strcmp(var, "core.createobject")) {
> if (!value)
> return config_error_nonbool(var);
OK. Presumably lockfile.h declares the variable.
> diff --git a/lockfile.c b/lockfile.c
> index 1d5ed01682..ac7eb86541 100644
> --- a/lockfile.c
> +++ b/lockfile.c
> @@ -71,19 +74,119 @@ static void resolve_symlink(struct strbuf *path)
> ...
> + * Naming: For "foo.lock", the PID file is "foo~pid.lock". The tilde is
> + * forbidden in refnames and allowed in Windows filenames, guaranteeing
> + * no collision with the refs namespace.
A comment on other uses of lockfiles is needed here.
> +/*
> + * Path generation helpers.
> + * Given base path "foo", generate:
> + * - lock path: "foo.lock"
> + * - pid path: "foo-pid.lock"
> + */
Is the caller responsible for supplying an empty "out" buf? I am
wondering if it would make it easier to use if these functions
cleared the supplied strbuf as the first thing they do.
You could imagine a calling sequence
strbuf_reset(&outbuf);
strbuf_add(&outbuf, "seed");
get_lock_path(&outbuf, "index");
... use outbuf.buf ...
strbuf_reset(&outbuf);
strbuf_add(&outbuf, "seed");
get_pid_path(&outbuf, "index");
... use outbuf.buf ...
to give some prefix to the filenames used for locks, and making
these functions clear the strbuf upfront would not allow such a
calling sequence, but such a caller can just feed "seedindex" to the
API, so it does not feel like a huge loss.
Of course, we could go to the other extreme, and make it the
responsibility of the caller to stuff "path" to the strbuf before
calling these functions, i.e.,
strbuf_reset(&outbuf);
strbuf_add(&outbuf, "index");
get_lock_path(&outbuf);
I am not sure if it is a good direction to go, though. Certainly
the functions need to be renamed to "add_lock_suffix()" and
"add_pid_suffix" respectively, if we go that route.
> +static void get_lock_path(struct strbuf *out, const char *path)
> +{
> + strbuf_addstr(out, path);
> + strbuf_addstr(out, LOCK_SUFFIX);
> +}
> +
> +static void get_pid_path(struct strbuf *out, const char *path)
> +{
> + strbuf_addstr(out, path);
> + strbuf_addstr(out, LOCK_PID_INFIX);
> + strbuf_addstr(out, LOCK_SUFFIX);
> +}
> +static struct tempfile *create_lock_pid_file(const char *pid_path, int mode)
> +{
> + struct strbuf content = STRBUF_INIT;
> + struct tempfile *pid_tempfile = NULL;
> + int fd = -1;
> +
> + if (!lockfile_pid_enabled)
> + goto out;
> +
> + fd = open(pid_path, O_WRONLY | O_CREAT | O_EXCL, mode);
> + if (fd < 0)
> + goto out;
> +
> + strbuf_addf(&content, "pid %" PRIuMAX "\n", (uintmax_t)getpid());
> + if (write_in_full(fd, content.buf, content.len) < 0) {
> + warning_errno(_("could not write lock pid file '%s'"), pid_path);
> + close(fd);
> + fd = -1;
You can leave the above two lines out, as jumping to "out" would
take care of closing the file descriptor.
> + unlink(pid_path);
> + goto out;
> + }
> +
> + close(fd);
> + fd = -1;
> + pid_tempfile = register_tempfile(pid_path);
> +
> +out:
> + if (fd >= 0)
> + close(fd);
> + strbuf_release(&content);
> + return pid_tempfile;
> +}
> +static int read_lock_pid(const char *pid_path, uintmax_t *pid_out)
> +{
> + struct strbuf content = STRBUF_INIT;
> + const char *val;
> + int ret = -1;
> +
> + if (strbuf_read_file(&content, pid_path, LOCK_PID_MAXLEN) <= 0)
> + goto out;
> +
> + strbuf_rtrim(&content);
> +
> + if (skip_prefix(content.buf, "pid ", &val)) {
> + char *endptr;
> + *pid_out = strtoumax(val, &endptr, 10);
> + if (*pid_out > 0 && !*endptr)
> + ret = 0;
Zero is a valid process ID; comparing endptr with the val to ensure
that we did read _some_ digits would be a more robust test.
If you drop strbuf_rtrim() above, then you can even check if endptr
points at a '\n' byte to validate the whole line while ensuring that
you read the decimal number correctly.
> static int lock_file(struct lock_file *lk, const char *path, int flags,
> int mode)
> {
> - struct strbuf filename = STRBUF_INIT;
> + struct strbuf base_path = STRBUF_INIT;
> + struct strbuf lock_path = STRBUF_INIT;
> + struct strbuf pid_path = STRBUF_INIT;
>
> - strbuf_addstr(&filename, path);
> + strbuf_addstr(&base_path, path);
> if (!(flags & LOCK_NO_DEREF))
> - resolve_symlink(&filename);
> + resolve_symlink(&base_path);
> +
> + get_lock_path(&lock_path, base_path.buf);
> + get_pid_path(&pid_path, base_path.buf);
Looking at this place alone, add_lock/pid_suffix() I alluded to
earlier may not be _too_ bad. There may be other callers that makes
the "caller prepares the filename in the strbuf and calls add suffix
helpers" pattern unwieldy.
> @@ -151,16 +254,47 @@ static int lock_file_timeout(struct lock_file *lk, const char *path,
> void unable_to_lock_message(const char *path, int err, struct strbuf *buf)
> {
> if (err == EEXIST) {
> - strbuf_addf(buf, _("Unable to create '%s.lock': %s.\n\n"
> - "Another git process seems to be running in this repository, e.g.\n"
> - "an editor opened by 'git commit'. Please make sure all processes\n"
> - "are terminated then try again. If it still fails, a git process\n"
> - "may have crashed in this repository earlier:\n"
> - "remove the file manually to continue."),
> - absolute_path(path), strerror(err));
> - } else
> + const char *abs_path = absolute_path(path);
> + struct strbuf lock_path = STRBUF_INIT;
> + struct strbuf pid_path = STRBUF_INIT;
> + uintmax_t pid;
> + int pid_status = 0; /* 0 = unknown, 1 = running, -1 = stale */
> +
> + get_lock_path(&lock_path, abs_path);
> + get_pid_path(&pid_path, abs_path);
> +
> + strbuf_addf(buf, _("Unable to create '%s': %s.\n\n"),
> + lock_path.buf, strerror(err));
I wonder if code snippet from here ....
> + /*
> + * Try to read PID file unconditionally - it may exist if
> + * core.lockfilePid was enabled.
> + */
> + if (!read_lock_pid(pid_path.buf, &pid)) {
> + if (kill((pid_t)pid, 0) == 0 || errno == EPERM)
> + pid_status = 1; /* running (or no permission to signal) */
> + else if (errno == ESRCH)
> + pid_status = -1; /* no such process - stale lock */
> + }
> +
> + if (pid_status == 1)
> + strbuf_addf(buf, _("Lock is held by process %" PRIuMAX ". "
> + "Wait for it to finish, or remove the lock file to continue"),
> + pid);
> + else if (pid_status == -1)
> + strbuf_addf(buf, _("Lock was held by process %" PRIuMAX ", "
> + "which is no longer running. Remove the stale lock file to continue"),
> + pid);
> + else
> + strbuf_addstr(buf, _("Another git process seems to be running in this repository. "
> + "Wait for it to finish, or remove the lock file to continue"));
... to here should be in a separate helper function. It would make
the control flow easier to follow.
> + strbuf_release(&lock_path);
> + strbuf_release(&pid_path);
> + } else {
> strbuf_addf(buf, _("Unable to create '%s.lock': %s"),
> absolute_path(path), strerror(err));
> + }
> }
>
> NORETURN void unable_to_lock_die(const char *path, int err)
> @@ -207,6 +341,8 @@ int commit_lock_file(struct lock_file *lk)
> {
> char *result_path = get_locked_file_path(lk);
>
> + delete_tempfile(&lk->pid_tempfile);
> +
> if (commit_lock_file_to(lk, result_path)) {
> int save_errno = errno;
> free(result_path);
> @@ -216,3 +352,9 @@ int commit_lock_file(struct lock_file *lk)
> free(result_path);
> return 0;
> }
> +
> +int rollback_lock_file(struct lock_file *lk)
> +{
> + delete_tempfile(&lk->pid_tempfile);
> + return delete_tempfile(&lk->tempfile);
> +}
> diff --git a/lockfile.h b/lockfile.h
> index 1bb9926497..e7233f28de 100644
> --- a/lockfile.h
> +++ b/lockfile.h
> @@ -119,6 +119,7 @@
>
> struct lock_file {
> struct tempfile *tempfile;
> + struct tempfile *pid_tempfile;
> };
>
> #define LOCK_INIT { 0 }
> @@ -127,6 +128,22 @@ struct lock_file {
> #define LOCK_SUFFIX ".lock"
> #define LOCK_SUFFIX_LEN 5
>
> +/*
> + * PID file naming: for a lock file "foo.lock", the PID file is "foo~pid.lock".
> + * The tilde is forbidden in refnames and allowed in Windows filenames, avoiding
> + * namespace collisions (e.g., refs "foo" and "foo~pid" cannot both exist).
> + */
> +#define LOCK_PID_INFIX "~pid"
> +#define LOCK_PID_INFIX_LEN 4
> +
> +/* Maximum length for PID file content */
> +#define LOCK_PID_MAXLEN 32
> +
> +/*
> + * Whether to create PID files alongside lock files.
> + * Configured via core.lockfilePid (boolean).
> + */
> +extern int lockfile_pid_enabled;
>
> /*
> * Flags
> @@ -169,12 +186,12 @@ struct lock_file {
> * handling, and mode are described above.
> */
> int hold_lock_file_for_update_timeout_mode(
> - struct lock_file *lk, const char *path,
> - int flags, long timeout_ms, int mode);
> + struct lock_file *lk, const char *path,
> + int flags, long timeout_ms, int mode);
>
> static inline int hold_lock_file_for_update_timeout(
> - struct lock_file *lk, const char *path,
> - int flags, long timeout_ms)
> + struct lock_file *lk, const char *path,
> + int flags, long timeout_ms)
> {
> return hold_lock_file_for_update_timeout_mode(lk, path, flags,
> timeout_ms, 0666);
> @@ -186,15 +203,14 @@ static inline int hold_lock_file_for_update_timeout(
> * argument and error handling are described above.
> */
> static inline int hold_lock_file_for_update(
> - struct lock_file *lk, const char *path,
> - int flags)
> + struct lock_file *lk, const char *path, int flags)
> {
> return hold_lock_file_for_update_timeout(lk, path, flags, 0);
> }
>
> static inline int hold_lock_file_for_update_mode(
> - struct lock_file *lk, const char *path,
> - int flags, int mode)
> + struct lock_file *lk, const char *path,
> + int flags, int mode)
> {
> return hold_lock_file_for_update_timeout_mode(lk, path, flags, 0, mode);
> }
> @@ -319,13 +335,10 @@ static inline int commit_lock_file_to(struct lock_file *lk, const char *path)
>
> /*
> * Roll back `lk`: close the file descriptor and/or file pointer and
> - * remove the lockfile. It is a NOOP to call `rollback_lock_file()`
> - * for a `lock_file` object that has already been committed or rolled
> - * back. No error will be returned in this case.
> + * remove the lockfile and any associated PID file. It is a NOOP to
> + * call `rollback_lock_file()` for a `lock_file` object that has already
> + * been committed or rolled back. No error will be returned in this case.
> */
> -static inline int rollback_lock_file(struct lock_file *lk)
> -{
> - return delete_tempfile(&lk->tempfile);
> -}
> +int rollback_lock_file(struct lock_file *lk);
>
> #endif /* LOCKFILE_H */
> diff --git a/t/meson.build b/t/meson.build
> index 459c52a489..2aec2c011e 100644
> --- a/t/meson.build
> +++ b/t/meson.build
> @@ -98,6 +98,7 @@ integration_tests = [
> 't0028-working-tree-encoding.sh',
> 't0029-core-unsetenvvars.sh',
> 't0030-stripspace.sh',
> + 't0031-lockfile-pid.sh',
> 't0033-safe-directory.sh',
> 't0034-root-safe-directory.sh',
> 't0035-safe-bare-repository.sh',
> diff --git a/t/t0031-lockfile-pid.sh b/t/t0031-lockfile-pid.sh
> new file mode 100755
> index 0000000000..44f3ba1f25
> --- /dev/null
> +++ b/t/t0031-lockfile-pid.sh
> @@ -0,0 +1,105 @@
> +#!/bin/sh
> +
> +test_description='lock file PID info tests
> +
> +Tests for PID info file alongside lock files.
> +The feature is opt-in via core.lockfilePid config setting (boolean).
> +'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'stale lock detected when PID is not running' '
> + git init repo &&
> + (
> + cd repo &&
> + touch .git/index.lock &&
> + printf "pid 99999" >.git/index~pid.lock &&
> + test_must_fail git -c core.lockfilePid=true add . 2>err &&
> + test_grep "process 99999, which is no longer running" err &&
> + test_grep "Remove the stale lock file" err
> + )
> +'
> +
> +test_expect_success 'PID info not shown by default' '
> + git init repo2 &&
> + (
> + cd repo2 &&
> + touch .git/index.lock &&
> + printf "pid 99999" >.git/index~pid.lock &&
> + test_must_fail git add . 2>err &&
> + # Should not crash, just show normal error without PID
> + test_grep "Unable to create" err &&
> + ! test_grep "is held by process" err
> + )
> +'
> +
> +test_expect_success 'running process detected when PID is alive' '
> + git init repo3 &&
> + (
> + cd repo3 &&
> + echo content >file &&
> + # Get the correct PID for this platform
> + shell_pid=$$ &&
> + if test_have_prereq MINGW && test -f /proc/$shell_pid/winpid
> + then
> + # In Git for Windows, Bash uses MSYS2 PIDs but git.exe
> + # uses Windows PIDs. Use the Windows PID.
> + shell_pid=$(cat /proc/$shell_pid/winpid)
> + fi &&
> + # Create a lock and PID file with current shell PID (which is running)
> + touch .git/index.lock &&
> + printf "pid %d" "$shell_pid" >.git/index~pid.lock &&
> + # Verify our PID is shown in the error message
> + test_must_fail git -c core.lockfilePid=true add file 2>err &&
> + test_grep "held by process $shell_pid" err
> + )
> +'
> +
> +test_expect_success 'PID info file cleaned up on successful operation when enabled' '
> + git init repo4 &&
> + (
> + cd repo4 &&
> + echo content >file &&
> + git -c core.lockfilePid=true add file &&
> + # After successful add, no lock or PID files should exist
> + test_path_is_missing .git/index.lock &&
> + test_path_is_missing .git/index~pid.lock
> + )
> +'
> +
> +test_expect_success 'no PID file created by default' '
> + git init repo5 &&
> + (
> + cd repo5 &&
> + echo content >file &&
> + git add file &&
> + # PID file should not be created when feature is disabled
> + test_path_is_missing .git/index~pid.lock
> + )
> +'
> +
> +test_expect_success 'core.lockfilePid=false does not create PID file' '
> + git init repo6 &&
> + (
> + cd repo6 &&
> + echo content >file &&
> + git -c core.lockfilePid=false add file &&
> + # PID file should not be created when feature is disabled
> + test_path_is_missing .git/index~pid.lock
> + )
> +'
> +
> +test_expect_success 'existing PID files are read even when feature disabled' '
> + git init repo7 &&
> + (
> + cd repo7 &&
> + touch .git/index.lock &&
> + printf "pid 99999" >.git/index~pid.lock &&
> + # Even with lockfilePid disabled, existing PID files are read
> + # to help diagnose stale locks
> + test_must_fail git add . 2>err &&
> + test_grep "process 99999" err
> + )
> +'
> +
> +test_done
>
> base-commit: 66ce5f8e8872f0183bb137911c52b07f1f242d13 |
|
On the Git mailing list, "D. Ben Knoble" wrote (reply to this): On Wed, Jan 7, 2026 at 8:59 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Paulo Casaretto via GitGitGadget" <gitgitgadget@gmail.com> writes:
> > The PID file is created when a lock is acquired (if enabled), and
> > automatically cleaned up when the lock is released (via commit or
> > rollback). The file is registered as a tempfile so it gets cleaned up
> > by signal and atexit handlers if the process terminates abnormally.
> >
> > When a lock conflict occurs, the code checks for an existing PID file
> > and, if found, uses kill(pid, 0) to determine if the process is still
> > running. This allows providing context-aware error messages:
> >
> > Lock is held by process 12345. Wait for it to finish, or remove
> > the lock file to continue.
> >
> > Or for a stale lock:
> >
> > Lock was held by process 12345, which is no longer running.
> > Remove the stale lock file to continue.
>
> Two additional thoughts.
>
> * 12345 may be running as your kill(12345,0) did not fail, but that
> may be a process different from the one that held the lock and
> then died without cleaning up.
[snip]
> > The feature is controlled via core.lockfilePid configuration (boolean).
> > Defaults to false. When enabled, PID files are created for all lock
> > operations.
> >
> > Existing PID files are always read when displaying lock errors,
> > regardless of the core.lockfilePid setting. This ensures helpful
> > diagnostics even when the feature was previously enabled and later
> > disabled.
>
> This makes it even more easily triggerable to notice a running
> process that happens to be reusing a process ID an old process used
> to have (i.e. the first "thought" above---the phrasing of the
> message needs to be carefully thought out).
Yep. I mentioned this back in
<CALnO6CB1igUL7nv6ByUmwMRc9tqEvs=18wD81GNpaA=FLpL2vw@mail.gmail.com>:
> just because a process X is running doesn't mean
> it was the one holding the lock, right? Since PIDs can be reused.
which seems to have been ignored, but I'm glad to know I didn't make
this concern up out of thin air :)
Best,
--
D. Ben Knoble |
|
This branch is now known as |
|
This patch series was integrated into seen via git@bf18600. |
|
There was a status update in the "Cooking" section about the branch Allow recording process ID of the process that holds the lock next to a lockfile for diagnosis. Comments? source: <pull.2011.v4.git.1767804355831.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@59eb1d0. |
|
There was a status update in the "Cooking" section about the branch Allow recording process ID of the process that holds the lock next to a lockfile for diagnosis. Comments? source: <pull.2011.v4.git.1767804355831.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@9c4ddf5. |
|
This patch series was integrated into seen via git@5b6e1bb. |
When a lock file is held, it can be helpful to know which process owns it, especially when debugging stale locks left behind by crashed processes. Add an optional feature that creates a companion PID file alongside each lock file, containing the PID of the lock holder. For a lock file "foo.lock", the PID file is named "foo~pid.lock". The tilde character is forbidden in refnames and allowed in Windows filenames, which guarantees no collision with the refs namespace (e.g., refs "foo" and "foo~pid" cannot both exist). The file contains a single line in the format "pid <value>" followed by a newline. The PID file is created when a lock is acquired (if enabled), and automatically cleaned up when the lock is released (via commit or rollback). The file is registered as a tempfile so it gets cleaned up by signal and atexit handlers if the process terminates abnormally. When a lock conflict occurs, the code checks for an existing PID file and, if found, uses kill(pid, 0) to determine if the process is still running. This allows providing context-aware error messages: Lock is held by process 12345. Wait for it to finish, or remove the lock file to continue. Or for a stale lock: Lock was held by process 12345, which is no longer running. Remove the stale lock file to continue. The feature is controlled via core.lockfilePid configuration (boolean). Defaults to false. When enabled, PID files are created for all lock operations. Existing PID files are always read when displaying lock errors, regardless of the core.lockfilePid setting. This ensures helpful diagnostics even when the feature was previously enabled and later disabled. Signed-off-by: Paulo Casaretto <pcasaretto@gmail.com>
pcasaretto
force-pushed
the
pid-holder-file
branch
from
c26bc77 to
5260bf2
Compare
|
This patch series was integrated into seen via git@d2a18ca. |
|
There was a status update in the "Cooking" section about the branch Allow recording process ID of the process that holds the lock next to a lockfile for diagnosis. Comments? source: <pull.2011.v4.git.1767804355831.gitgitgadget@gmail.com> |
Author
|
/submit |
|
Submitted as pull.2011.v5.git.1768933954845.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
Junio C Hamano wrote on the Git mailing list (how to reply to this email): "Paulo Casaretto via GitGitGadget" <gitgitgadget@gmail.com> writes:
> Existing PID files are always read when displaying lock errors,
> regardless of the core.lockfilePid setting. This ensures helpful
> diagnostics even when the feature was previously enabled and later
> disabled.
> +core.lockfilePid::
> + If true, Git will create a PID file alongside lock files. When a
Hmph, are there cases where a single process grab two more more lock
files and a single PID file helps identify who holds these multiple
lock files? I somehow had an impression that with the configuration
on, we create an extra PID file for each lock file we create.
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 0243f17d53..4378256fa5 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -539,8 +539,7 @@ static const char *prepare_index(const char **argv, const char *prefix,
>
> path = repo_git_path(the_repository, "next-index-%"PRIuMAX,
> (uintmax_t) getpid());
> - hold_lock_file_for_update(&false_lock, path,
> - LOCK_DIE_ON_ERROR);
> + hold_lock_file_for_update(&false_lock, path, LOCK_DIE_ON_ERROR);
A change that is not necessary/essential for the purpose of the
topic?
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 92c6e7b954..1dcc8dd550 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -748,8 +748,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
> xsnprintf(my_host, sizeof(my_host), "unknown");
>
> pidfile_path = repo_git_path(the_repository, "gc.pid");
> - fd = hold_lock_file_for_update(&lock, pidfile_path,
> - LOCK_DIE_ON_ERROR);
> + fd = hold_lock_file_for_update(&lock, pidfile_path, LOCK_DIE_ON_ERROR);
Likewise?
> @@ -1016,8 +1015,7 @@ int cmd_gc(int argc,
>
> if (daemonized) {
> char *path = repo_git_path(the_repository, "gc.log");
> - hold_lock_file_for_update(&log_lock, path,
> - LOCK_DIE_ON_ERROR);
> + hold_lock_file_for_update(&log_lock, path, LOCK_DIE_ON_ERROR);
Likewise?
> +static void get_pid_path(struct strbuf *out, const char *path)
> +{
> + strbuf_addstr(out, path);
> + strbuf_addstr(out, LOCK_PID_INFIX);
> + strbuf_addstr(out, LOCK_SUFFIX);
> +}
If we get rid of LOCK_PID_INFIX, and instead did
#define PID_LOCK_SUFFIX "~pid.lock"
wouldn't it make the overall patch simpler? I do not see any code
that uses LOCK_PID_INFIX independently (other than the above code,
obviously). Then get_pid_path() would become
static void get_pid_path(struct strbuf *out, const char *path)
{
strbuf_addstr(out, path);
strbuf_addstr(out, PID_LOCK_SUFFIX);
}
While at it, we can also lose LOCK_PID_INFIX_LEN that nobody uses ...
> @@ -127,6 +128,22 @@ struct lock_file {
> #define LOCK_SUFFIX ".lock"
> #define LOCK_SUFFIX_LEN 5
>
> +/*
> + * PID file naming: for a lock file "foo.lock", the PID file is "foo~pid.lock".
> + * The tilde is forbidden in refnames and allowed in Windows filenames, avoiding
> + * namespace collisions (e.g., refs "foo" and "foo~pid" cannot both exist).
> + */
> +#define LOCK_PID_INFIX "~pid"
> +#define LOCK_PID_INFIX_LEN 4
... from here.
> +
> +/* Maximum length for PID file content */
> +#define LOCK_PID_MAXLEN 32
This is used as the last parameter to strbuf_read_file(), which does
not mean we stop reading after reaching this length at all, so the
name of this constant is very misleading. We only read the file in
the error code path so there isn't much point in trying to optimize
reading from these files too heavily. I'd suggest getting rid of
this constant, and passing 0 to the function instead to signal "we
are willing to take the default given by the strbuf subsystem".
|
|
This patch series was integrated into seen via git@3bed3ab. |
|
Jeff King wrote on the Git mailing list (how to reply to this email): On Tue, Jan 20, 2026 at 06:32:34PM +0000, Paulo Casaretto via GitGitGadget wrote:
> +static struct tempfile *create_lock_pid_file(const char *pid_path, int mode)
> +{
> + struct strbuf content = STRBUF_INIT;
> + struct tempfile *pid_tempfile = NULL;
> + int fd = -1;
> +
> + if (!lockfile_pid_enabled)
> + goto out;
> +
> + fd = open(pid_path, O_WRONLY | O_CREAT | O_EXCL, mode);
> + if (fd < 0)
> + goto out;
> +
> + strbuf_addf(&content, "pid %" PRIuMAX "\n", (uintmax_t)getpid());
> + if (write_in_full(fd, content.buf, content.len) < 0) {
> + warning_errno(_("could not write lock pid file '%s'"), pid_path);
> + close(fd);
> + fd = -1;
> + unlink(pid_path);
> + goto out;
> + }
> +
> + close(fd);
> + fd = -1;
> + pid_tempfile = register_tempfile(pid_path);
> +
> +out:
> + if (fd >= 0)
> + close(fd);
> + strbuf_release(&content);
> + return pid_tempfile;
> +}
Coverity complains that the close(fd) call in the "out" label is
unreachable, and I think it is right. When we jump from before the
open(), or if the open failed, then fd is negative (and thus no close).
If we get there when write_in_full() fails, then we close ourselves in
the conditional. And if we succeed, then we close the descriptor before
registering the tempfile.
I don't think it's wrong, but the cleanup is redundant between the "out"
path and the others.
Did you mean this:
diff --git a/lockfile.c b/lockfile.c
index 731cdd4944..e5d6ae0df6 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -122,14 +122,10 @@ static struct tempfile *create_lock_pid_file(const char *pid_path, int mode)
strbuf_addf(&content, "pid %" PRIuMAX "\n", (uintmax_t)getpid());
if (write_in_full(fd, content.buf, content.len) < 0) {
warning_errno(_("could not write lock pid file '%s'"), pid_path);
- close(fd);
- fd = -1;
unlink(pid_path);
goto out;
}
- close(fd);
- fd = -1;
pid_tempfile = register_tempfile(pid_path);
out:
which would just let the close after the out label handle all cases?
-Peff |
|
Eric Sunshine wrote on the Git mailing list (how to reply to this email): On Wed, Jan 21, 2026 at 2:15 AM Jeff King <peff@peff.net> wrote:
> I don't think it's wrong, but the cleanup is redundant between the "out"
> path and the others.
>
> Did you mean this:
>
> diff --git a/lockfile.c b/lockfile.c
> @@ -122,14 +122,10 @@ static struct tempfile *create_lock_pid_file(const char *pid_path, int mode)
> strbuf_addf(&content, "pid %" PRIuMAX "\n", (uintmax_t)getpid());
> if (write_in_full(fd, content.buf, content.len) < 0) {
> warning_errno(_("could not write lock pid file '%s'"), pid_path);
> - close(fd);
> - fd = -1;
> unlink(pid_path);
> goto out;
> }
>
> - close(fd);
> - fd = -1;
> pid_tempfile = register_tempfile(pid_path);
>
> out:
>
> which would just let the close after the out label handle all cases?
Correct me if I'm wrong, but wouldn't this suggested change be
problematic on Microsoft Windows? Specifically, if I recall correctly,
Windows won't allow a file to be deleted if any processes still have
it open, and this change eliminates the call to close() preceding the
call to unlink(), so the file would still be held open when the
attempt is made to remove it.
If so, then probably better would be to drop the unreachable `if (fd
>= 0) close(fd)` after the `out` label. |
|
User |
|
Johannes Sixt wrote on the Git mailing list (how to reply to this email): Am 21.01.26 um 09:13 schrieb Eric Sunshine:
> On Wed, Jan 21, 2026 at 2:15 AM Jeff King <peff@peff.net> wrote:
>> I don't think it's wrong, but the cleanup is redundant between the "out"
>> path and the others.
>>
>> Did you mean this:
>>
>> diff --git a/lockfile.c b/lockfile.c
>> @@ -122,14 +122,10 @@ static struct tempfile *create_lock_pid_file(const char *pid_path, int mode)
>> strbuf_addf(&content, "pid %" PRIuMAX "\n", (uintmax_t)getpid());
>> if (write_in_full(fd, content.buf, content.len) < 0) {
>> warning_errno(_("could not write lock pid file '%s'"), pid_path);
>> - close(fd);
>> - fd = -1;
>> unlink(pid_path);
>> goto out;
>> }
>>
>> - close(fd);
>> - fd = -1;
>> pid_tempfile = register_tempfile(pid_path);
>>
>> out:
>>
>> which would just let the close after the out label handle all cases?
>
> Correct me if I'm wrong, but wouldn't this suggested change be
> problematic on Microsoft Windows? Specifically, if I recall correctly,
> Windows won't allow a file to be deleted if any processes still have
> it open, and this change eliminates the call to close() preceding the
> call to unlink(), so the file would still be held open when the
> attempt is made to remove it.
You analysis is correct. I was just about to point this out, too.
-- Hannes
|
|
User |
|
This patch series was integrated into seen via git@b417d61. |
|
Junio C Hamano wrote on the Git mailing list (how to reply to this email): Jeff King <peff@peff.net> writes:
> I don't think it's wrong, but the cleanup is redundant between the "out"
> path and the others.
>
> Did you mean this:
>
> diff --git a/lockfile.c b/lockfile.c
> index 731cdd4944..e5d6ae0df6 100644
> --- a/lockfile.c
> +++ b/lockfile.c
> @@ -122,14 +122,10 @@ static struct tempfile *create_lock_pid_file(const char *pid_path, int mode)
> strbuf_addf(&content, "pid %" PRIuMAX "\n", (uintmax_t)getpid());
> if (write_in_full(fd, content.buf, content.len) < 0) {
> warning_errno(_("could not write lock pid file '%s'"), pid_path);
> - close(fd);
> - fd = -1;
> unlink(pid_path);
> goto out;
> }
>
> - close(fd);
> - fd = -1;
> pid_tempfile = register_tempfile(pid_path);
>
> out:
>
> which would just let the close after the out label handle all cases?
I recall suggesting this myself in
https://lore.kernel.org/git/xmqqbjj4hnkr.fsf@gitster.g
without realizing that this would probably not work on Windows where
unlink() cannot work correctly until the file descriptor is closed. |
|
Jeff King wrote on the Git mailing list (how to reply to this email): On Wed, Jan 21, 2026 at 03:13:41AM -0500, Eric Sunshine wrote:
> > diff --git a/lockfile.c b/lockfile.c
> > @@ -122,14 +122,10 @@ static struct tempfile *create_lock_pid_file(const char *pid_path, int mode)
> > strbuf_addf(&content, "pid %" PRIuMAX "\n", (uintmax_t)getpid());
> > if (write_in_full(fd, content.buf, content.len) < 0) {
> > warning_errno(_("could not write lock pid file '%s'"), pid_path);
> > - close(fd);
> > - fd = -1;
> > unlink(pid_path);
> > goto out;
> > }
> >
> > - close(fd);
> > - fd = -1;
> > pid_tempfile = register_tempfile(pid_path);
> >
> > out:
> >
> > which would just let the close after the out label handle all cases?
>
> Correct me if I'm wrong, but wouldn't this suggested change be
> problematic on Microsoft Windows? Specifically, if I recall correctly,
> Windows won't allow a file to be deleted if any processes still have
> it open, and this change eliminates the call to close() preceding the
> call to unlink(), so the file would still be held open when the
> attempt is made to remove it.
>
> If so, then probably better would be to drop the unreachable `if (fd
> >= 0) close(fd)` after the `out` label.
Ah, yeah, you're right. Ironically I spent quite a while thinking on the
implications of calling close() before register_tempfile() and decided
it didn't matter, but totally ignored the first half of the hunk. ;)
The second half is still valid, I think, but at that point it is the
only path that uses the close() in the out-path, so we might as well
drop the out-path one.
-Peff |
|
Junio C Hamano wrote on the Git mailing list (how to reply to this email): Jeff King <peff@peff.net> writes:
> The second half is still valid, I think, but at that point it is the
> only path that uses the close() in the out-path, so we might as well
> drop the out-path one.
True. A fix-up may look like this. I've got rid of the assignments
to "fd" that are not used.
The changes to first two files simply revert unnecessary changes.
builtin/commit.c | 3 ++-
builtin/gc.c | 6 ++++--
lockfile.c | 6 +-----
3 files changed, 7 insertions(+), 8 deletions(-)
diff --git c/builtin/commit.c w/builtin/commit.c
index 4378256fa5..0243f17d53 100644
--- c/builtin/commit.c
+++ w/builtin/commit.c
@@ -539,7 +539,8 @@ static const char *prepare_index(const char **argv, const char *prefix,
path = repo_git_path(the_repository, "next-index-%"PRIuMAX,
(uintmax_t) getpid());
- hold_lock_file_for_update(&false_lock, path, LOCK_DIE_ON_ERROR);
+ hold_lock_file_for_update(&false_lock, path,
+ LOCK_DIE_ON_ERROR);
create_base_index(current_head);
add_remove_files(&partial);
diff --git c/builtin/gc.c w/builtin/gc.c
index 1dcc8dd550..92c6e7b954 100644
--- c/builtin/gc.c
+++ w/builtin/gc.c
@@ -748,7 +748,8 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
xsnprintf(my_host, sizeof(my_host), "unknown");
pidfile_path = repo_git_path(the_repository, "gc.pid");
- fd = hold_lock_file_for_update(&lock, pidfile_path, LOCK_DIE_ON_ERROR);
+ fd = hold_lock_file_for_update(&lock, pidfile_path,
+ LOCK_DIE_ON_ERROR);
if (!force) {
static char locking_host[HOST_NAME_MAX + 1];
static char *scan_fmt;
@@ -1015,7 +1016,8 @@ int cmd_gc(int argc,
if (daemonized) {
char *path = repo_git_path(the_repository, "gc.log");
- hold_lock_file_for_update(&log_lock, path, LOCK_DIE_ON_ERROR);
+ hold_lock_file_for_update(&log_lock, path,
+ LOCK_DIE_ON_ERROR);
dup2(get_lock_file_fd(&log_lock), 2);
atexit(process_log_file_at_exit);
free(path);
diff --git c/lockfile.c w/lockfile.c
index 6d03c60d50..13e2ad1307 100644
--- c/lockfile.c
+++ w/lockfile.c
@@ -110,7 +110,7 @@ static struct tempfile *create_lock_pid_file(const char *pid_path, int mode)
{
struct strbuf content = STRBUF_INIT;
struct tempfile *pid_tempfile = NULL;
- int fd = -1;
+ int fd;
if (!lockfile_pid_enabled)
goto out;
@@ -123,18 +123,14 @@ static struct tempfile *create_lock_pid_file(const char *pid_path, int mode)
if (write_in_full(fd, content.buf, content.len) < 0) {
warning_errno(_("could not write lock pid file '%s'"), pid_path);
close(fd);
- fd = -1;
unlink(pid_path);
goto out;
}
close(fd);
- fd = -1;
pid_tempfile = register_tempfile(pid_path);
out:
- if (fd >= 0)
- close(fd);
strbuf_release(&content);
return pid_tempfile;
} |
|
There was a status update in the "Cooking" section about the branch Allow recording process ID of the process that holds the lock next to a lockfile for diagnosis. Will merge to 'next'? source: <pull.2011.v5.git.1768933954845.gitgitgadget@gmail.com> |
|
Jeff King wrote on the Git mailing list (how to reply to this email): On Wed, Jan 21, 2026 at 10:55:42AM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > The second half is still valid, I think, but at that point it is the
> > only path that uses the close() in the out-path, so we might as well
> > drop the out-path one.
>
> True. A fix-up may look like this. I've got rid of the assignments
> to "fd" that are not used.
Yup, that all looks correct to me.
-Peff |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes in v5:
now acknowledge that a running process with the recorded PID may not be
the original lock holder. Messages changed from definitive ("Lock is
held by process X") to cautious ("Lock may be held by process X; if no
git process is running, the lock file may be stale (PIDs can be
reused)").
as this is too prescriptive about what action to take.
CC: Taylor Blau me@ttaylorr.com
cc: "D. Ben Knoble" ben.knoble@gmail.com
cc: Torsten Bögershausen tboegi@web.de
cc: Jeff King peff@peff.net
cc: "Paulo Casaretto (Shopify)" paulo.casaretto@shopify.com
cc: Patrick Steinhardt ps@pks.im
cc: Eric Sunshine sunshine@sunshineco.com
cc: Johannes Sixt j6t@kdbg.org