Re: [GSoC] [PATCH v4 1/8] submodule--helper: add options for compute_submodule_clone_url()

[Kaartic Sivaraam <kaartic.sivaraam@gmail.com>]

On 08/08/21 11:11 pm, Kaartic Sivaraam wrote:
> On 07/08/21 12:46 pm, Atharva Raykar wrote:
>> Let's modify the interface to `compute_submodule_clone_url()` function
>> by adding two more arguments, so that we can reuse this in various parts
>> of `submodule--helper.c` that follow a common pattern, which is--read
>> the remote url configuration of the superproject and then call
>> `relative_url()`.
>>
>> This function is nearly identical to `resolve_relative_url()`, the only
>> difference being the extra warning message. We can add a quiet flag to
>> it, ...
> 
> It took me a while to figure what "it" meant in the above sentence. Does it
> refer to `compute_submodule_clone_url` or `resolve_relative_url`. After one
> sees the patch and takes a look at `resolve_relative_url`, it's clear the "it"
> indeed does refer to `resolve_relative_url`. But it might worth clarifying this
> in the commit message itself.
> 
> Certainly not worth a re-roll on its own. May be Junio could amend this while queing ?
> 
Actually, I just noticed two other things which might be re-roll worthy. Read on ...

> -static char *compute_submodule_clone_url(const char *rel_url)
> +static char *compute_submodule_clone_url(const char *rel_url, const char *up_path, int quiet)
>  {
>  	char *remoteurl, *relurl;

I know this isn't new code. But there's already an argument names
'rel_url'. So, a variable named 'relurl' in the same scope is making it
hard to distinguish between these two. Could you also try distinguishing
these better by renaming 'relurl' to 'res' or something else?

>  	char *remote = get_default_remote();
> @@ -598,10 +598,14 @@ static char *compute_submodule_clone_url(const char *rel_url)
>  
>  	strbuf_addf(&remotesb, "remote.%s.url", remote);
>  	if (git_config_get_string(remotesb.buf, &remoteurl)) {
> -		warning(_("could not look up configuration '%s'. Assuming this repository is its own authoritative upstream."), remotesb.buf);
> +		if (!quiet)
> +			warning(_("could not look up configuration '%s'. "
> +				  "Assuming this repository is its own "
> +				  "authoritative upstream."),
> +				remotesb.buf);
>  		remoteurl = xgetcwd();
>  	}
> -	relurl = relative_url(remoteurl, rel_url, NULL);
> +	relurl = relative_url(remoteurl, rel_url, up_path);

After reading 2/8 of the series, I just noticed that 'remoteurl' is always
initialized in 'resolve_realtive_url'. It is either initialized to the return
value of 'xgetcwd' or retains its assigned value of 'NULL'. But it looks
like that's not the case here. 'remoteurl' could be used uninitialized
when the above if block does not get executed which in turn could result in
weird behaviour in case 'remoteurl' gets a value of anything other than 'NULL'
at runtime.

This again has nothing to do with the change done in this patch. Regardless, it
looks like something worth correcting. Thus, I thought of pointing it out.

>  
>  	free(remote);
>  	free(remoteurl);
> @@ -660,7 +664,7 @@ static void init_submodule(const char *path, const char *prefix,
>  		if (starts_with_dot_dot_slash(url) ||
>  		    starts_with_dot_slash(url)) {
>  			char *oldurl = url;
> -			url = compute_submodule_clone_url(oldurl);
> +			url = compute_submodule_clone_url(oldurl, NULL, 0);
>  			free(oldurl);
>  		}
>


-- 
Sivaraam