Re: [GSoC] [PATCH v4 1/8] submodule--helper: add options for compute_submodule_clone_url()
From: Kaartic Sivaraam <hidden>
Date: 2021-08-08 18:26:55
On 08/08/21 11:11 pm, Kaartic Sivaraam wrote:
On 07/08/21 12:46 pm, Atharva Raykar wrote:quoted
Let's modify the interface to `compute_submodule_clone_url()` function by adding two more arguments, so that we can reuse this in various parts of `submodule--helper.c` that follow a common pattern, which is--read the remote url configuration of the superproject and then call `relative_url()`. This function is nearly identical to `resolve_relative_url()`, the only difference being the extra warning message. We can add a quiet flag to it, ...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 ...
-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?
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
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); }
-- Sivaraam