Atharva Raykar <raykar.ath@xxxxxxxxx> writes:
> Kaartic Sivaraam <kaartic.sivaraam@xxxxxxxxx> 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.
