Re: [PATCH v6 11/42] tag.c: use ref transactions when doing updates

From: Ronnie Sahlberg <sahlberg@google.com>
To: Jonathan Nieder <jrnieder@gmail.com>
Cc: "git@vger.kernel.org" <git@vger.kernel.org>,
	Michael Haggerty <mhagger@alum.mit.edu>
Subject: Re: [PATCH v6 11/42] tag.c: use ref transactions when doing updates
Date: Thu, 15 May 2014 09:45:01 -0700	[thread overview]
Message-ID: <CAL=YDWk8uZzJyex0YoBx81KhJZjS99qW1nV2AOe=rZhgoYRAcg@mail.gmail.com> (raw)
In-Reply-To: <20140515002738.GH9218@google.com>

On Wed, May 14, 2014 at 5:27 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Ronnie Sahlberg wrote:
>
>> --- a/builtin/tag.c
>> +++ b/builtin/tag.c
>> @@ -701,11 +702,12 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>>       if (annotate)
>>               create_tag(object, tag, &buf, &opt, prev, object);
>>
>> -     lock = lock_any_ref_for_update(ref.buf, prev, 0, NULL);
>> -     if (!lock)
>> -             die(_("%s: cannot lock the ref"), ref.buf);
>> -     if (write_ref_sha1(lock, object, NULL) < 0)
>> -             die(_("%s: cannot update the ref"), ref.buf);
>> +     transaction = ref_transaction_begin();
>> +     if (!transaction ||
>> +         ref_transaction_update(transaction, ref.buf, object, prev,
>> +                                0, !is_null_sha1(prev)) ||
>> +         ref_transaction_commit(transaction, NULL, &err))
>> +             die(_("%s: cannot update the ref: %s"), ref.buf, err.buf);
>
> Makes sense for the _update and _commit case.  (BTW, why is have_old
> a separate boolean instead of a bit in flags?)
>
> For the _begin() case, can ref_transaction_begin() ever fail?  xcalloc
> die()s on allocation failure.  So I think it's fine to assume
> transaction is non-null (i.e., drop the !transaction condition), or if
> you want to be defensive, then label it as a bug --- e.g.:
>
>         if (!transaction)
>                 die("BUG: ref_transaction_begin() returned NULL?");
>
> Otherwise if ref_transaction_begin regresses in the future and this
> case is tripped then the message would be
>
>         fatal: refs/tags/v1.0: cannot update the ref:
>
> which is not as obvious an indicator that the user should contact
> the mailing list.

For the current refs implementation, _begin can never return NULL
since the only failure mode would be OOM in which case we die().
And then for that case we could remove the !transaction check since transaction
can never be NULL here.

(I am not a big fan of die() in general)

However, if we implement a different datastore for refs in the future
it is likely that
the ref_transaction_begin equivalent for that backend could well start returning
failures for a lot other reasons than just OOM.

I could imagine that tdb_transaction_start() could fail for a
corrupted database.
An SQL based backend could fail due to the client library failing to
open a socket to the db,
etc.

But you bring a good point about the error message.

Instead of the suggestions above, would you accept an alternative
approach where I would
add an err argument to ref_transaction_begin() instead?

For a hypothetical mysql backend, this could then do something like :

>> +     transaction = ref_transaction_begin();
>> +     if (!transaction ||
>> +         ref_transaction_update(transaction, ref.buf, object, prev,
>> +                                0, !is_null_sha1(prev)) ||
>> +         ref_transaction_commit(transaction, NULL, &err))
>> +             die(_("%s: cannot update the ref: %s"), ref.buf, err.buf);

Which could then result in output like

fatal: refs/heads/master: cannot update the ref: failed to connect to
mysql database ...

So I suggest that instead of doing these changes I will add an err
argument to ref_transaction_begin.
Does that sound ok with you?

>
> Thanks,
> Jonathan