Re: [PATCH v8 06/44] refs.c: add an err argument ro delete_loose_ref

From: Ronnie Sahlberg <sahlberg@google.com>
Date: May 15, 2014, 20:00 UTC
Message-ID: <CAL=YDWkE4WnE1ELRJcXFEcNhSw=gv9XjqvrK_EqD3PQu8i0URA@mail.gmail.com>
In-Reply-To: <20140515190434.GF26471@google.com>

On Thu, May 15, 2014 at 12:04 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

Show 19 quoted lines

> Ronnie Sahlberg wrote:
>
>> [Subject: refs.c: add an err argument ro delete_loose_ref]
>
> s/ro/to/
> s/delete_loose_ref/delete_ref_loose/
>
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -2484,17 +2484,22 @@ static int repack_without_ref(const char *refname)
>>       return repack_without_refs(&refname, 1, NULL);
>>  }
>>
>> -static int delete_ref_loose(struct ref_lock *lock, int flag)
>> +static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf *err)
>
> Should this get an onerr flag to suppress the message to stderr
> or unconditionally suppress it when err != NULL?
>

Fixed. I added a new function unlink_or_err that will update err if non-NULL and unse warning() otherwise.

Show 25 quoted lines

> [...]
>>               lock->lk->filename[i] = 0;
>> -             err = unlink_or_warn(lock->lk->filename);
>> +             res = unlink_or_warn(lock->lk->filename);
>
> It seems like in the new error handling scheme there should be a new
> variant on wrapper.c's warn_if_unremovable:
>
>         static int add_err_if_unremovable(const char *op, const char *file, struct strbuf *err, int rc)
>         {
>                 int err = errno;
>                 if (rc < 0 && err != ENOENT) {
>                         strbuf_addf(err, "unable to %s %s: %s",
>                                     op, file, strerror(errno));
>                         errno = err;
>                 }
>                 return rc;
>         }
>
>         static int unlink_or_err(const char *file, struct strbuf *err)
>         {
>                 return add_err_if_unremovable("unlink", file, err, unlink(file));
>         }
>
>                 res = unlink_or_err(lock->lk->filename, err);