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

On 09/08/21 2:17 pm, Atharva Raykar wrote:

Atharva Raykar [off-list ref] writes:

quoted

Kaartic Sivaraam [off-list ref] writes:

quoted

On 08/08/21 11:11 pm, Kaartic Sivaraam wrote:

quoted

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