On Fri, May 16, 2014 at 3:52 PM, Jonathan Nieder <[email protected]> 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
