On 04/07/2017 01:20 PM, Duy Nguyen wrote:
> On Fri, Mar 31, 2017 at 9:11 PM, Michael Haggerty <mhagger@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
>> ---
>> 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?
True, setting `ok` here is redundant. I don't think there's much point
worrying about whether `ref_iterator_abort()` fails here, since we've
already gotten the information that we require.
I'll remove it.
Thanks,
Michael