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

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(-)