Re: [GSoC] [PATCH v4 1/8] submodule--helper: add options for compute_submodule_clone_url()
Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes:
Show 12 quoted lines
> 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 ;-)
Show 9 quoted lines
>> -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?Show 26 quoted lines
>> 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.
Show 11 quoted lines
>> 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);
>> }
>>Atharva Raykar <raykar.ath@gmail.com>