Re: [PATCH 11/11] refs: split and make get_*_ref_store() public API

On Tue, Feb 14, 2017 at 2:04 AM, Duy Nguyen [off-list ref] wrote:

On Tue, Feb 14, 2017 at 6:55 AM, Stefan Beller [off-list ref] wrote:

quoted

quoted

+/*
+ * Return the ref_store instance for the specified submodule. For the
+ * main repository, use submodule==NULL; such a call cannot fail.

So now we have both a get_main as well as a get_submodule function,
but the submodule function can return the main as well?

I'd rather see this as a BUG; or asking another way:
What is the difference between get_submodule_ref_store(NULL)
and get_main_ref_store() ?

Technical debts :) In order to do that, we need to make sure
_submodule() never takes NULL as main repo. But current code does. On
example is revision.c where submodule==NULL is the main repo. In the
end, I agree that get_submodule_ref_store(NULL) should be a bug.

quoted

As you went through all call sites (by renaming the function), we'd
be able to tell that there is no caller with NULL, or is it?

Direct call sites are just middle men though, e.g.
for_each_ref_submodule(). I'll attempt to clean this up at some point
in future (probably at the same time I attempt to kill *_submodule ref
api). But I think for now I'll just put a TODO or FIXME comment here.

ok, fine with me.

My line of thinking when originally responding was to put the FIXME
comment at the caller site, and there we'd differentiate between the
two cases and call the appropriate function.

--
Duy