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

From: Atharva Raykar <raykar.ath@gmail.com>
Date: Aug 9, 2021, 08:47 UTC
Message-ID: <m2wnov9f8s.fsf@gmail.com>
In-Reply-To: <m27dgvaxfj.fsf@gmail.com>

Atharva Raykar <raykar.ath@gmail.com> writes:

Show 33 quoted lines

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

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