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