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/[email protected]/
[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