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

[Junio C Hamano <gitster@pobox.com>]

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

Lucky we had this wiggle room for us ;-)

> The file uses a
> simple key-value format ("pid <value>") following the same pattern as
> Git object headers, making it extensible for future metadata.

It may be just me, but "a Git object header" to me means the "<type>
<length> <NUL>".  I suspect that you modelled this more after
commit headers (in which case you may want to say "pid <value> LF"
to stress the fact that a record is newline terminated).

> +The PID file is named by inserting `.pid` before the `.lock` suffix.

Don't we use tilde somewhere???

> +For example, if the lock file is `index.lock`, the PID file will be
> +`index.pid.lock`. The file contains `pid <value>` on a single line,
> +following the same key-value format used by Git object headers.

Same comment as the one for the proposed log message applies here.

> diff --git a/apply.c b/apply.c
> index c9fb45247d..6d24f1d9f8 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -4212,7 +4212,8 @@ static int build_fake_ancestor(struct apply_state *state, struct patch *list)
>  		}
>  	}
>  
> -	hold_lock_file_for_update(&lock, state->fake_ancestor, LOCK_DIE_ON_ERROR);
> +	hold_lock_file_for_update(&lock, state->fake_ancestor, LOCK_DIE_ON_ERROR,
> +				  LOCKFILE_PID_OTHER);

Hmph, I thought I suggested not to send this as a separate
parameter, and instead use a set of bits in an existing flag word
that currently carries only "what to do upon error" bits?

>  			hold_lock_file_for_update(&state->lock_file,
>  						  state->index_file,
> -						  LOCK_DIE_ON_ERROR);
> +						  LOCK_DIE_ON_ERROR,
> +						  LOCKFILE_PID_INDEX);

Ditto.

> +static const struct lockfile_pid_component_name {
> +	const char *name;
> +	enum lockfile_pid_component component_bits;
> +} lockfile_pid_component_names[] = {
> +	{ "index", LOCKFILE_PID_INDEX },
> +	{ "config", LOCKFILE_PID_CONFIG },
> +	{ "refs", LOCKFILE_PID_REFS },
> +	{ "commit-graph", LOCKFILE_PID_COMMIT_GRAPH },
> +	{ "midx", LOCKFILE_PID_MIDX },
> +	{ "shallow", LOCKFILE_PID_SHALLOW },
> +	{ "gc", LOCKFILE_PID_GC },
> +	{ "other", LOCKFILE_PID_OTHER },
> +	{ "all", LOCKFILE_PID_ALL },
> +};

I wonder if we want to sort the array entries here, even if we look
it up linearly (as the table is small enough)?

> +	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);
> +		goto out;

The caller still will get a non-NULL pid_tempfile?  If you failed to
write the contents, wouldn't it allow the caller to handle the
failure more smoothly if you allowed the caller to know about the
failure, perhaps by removing the temporary file and returning NULL?

> +	}
> +
> +	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;

Any and all failure to read the pid_path is a silent, including an
empty one?

I'll stop here for now.

Thanks.