Re: [PATCH v2 04/20] refs_verify_refname_available(): implement once for all backends

> On Fri, Mar 31, 2017 at 9:11 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> It turns out that we can now implement
>> `refs_verify_refname_available()` based on the other virtual
>> functions, so there is no need for it to be defined at the backend
>> level. Instead, define it once in `refs.c` and remove the
>> `files_backend` definition.
>>
>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
>> ---
>>  refs.c               | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>>  refs.h               |  2 +-
>>  refs/files-backend.c | 39 +++++-------------------
> 
> Much appreciated. This will make future backends simpler to implement as well.
> 
>> +       iter = refs_ref_iterator_begin(refs, dirname.buf, 0,
>> +                                      DO_FOR_EACH_INCLUDE_BROKEN);
>> +       while ((ok = ref_iterator_advance(iter)) == ITER_OK) {
>> +               if (skip &&
>> +                   string_list_has_string(skip, iter->refname))
>> +                       continue;
>> +
>> +               strbuf_addf(err, "'%s' exists; cannot create '%s'",
>> +                           iter->refname, refname);
>> +               ok = ref_iterator_abort(iter);
> 
> Saving the return code in "ok" seems redundant because you don't use
> it. Or did you want to check that ok == ITER_DONE or die() too?