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

[Atharva Raykar <raykar.ath@gmail.com>]

Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes:

> On 08/08/21 11:11 pm, Kaartic Sivaraam wrote:
>> On 07/08/21 12:46 pm, Atharva Raykar wrote:
>>> [...]
>> 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 ...

I'll keep re-rolling till the code is good, it's never a problem ;-)

>> -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?

Okay.

>>  	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.
>

Right. I agree it should be corrected.

>>   	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);
>>  		}
>>