Re: [PATCH v11 32/41] refs.c: make delete_ref use a transaction

From: Ronnie Sahlberg <sahlberg@google.com>
Date: Jun 3, 2014, 16:59 UTC
Message-ID: <CAL=YDW=W4V-Eb8nK5dx_dKyVHuKDwvGJhLihosgmGZx8kO-O2w@mail.gmail.com>
In-Reply-To: <20140530172808.GG12314@google.com>

Thanks!

On Fri, May 30, 2014 at 10:28 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:

Show 38 quoted lines

> Ronnie Sahlberg wrote:
>
>> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
>> ---
>>  refs.c | 34 +++++++++++++---------------------
>>  1 file changed, 13 insertions(+), 21 deletions(-)
>
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
>
> [...]
>> +++ b/refs.c
> [...]
>> @@ -2542,24 +2537,21 @@ static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf *err)
>>
>>  int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
>>  {
> [...]
>> +     if (!transaction ||
>> +         ref_transaction_delete(transaction, refname, sha1, delopt,
>> +                                sha1 && !is_null_sha1(sha1), &err) ||
>> +         ref_transaction_commit(transaction, NULL, &err)) {
>> +             error("%s", err.buf);
>> +             ref_transaction_free(transaction);
>> +             strbuf_release(&err);
>>               return 1;
>> +     }
> [...]
>> -     ret |= repack_without_ref(lock->ref_name);
>
> The old return value could be 1 or -1 depending on how the deletion
> failed.  Now it's consistently 1.
>
> The only callers I see that care are cmd_symbolic_ref and
> cmd_update_ref, for which 1 is better (-1 would result in an exit
> status of 255, which means something like "died with signal 127").
>
> Thanks,
> Jonathan