[PATCH 0/2] Fix an error-handling path when locking refs

[Michael Haggerty <mhagger@alum.mit.edu>]

In [1], Peff described a bug that he found that could cause a
reference transaction to be partly carried-out and partly not, with a
successful return. The scenario that he discussed was the deletion of
one reference and creation of another, where the two references D/F
conflict with each other.

But in fact the problem is more general; it can happen whenever a
reference deletion within a transaction is processed successfully, and
then another reference update in the same transaction fails in
`lock_ref_for_update()`. In such a case the failed update and any
subsequent ones could be silently ignored.

There is a longer explanation in the second patch's commit message.

The tests that I added probe a bunch of D/F update scenarios, which I
think should be characteristic of the scenarios that would trigger
this bug. It would be nice to have tests that examine other types of
failures (e.g., wrong `old_oid` values).

Bit since the fix is obviously a strict improvement and can prevent
data loss, and since the release process is already pretty far along,
I wanted to send this out ASAP. We can follow it up later with
additional tests.

These patches apply to current master. They are also available from my
GitHub fork [2] as branch `ref-locking-fix`.

While looking at this code again, I realized that the new code
rewrites the `packed-refs` file more often than did the old code.
Specifically, after dc39e09942 (files_ref_store: use a transaction to
update packed refs, 2017-09-08), the `packed-refs` file is overwritten
for any transaction that includes a reference delete. Prior to that
commit, `packed-refs` was only overwritten if a deleted reference was
actually present in the existing `packed-refs` file. This is even the
case if there was previously no `packed-refs` file at all; now any
reference deletion causes an empty `packed-refs` file to be created.

I think this is a conscionable regression, since deleting references
that are purely loose is probably not all that common, and the old
code had to read the whole `packed-refs` file anyway. Nevertheless, it
is obviously something that I would like to fix (though maybe not for
2.15? I'm open to input about its urgency.)

[1] https://public-inbox.org/git/20171024082409.smwsd6pla64jjlua@sigill.intra.peff.net/
[2] https://github.com/mhagger/git

Michael Haggerty (2):
  t1404: add a bunch of tests of D/F conflicts
  files_transaction_prepare(): fix handling of ref lock failure

 refs/files-backend.c         |   2 +-
 t/t1404-update-ref-errors.sh | 146 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 147 insertions(+), 1 deletion(-)

-- 
2.14.1