On Fri, May 16, 2014 at 3:52 PM, Jonathan Nieder jrnie...@gmail.com wrote:
Ronnie Sahlberg wrote:
Change store_updated_refs to use a single ref transaction for all refs that
are updated during the fetch. This makes the fetch more atomic when update
failures occur.
Fun.
[...]
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
[...]
@@ -373,27 +374,13 @@ static int s_update_ref(const char *action,
struct ref *ref,
int check_old)
{
- char msg[1024];
- char *rla = getenv(GIT_REFLOG_ACTION);
- struct ref_transaction *transaction;
Previously fetch respected GIT_REFLOG_ACTION, and this is dropping
that support. Intended?
I think it was added back later in the series when
ref_transaction_update started taking a msg argument.
I have reordered the patches in the ref-transactions so that the fetch
updates come after we add msg support to _update.
Please see current builtin/fetch.c in my ref-transactions branch.
[...]
+ if (ref_transaction_update(transaction, ref-name, ref-new_sha1,
+ref-old_sha1, 0, check_old))
+ return STORE_REF_ERROR_OTHER;
Should pass a strbuf in to get a message back.
I added err strbuf to both update and commit and now also error(%s,
err.buf) on failure.
[...]
+
return 0;
}
@@ -565,6 +552,13 @@ static int store_updated_refs(const char *raw_url,
const char *remote_name,
goto abort;
}
+ errno = 0;
+ transaction = ref_transaction_begin();
+ if (!transaction) {
+ rc = error(_(cannot start ref transaction\n));
+ goto abort;
error() appends a newline on its own, so the message shouldn't
end with newline.
More importantly, this message isn't helpful without more detail about
why _transaction_begin() failed. The user doesn't know what
error: cannot start ref transaction
means.
error: cannot connect to mysqld for ref storage: [etc]
would tell what they need to know. That is:
transaction = ref_transaction_begin(err);
if (!transaction) {
rc = error(%s, err.buf);
goto abort;
}
I add this in the next patch series. Right now ref_transaction_begin
can never return failure back to the caller.
errno is not used here, so no need to set errno = 0.
removed.
[...]
@@ -676,6 +670,10 @@ static int store_updated_refs(const char *raw_url,
const char *remote_name,
}
}
}
+ if ((ret = ref_transaction_commit(transaction, NULL)))
+ rc |= ret == -2 ? STORE_REF_ERROR_DF_CONFLICT :
+ STORE_REF_ERROR_OTHER;
(I cheated and stole the newer code from your repo.)
Yay! Style nit: git avoids assignments in if conditionals.
ret = ref_tr...
if (ret == -2)
rc |= ...
else if (ret)
rc |= ...
Does _update need the too as futureproofing for backends that can
detect the name conflict sooner?
Yes it will. I will update the next series to do this. If _update
fails with name conflict it will return -2.
Also it will store the status in the transaction so that a later call
to _commit will also return -2.
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
