Re: [PATCH 1/2] refs.c: add resolve_ref_submodule()

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Feb 9, 2017, 05:20 UTC
Message-ID: <19bcda56-077e-b0a8-4408-d0012754bf17@alum.mit.edu>
In-Reply-To: <20170208113144.8201-2-pclouds@gmail.com>

On 02/08/2017 12:31 PM, Nguyễn Thái Ngọc Duy wrote:

Show 67 quoted lines

> This is basically the extended version of resolve_gitlink_ref() where we
> have access to more info from the underlying resolve_ref_recursively() call.
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  refs.c | 20 ++++++++++++++------
>  refs.h |  3 +++
>  2 files changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/refs.c b/refs.c
> index cd36b64ed9..02e35d83f3 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1325,18 +1325,18 @@ const char *resolve_ref_unsafe(const char *refname, int resolve_flags,
>  				       resolve_flags, sha1, flags);
>  }
>  
> -int resolve_gitlink_ref(const char *submodule, const char *refname,
> -			unsigned char *sha1)
> +const char *resolve_ref_submodule(const char *submodule, const char *refname,
> +				  int resolve_flags, unsigned char *sha1,
> +				  int *flags)
>  {
>  	size_t len = strlen(submodule);
>  	struct ref_store *refs;
> -	int flags;
>  
>  	while (len && submodule[len - 1] == '/')
>  		len--;
>  
>  	if (!len)
> -		return -1;
> +		return NULL;
>  
>  	if (submodule[len]) {
>  		/* We need to strip off one or more trailing slashes */
> @@ -1349,9 +1349,17 @@ int resolve_gitlink_ref(const char *submodule, const char *refname,
>  	}
>  
>  	if (!refs)
> -		return -1;
> +		return NULL;
> +
> +	return resolve_ref_recursively(refs, refname, resolve_flags, sha1, flags);
> +}
> +
> +int resolve_gitlink_ref(const char *submodule, const char *refname,
> +			unsigned char *sha1)
> +{
> +	int flags;
>  
> -	if (!resolve_ref_recursively(refs, refname, 0, sha1, &flags) ||
> +	if (!resolve_ref_submodule(submodule, refname, 0, sha1, &flags) ||
>  	    is_null_sha1(sha1))
>  		return -1;
>  	return 0;
> diff --git a/refs.h b/refs.h
> index 9fbff90e79..74542468d8 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -88,6 +88,9 @@ int peel_ref(const char *refname, unsigned char *sha1);
>   */
>  int resolve_gitlink_ref(const char *submodule, const char *refname,
>  			unsigned char *sha1);
> +const char *resolve_ref_submodule(const char *submodule, const char *refname,
> +				  int resolve_flags, unsigned char *sha1,
> +				  int *flags);

This function is the analog of resolve_ref_unsafe(); i.e., it returns a pointer to either a static buffer or a pointer into the refname argument. Therefore, I think it should have "unsafe" in its name. And/or maybe there should be a safe version of the function analogous to resolve_refdup().

Moreover, this function has inherited the code for stripping trailing slashes from the submodule name. I have the feeling that this is a wart, not a feature, and that it would be sad to see it spread. How about moving the slash-stripping code to resolve_gitlink_ref() and making resolve_ref_submodule() assume that its submodule name is already clean?

It would be nice to have a docstring here.

I also have some higher-level concerns about the approach of this patch series, which I'll write about in a comment to patch 2/2.

Michael