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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 20, 2026, 20:02 UTC
Message-ID: <xmqqjyxcyrvm.fsf@gitster.g>
In-Reply-To: <pull.2011.v5.git.1768933954845.gitgitgadget@gmail.com>

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

Show 11 quoted lines

> 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?

Show 11 quoted lines

> 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?

Show 7 quoted lines

> @@ -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?

Show 6 quoted lines

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

Show 11 quoted lines

> @@ -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".