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

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

On 09/08/21 2:17 pm, Atharva Raykar wrote:
> 
> Atharva Raykar <raykar.ath@gmail.com> writes:
> 
>> 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:
>>>> [...]
>>>>   	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.
> 
> Actually on having another look, I'm not sure if we need to assign NULL
> to 'remoteurl' at all.
> 
> The 'if (git_config_get_string(...))' on success will allocate
> 'remoteurl'. If it fails, it will be given the return value of
> 'xgetcwd()'. There is nothing in the config API docs that suggest a
> success mode for the git_config_get_*() functions that will assign
> nothing to the buffer we give it. Therefore, by the time we get to the
> variable's first use in the 'relative_url()' function, we are guaranteed
> to have a well-defined value.
> 

Ah ha! That explains why we haven't got any reports about weird behaviours
when using the likes of `git submodule init` so far ;-)

Thanks for digging this and sorry about the false flag!

> It seems to me that the original 'resolve_relative_url()' had an
> unnecessary NULL initialization.
> 

Makes sense. I guess I feel into the trap of blindly trusting that the original
code was written correctly x-<

-- 
Sivaraam