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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Peff,

On Thu, 15 Nov 2018, Jeff King wrote:

> On Thu, Nov 15, 2018 at 01:57:45PM +0100, Johannes Schindelin wrote:
> 
> > > 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
> 
> Hmph. So who has it open, and why isn't the tempfile code working as
> designed?
> 
> Aha, I see the problem. We dup() the descriptor in create_bundle(). So
> the patch _is_ necessary and (fairly) correct. But the explanation
> probably ought to be something like:
> 
>   In create_bundle(), we duplicate the lockfile descriptor via dup().
>   This means that even though the lockfile code carefully calls close()
>   before unlinking the lockfile, we may still have the file open. Unix
>   systems don't care, but under Windows, this prevents the unlink
>   (causing an annoying warning and a stale lockfile).
> 
> But that also means that all of the other places we could die (e.g., in
> write_or_die) are going to have the same problem. We've fixed only one.
> Is there a way we can avoid doing the dup() in the first place?
> 
> The comment there explains that we duplicate because write_pack_data()
> will close the descriptor. Unfortunately, that's hard to change because
> it comes from run-command. But we don't actually need the descriptor
> ourselves after it's closed; we're just trying to appease the lockfile
> code; see e54c347c1c (create_bundle(): duplicate file descriptor to
> avoid closing it twice, 2015-08-10).
> 
> We just need some reasonable way of telling the lock code what's
> happening. Something like the patch below, which is a moral revert of
> e54c347c1c, but instead wrapping the "lock->fd = -1" in an official API.
> 
> Does this make your warning go away?
> 
> diff --git a/bundle.c b/bundle.c
> [...]

I cannot claim that I wrapped my head around your explanation or your diff (I am
busy trying to prepare Git for Windows' `master` for rebasing to v2.20.0-rc0),
but it does fix the problem. Thank you so much!

The line `test_expect_code 1 ...` needs to be adjusted to `test_expect_code
128`, of course, and to discern from the fixed problem (which also exits with
code 128), the error output should be verified, like so:

-- snip --
test_expect_success 'try to create a bundle with empty ref count' '
	test_must_fail git bundle create foobar.bundle master..master 2>err &&
	test_i18ngrep "Refusing to create empty bundle" err
'
-- snap --

Do you want to integrate this test into your patch and run with it, or do you
want me to shepherd your patch?

Ciao,
Dscho