Re: [PATCH v2 1/1] bundle: cleanup lock files on error

From: Jeff King <peff@peff.net>
Date: Nov 15, 2018, 04:34 UTC
Message-ID: <20181115043409.GA3419@sigill.intra.peff.net>
In-Reply-To: <CAGZ79kZ_wrQ=OdJ6xWbL+F5RDb38YAmzc87o8A=Zb8zBywMJBQ@mail.gmail.com>

On Wed, Nov 14, 2018 at 02:08:48PM -0800, Stefan Beller wrote:

Show 15 quoted lines

> On Wed, Nov 14, 2018 at 1:43 PM Martin Ågren <martin.agren@gmail.com> wrote:
> >
> > On Wed, 14 Nov 2018 at 16:26, Gaël Lhez via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:
> > > However, the `.lock` file was still open and on Windows that means
> > > that it could not be deleted properly. This patch fixes that issue.
> >
> > Hmmm, doesn't the tempfile machinery remove the lock file when we die?
> 
> On Windows this seems not to be the case. (Open files cannot be deleted
> as the open file is not kept by inode or similar but by the file path there?)
> 
> Rewording your concern: Could the tempfile machinery be taught to
> work properly on Windows, e.g. by first closing all files and then deleting
> them afterwards?

It already tries to do so. See delete_tempfile(), or more likely in the die() case, the remove_tempfiles() handler which is called at exit.

Are we sure this is still a problem?

I looked at the test to see if it would pass, but it is not even checking anything about lockfiles! It just checks that we exit 1 by returning up the callstack instead of calling die(). And of course it would not have a problem under Linux either way. But if I run something similar under strace, I see:

  $ strace ./git bundle create foobar.bundle HEAD..HEAD
  [...]
  openat(AT_FDCWD, "/home/peff/compile/git/foobar.bundle.lock", O_RDWR|O_CREAT|O_EXCL|O_CLOEXEC, 0666) = 3
  [...]
  close(3)                                = 0
  unlink("/home/peff/compile/git/foobar.bundle.lock") = 0
  exit_group(128)                         = ?

which seems right.

-Peff