Re: [PATCH v8 24/44] fetch.c: use a single ref transaction for all ref updates

From: Ronnie Sahlberg <sahlberg@google.com>
Date: May 19, 2014, 16:56 UTC
Message-ID: <CAL=YDWkmYxTm_RJ2O0Wt0xW8MxsmyZD2nDa=4_p9wQYKFPTvVw@mail.gmail.com>
In-Reply-To: <20140516225211.GI12314@google.com>

On Fri, May 16, 2014 at 3:52 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

Show 22 quoted lines

> 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.

Show 7 quoted lines

>
> [...]
>> +     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.

Show 36 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;
>         }
>

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.

Show 22 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?

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