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

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

On Wed, Mar 1, 2017 at 1:03 AM, Michael Haggerty [off-list ref] wrote:

quoted

quoted

 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