Re: [PATCH v5 12/24] refs.c: kill register_ref_store(), add register_submodule_ref_store()

[Michael Haggerty]

On 03/01/2017 01:00 PM, Duy Nguyen wrote:
> On Wed, Mar 1, 2017 at 1:03 AM, Michael Haggerty <mhag...@alum.mit.edu> wrote:
>>>  struct ref_store *get_ref_store(const char *submodule)
>>>  {
>>>       struct strbuf submodule_sb = STRBUF_INIT;
>>> @@ -1480,6 +1479,9 @@ struct ref_store *get_ref_store(const char *submodule)
>>>       if (is_nonbare_repository_dir(&submodule_sb))
>>>               refs = ref_store_init(submodule);
>>>       strbuf_release(&submodule_sb);
>>> +
>>> +     if (refs)
>>
>> I think `refs` should always be non-NULL here for the same reason.
> 
> That's true if is_nonbar_repo... returns true. If it's false (e.g.
> uninitialized submodule) then refs remains NULL from before (I didn't
> know about this until I hit a segfault in rev-list in another series)

Oh, yes, true.

But given that, I think the code would be clearer if the two calls were
in the same if; i.e.,

        refs = lookup_submodule_ref_store(submodule);
        if (refs)
                return refs;

        strbuf_addstr(&submodule_sb, submodule);
        if (is_nonbare_repository_dir(&submodule_sb)) {
                refs = ref_store_init(submodule);
                register_submodule_ref_store(refs, submodule);
        }
        strbuf_release(&submodule_sb);

        return refs;

or even the `if (!is_nonbare_repository_dir(...)) goto cleanup;` pattern
to make it clearer that this is an error return.

Michael