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

[Junio C Hamano <gitster@pobox.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.

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