Re: [PATCH v2 1/3] refs: allow callers to supply old OIDs for batch deletion

Maciej Ciemborowicz <maciej.ciemborowicz@gmail.com> writes:

I had to read this sentence three times and still couldn't guess what it wanted to say. I _think_ the code is passing a list of refnames, and your new parameter that is oid_array serves as a parallel list, where the ref, identified by the Nth element of the list of refnames, is protected from deletion with the Nth element of the list of oids in such a way that ref is not removed unless it points at the specified object. You'd need to find a concise way to tell that story instead of the above sentence that does not give readers any meaningful information.

OK. So it is not end-users' but calling code's responsibility to ensure that the optional list of object names have exactly the same number of entries as the list of refs.

And this is as before. Shouldn't the new test above be placed below this? After all, if we are removing no refs, we really do not care what garbage is in the old oids array---we won't even look at it.

To me, this reads more like "We want to make sure that each deletion is independent and philosophically each of them should belong in separate transactions so that even when some fails the rest would proceed. Luckily, the current API does not allow you to check the current value to protect refs from deletion, so we can cram all delete operations in a single transaction and still claim that we are not making it all-or-none!". Natural continuation of that argument is "If we ever extend the API so that refs are optionally protected from deletion, we can get into a situation where some refs can be successfully removed while others cannot. Keeping everything in a single transaction WILL BECOME A WRONG DESIGN CHOICE when it happens."

And this new code is doing exactly that, making all the deletions, of possibly unrelated refs, into an all-or-none matter.

Don't we need to have separate transactions to delete each ref to retain the "delete them independently" semantics? If the caller (e.g., "git fetch --prune" without "--atomic") wants to delete 1000 refs, and a single ref fails its old-oid check due to a concurrent update, none of the 1000 refs will be removed and the transaction would be aborted. <refs.h> explains this function like so:

    /*
     * Delete the specified references. If there are any problems, emit
     * errors but attempt to keep going (i.e., the deletes are not done in
     * an all-or-nothing transaction). msg and flags are passed through to
     * ref_transaction_delete().
     */
    int refs_delete_refs(struct ref_store *refs, const char *msg,
                         struct string_list *refnames, unsigned int flags);

because we want to avoid exactly such a failure mode.

I do not offhand remember if our ref transactions have a mode where it acts more like a glorified "batch" job and commit does not necessarily require everything succeeding, but if it do, then it is OK to keep using a single transaction but to run it in such a "best effort" mode.