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?
[...]
> + 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.
[...]
> +
> 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;
}
errno is not used here, so no need to set errno = 0.
[...]
> @@ -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?
Thanks,
Jonathan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html