From: Ronnie Sahlberg <sahlberg@google.com>
Date: Mon, 14 Jul 2014 18:49:00 GMT
Subject: Re: [PATCH v20 30/48] refs.c: change update_ref to use a transaction
Message-ID: <CAL=YDWnoXBXKaEFV8F_gGEALY3A4Bx3OKmHe8h4=VQgPN+me=Q@mail.gmail.com>
In-Reply-To: <53BBEA04.5040103@alum.mit.edu>

On Tue, Jul 8, 2014 at 5:54 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> On 06/20/2014 04:43 PM, Ronnie Sahlberg wrote:
>> Change the update_ref helper function to use a ref transaction internally.
>>
>> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
>> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
>> ---
>>  refs.c | 28 ++++++++++++++++++++++++----
>>  1 file changed, 24 insertions(+), 4 deletions(-)
>>
>> diff --git a/refs.c b/refs.c
>> index 8c695ba..4bdccc5 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -3524,11 +3524,31 @@ int update_ref(const char *action, const char *refname,
>>              const unsigned char *sha1, const unsigned char *oldval,
>>              int flags, enum action_on_err onerr)
>>  {
>> -     struct ref_lock *lock;
>> -     lock = update_ref_lock(refname, oldval, flags, NULL, onerr);
>> -     if (!lock)
>> +     struct ref_transaction *t;
>> +     struct strbuf err = STRBUF_INIT;
>> +
>> +     t = ref_transaction_begin(&err);
>> +     if (!t ||
>> +         ref_transaction_update(t, refname, sha1, oldval, flags,
>> +                                !!oldval, &err) ||
>> +         ref_transaction_commit(t, action, &err)) {
>> +             const char *str = "update_ref failed for ref '%s': %s";
>> +
>> +             ref_transaction_free(t);
>> +             switch (onerr) {
>> +             case UPDATE_REFS_MSG_ON_ERR:
>> +                     error(str, refname, err.buf);
>> +                     break;
>> +             case UPDATE_REFS_DIE_ON_ERR:
>> +                     die(str, refname, err.buf);
>> +                     break;
>> +             case UPDATE_REFS_QUIET_ON_ERR:
>> +                     break;
>> +             }
>> +             strbuf_release(&err);
>>               return 1;
>> -     return update_ref_write(action, refname, sha1, lock, NULL, onerr);
>> +     }
>> +     return 0;
>>  }
>>
>
> Should this function be scheduled for the "take strbuf *err argument"
> treatment instead of continuing to use an action_on_err parameter?
> (Maybe you've changed this later in the patch series?)
>
> I'm not saying this change has to be part of the current patch series,
> but let's consider it for the future.

There is a patch that does that in a later series. At that stage we
get rid of all action_on_err arguments.


>
> Michael
>
> --
> Michael Haggerty
> mhagger@alum.mit.edu
>