Re: [PATCH v8 24/44] fetch.c: use a single ref transaction for all ref updates
- From
Jonathan Nieder <jrnieder@gmail.com>
- Date
- May 16, 2014, 22:52 UTC
- Message-ID
- <20140516225211.GI12314@google.com>
- In-Reply-To
- <1400174999-26786-25-git-send-email-sahlberg@google.com>
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
[...]
Show 7 quoted lines
> @@ -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.
[...]
Show 13 quoted lines
> +
> 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.
[...]
Show 7 quoted lines
> @@ -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