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

From: Jeff King <peff@peff.net>
Date: Nov 15, 2018, 16:43 UTC
Message-ID: <20181115164300.GA29290@sigill.intra.peff.net>
In-Reply-To: <nycvar.QRO.7.76.6.1811151725310.41@tvgsbejvaqbjf.bet>

On Thu, Nov 15, 2018 at 05:32:15PM +0100, Johannes Schindelin wrote:

Show 14 quoted lines

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

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

-Peff