On Wed, May 14, 2014 at 5:27 PM, Jonathan Nieder <jrnie...@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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html