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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jan 21, 2026, 08:13 UTC
Message-ID: <CAPig+cSE7Y-MLu1PTdo2kUq_MztMQgm0eYby03cX2K5YAJLwsg@mail.gmail.com>
In-Reply-To: <20260121071344.GA570838@coredump.intra.peff.net>

On Wed, Jan 21, 2026 at 2:15 AM Jeff King <peff@peff.net> wrote:

Show 23 quoted lines

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