Re: [PATCH v4 15/16] refs.c: remove fallback-to-main-store code get_submodule_ref_store()

[Michael Haggerty]

On 08/23/2017 02:37 PM, Nguyễn Thái Ngọc Duy wrote:
> At this state, there are three get_submodule_ref_store() callers:
> 
>  - for_each_remote_ref_submodule()
>  - handle_revision_pseudo_opt()
>  - resolve_gitlink_ref()
> 
> The first two deal explicitly with submodules (and we should never fall
> back to the main ref store as a result). They are only called from
> submodule.c:
> 
>  - find_first_merges()
>  - submodule_needs_pushing()
>  - push_submodule()
> 
> The last one, as its name implies, deals only with submodules too, and
> the "submodule" (path) argument must be a non-NULL, non-empty string.
> 
> So, this "if NULL or empty string" code block should never ever
> trigger. And it's wrong to fall back to the main ref store
> anyway. Delete it.

Nice! Thanks for the cleanup.

Michael

[PATCH v4 15/16] refs.c: remove fallback-to-main-store code get_submodule_ref_store()

[Nguyễn Thái Ngọc Duy]

At this state, there are three get_submodule_ref_store() callers:

 - for_each_remote_ref_submodule()
 - handle_revision_pseudo_opt()
 - resolve_gitlink_ref()

The first two deal explicitly with submodules (and we should never fall
back to the main ref store as a result). They are only called from
submodule.c:

 - find_first_merges()
 - submodule_needs_pushing()
 - push_submodule()

The last one, as its name implies, deals only with submodules too, and
the "submodule" (path) argument must be a non-NULL, non-empty string.

So, this "if NULL or empty string" code block should never ever
trigger. And it's wrong to fall back to the main ref store
anyway. Delete it.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs.c | 11 +++
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/refs.c b/refs.c
index 8c989ffec7..a0c5078901 100644
--- a/refs.c
+++ b/refs.c
@@ -1587,6 +1587,9 @@ struct ref_store *get_submodule_ref_store(const char 
*submodule)
char *to_free = NULL;
size_t len;
 
+   if (!submodule)
+   return NULL;
+
if (submodule) {
len = strlen(submodule);
while (len && is_dir_sep(submodule[len - 1]))
@@ -1595,14 +1598,6 @@ struct ref_store *get_submodule_ref_store(const char 
*submodule)
return NULL;
}
 
-   if (!submodule || !*submodule) {
-   /*
-* FIXME: This case is ideally not allowed. But that
-* can't happen until we clean up all the callers.
-*/
-   return get_main_ref_store();
-   }
-
if (submodule[len])
/* We need to strip off one or more trailing slashes */
submodule = to_free = xmemdupz(submodule, len);
-- 
2.11.0.157.gd943d85