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

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Mar 1, 2017, 12:31 UTC
Message-ID: <f0821bd8-dda5-1b11-e66f-8fa4ab169778@alum.mit.edu>
In-Reply-To: <CACsJy8BvKvo8_bRHH++dp53DaPbDVkKU0EUQC7ofYvBo+Tec0Q@mail.gmail.com>

On 03/01/2017 01:00 PM, Duy Nguyen wrote:

Show 16 quoted lines

> On Wed, Mar 1, 2017 at 1:03 AM, Michael Haggerty <mhagger@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