From: Jeff King <peff@peff.net>
Date: Wed, 21 Jan 2026 16:39:24 GMT
Subject: Re: [PATCH v5] lockfile: add PID file for debugging stale locks
Message-ID: <20260121163924.GA576236@coredump.intra.peff.net>
In-Reply-To: <CAPig+cSE7Y-MLu1PTdo2kUq_MztMQgm0eYby03cX2K5YAJLwsg@mail.gmail.com>

On Wed, Jan 21, 2026 at 03:13:41AM -0500, Eric Sunshine wrote:

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

Ah, yeah, you're right. Ironically I spent quite a while thinking on the
implications of calling close() before register_tempfile() and decided
it didn't matter, but totally ignored the first half of the hunk. ;)

The second half is still valid, I think, but at that point it is the
only path that uses the close() in the out-path, so we might as well
drop the out-path one.

-Peff