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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Nov 15, 2018, 12:57 UTC
Message-ID: <nycvar.QRO.7.76.6.1811151354150.41@tvgsbejvaqbjf.bet>
In-Reply-To: <20181115043409.GA3419@sigill.intra.peff.net>

Hi Peff,

On Wed, 14 Nov 2018, Jeff King wrote:

Show 38 quoted lines

> On Wed, Nov 14, 2018 at 02:08:48PM -0800, Stefan Beller wrote:
> 
> > 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.

Without the fix, the added regression test fails thusly:

-- snip -- [...] ++ test_expect_code 1 git bundle create foobar.bundle master..master ++ want_code=1 ++ shift ++ git bundle create foobar.bundle master..master fatal: Refusing to create empty bundle. warning: unable to unlink 'C:/git-sdk-64/usr/src/git/wip2/t/trash directory.t5607-clone-bundle/foobar.bundle.lock': Permission denied ++ exit_code=128 ++ test 128 = 1 ++ echo 'test_expect_code: command exited with 128, we wanted 1 git bundle create foobar.bundle master..master' test_expect_code: command exited with 128, we wanted 1 git bundle create foobar.bundle master..master ++ return 1 error: last command exited with $?=1 not ok 9 - try to create a bundle with empty ref count # # test_expect_code 1 git bundle create foobar.bundle master..master # -- snap --

So yes, we are trying to unlink the `.lock` file, and as far as I can tell that `unlink()` call comes from the tempfile cleanup asked for by Martin. However, as we still have a handle open to that file, that call fails.

I do not think that there is any better way to fix this than to close the file explicitly. If we tried to just close whatever file descriptor is still open to that file before deleting it, we would possibly cause problems in code that is still to be executed and assumes that it has a perfectly valid file descriptor. Besides, trying to do this kind of "automatically" won't work, like, at all, when it is one child process that holds an open file descriptor while another process wants to delete the file.

Ciao, Dscho