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

[Junio C Hamano <gitster@pobox.com>]

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

> refs_delete_refs() currently performs unconditional deletions. Thus callers
> cannot preserve old values that they have already resolved, and
> reference-transaction hooks consequently see a null old OID.
>
> Add an optional oid_array whose entries correspond to the refnames.

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.

>  int refs_delete_refs(struct ref_store *refs, const char *logmsg,
> -		     struct string_list *refnames, unsigned int flags)
> +		     struct string_list *refnames,
> +		     const struct oid_array *old_oids,
> +		     unsigned int flags)
>  {
>  	struct ref_transaction *transaction;
>  	struct strbuf err = STRBUF_INIT;
> -	struct string_list_item *item;
> +	size_t i;
>  	int ret = 0, failures = 0;
>  	char *msg;
>  
> +	if (old_oids && old_oids->nr != refnames->nr)
> +		BUG("refname and old OID counts do not match");

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.

>  	if (!refnames->nr)
>  		return 0;

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.

>  	msg = normalize_reflog_message(logmsg);
>  
> -	/*
> -	 * Since we don't check the references' old_oids, the
> -	 * individual updates can't fail, so we can pack all of the
> -	 * updates into a single transaction.
> -	 */

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.

>  	transaction = ref_store_transaction_begin(refs, 0, &err);
>  	if (!transaction) {
>  		ret = error("%s", err.buf);
>  		goto out;
>  	}
>  
> -	for_each_string_list_item(item, refnames) {
> +	for (i = 0; i < refnames->nr; i++) {
> +		struct string_list_item *item = &refnames->items[i];
> +		const struct object_id *old_oid = old_oids ? &old_oids->oid[i] : NULL;
> +
> +		if (old_oid && is_null_oid(old_oid))
> +			old_oid = NULL;
>  		ret = ref_transaction_delete(transaction, item->string,
> -					     NULL, NULL, flags, msg, &err);
> +					     old_oid, NULL, flags, msg, &err);
>  		if (ret) {
>  			warning(_("could not delete reference %s: %s"),
>  				item->string, err.buf);