Re: [PATCH v3 05/12] refs: move submodule slash stripping code to get_submodule_ref_store

[Michael Haggerty <mhagger@alum.mit.edu>]

On 04/19/2017 01:01 PM, Nguyễn Thái Ngọc Duy wrote:
> This is a better place that will benefit all submodule callers instead
> of just resolve_gitlink_ref()
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  refs.c | 33 +++++++++++++++++----------------
>  1 file changed, 17 insertions(+), 16 deletions(-)
> 
> diff --git a/refs.c b/refs.c
> index 5902a3d9e5..26474cb62a 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1422,25 +1422,10 @@ const char *resolve_ref_unsafe(const char *refname, int resolve_flags,
>  int resolve_gitlink_ref(const char *submodule, const char *refname,
>  			unsigned char *sha1)
>  {
> -	size_t len = strlen(submodule);
>  	struct ref_store *refs;
>  	int flags;
>  
> -	while (len && submodule[len - 1] == '/')
> -		len--;
> -
> -	if (!len)
> -		return -1;
> -
> -	if (submodule[len]) {
> -		/* We need to strip off one or more trailing slashes */
> -		char *stripped = xmemdupz(submodule, len);
> -
> -		refs = get_submodule_ref_store(stripped);
> -		free(stripped);
> -	} else {
> -		refs = get_submodule_ref_store(submodule);
> -	}
> +	refs = get_submodule_ref_store(submodule);
>  
>  	if (!refs)
>  		return -1;
> @@ -1558,7 +1543,17 @@ struct ref_store *get_submodule_ref_store(const char *submodule)
>  {
>  	struct strbuf submodule_sb = STRBUF_INIT;
>  	struct ref_store *refs;
> +	char *to_free = NULL;
>  	int ret;
> +	size_t len;
> +
> +	if (submodule) {
> +		len = strlen(submodule);
> +		while (len && submodule[len - 1] == '/')
> +			len--;
> +		if (!len)
> +			submodule = NULL;
> +	}

Ugh. Should a submodule named "///" *really* be considered to refer to
the main ref_store? I understand that's what the code did before this
patch, but it seems to me more like an accident of the old design rather
than something worth supporting. In other words, if a caller would
really pass us such a string, it seems like we could declare the caller
buggy, no?

> [...]

Otherwise, looks good and makes a lot of sense.

Michael