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

Kaartic Sivaraam [off-list ref] writes:

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

quoted

On 07/08/21 12:46 pm, Atharva Raykar wrote:

quoted

[...]

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 ;-)

quoted

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

Okay.

quoted

 	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.

quoted

  	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);
 		}