On 03/01/2017 01:00 PM, Duy Nguyen wrote:
> On Wed, Mar 1, 2017 at 1:03 AM, Michael Haggerty <mhagger@xxxxxxxxxxxx> 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