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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Nov 15, 2018, 20:01 UTC
Message-ID: <nycvar.QRO.7.76.6.1811152059060.41@tvgsbejvaqbjf.bet>
In-Reply-To: <20181115164300.GA29290@sigill.intra.peff.net>

Hi Peff,

On Thu, 15 Nov 2018, Jeff King wrote:

Show 31 quoted lines

> On Thu, Nov 15, 2018 at 05:32:15PM +0100, Johannes Schindelin wrote:
> 
> > 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 --
> 
> It seems like we should be checking that the stale lockfile isn't left,
> which is the real problem (the warning is annoying, but is a symptom of
> the same thing). I.e., something like:
> 
>   test_must_fail git bundle create foobar.bundle master..master &&
>   test_path_is_missing foobar.bundle.lock
> 
> That would already pass on non-Windows platforms, but that's OK. It will
> never give a false failure.
> 
> If you don't mind, can you confirm that the test above fails without
> either of the two patches under discussion?

This test succeeds with your patch as well as with Gaël's, and fails when neither patch is applied. So you're right, it is the much better test.

> > Do you want to integrate this test into your patch and run with it, or
> > do you want me to shepherd your patch?
> 
> I'll wrap it up with a commit message and a test.

Thank you so much! Dscho