Re: [PATCH v11 08/41] refs.c: add an err argument to delete_ref_loose

> Hi,
>
> Comments from http://marc.info/?l=git&m=140079653930751&w=2:
>
> Ronnie Sahlberg wrote:
>
> [...]
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -2491,17 +2491,43 @@ 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 add_err_if_unremovable(const char *op, const char *file,
>> +                               struct strbuf *e, int rc)
>> +{
>> +     int err = errno;
>> +     if (rc < 0 && err != ENOENT) {
>> +             strbuf_addf(e, "unable to %s %s: %s",
>> +                         op, file, strerror(errno));
>> +             errno = err;
>> +     }
>> +     return rc;
>> +}
>> +
>> +static int unlink_or_err(const char *file, struct strbuf *err)
>> +{
>> +     if (err)
>> +             return add_err_if_unremovable("unlink", file, err,
>> +                                           unlink(file));
>> +     else
>> +             return unlink_or_warn(file);
>> +}
>
> The new unlink_or_err has an odd contract when the err argument is passed.
> On error:
>
>  * if errno != ENOENT, it will append a message to err and return -1 (good)
>  * if errno == ENOENT, it will not append a message to err but will
>    still return -1.
>
> This means the caller has to check errno to figure out whether err is
> meaningful when it returns an error.
>
> Perhaps it should return 0 when errno == ENOENT.  After all, in that
> case the file does not exist any more, which is all we wanted.

>
>
>> +
>> +static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf *err)
>>  {
>>       if (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {
>>               /* loose */
>> -             int err, i = strlen(lock->lk->filename) - 5; /* .lock */
>> +             int res, i = strlen(lock->lk->filename) - 5; /* .lock */
>>
>>               lock->lk->filename[i] = 0;
>> -             err = unlink_or_warn(lock->lk->filename);
>> +             res = unlink_or_err(lock->lk->filename, err);
>>               lock->lk->filename[i] = '.';
>> -             if (err && errno != ENOENT)
>> +             if (res && errno != ENOENT) {
>> +                     if (err)
>> +                             strbuf_addf(err,
>> +                                         "failed to delete loose ref '%s'",
>> +                                         lock->lk->filename);
>
> On failure we seem to add our own message to err, too, so the
> resulting message would be something like
>
>         fatal: unable to unlink .git/refs/heads/master: \
>         Permission deniedfailed to delete loose ref '.git/refs/heads/master.lock'
>
> Is the second strbuf_addf a typo?