Re: [PATCH v3] lockfile: add PID file for debugging stale locks
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Dec 25, 2025, 00:01 UTC
- Message-ID
- <xmqqwm2bmnug.fsf@gitster.g>
- In-Reply-To
- <pull.2011.v3.git.1766579053136.gitgitgadget@gmail.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.
Show 11 quoted lines
> 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?
Show 5 quoted lines
> hold_lock_file_for_update(&state->lock_file, > state->index_file, > - LOCK_DIE_ON_ERROR); > + LOCK_DIE_ON_ERROR, > + LOCKFILE_PID_INDEX);
Ditto.
Show 14 quoted lines
> +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?
Show 12 quoted lines
> + } > + > + close(fd); > + fd = -1; > + pid_tempfile = register_tempfile(pid_path); > + > +out: > + if (fd >= 0) > + close(fd); > + strbuf_release(&content); > + return pid_tempfile; > +}
Show 8 quoted lines
> +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.