[GSoC,v5,1/9] submodule--helper: add options for compute_submodule_clone_url()

Commit Message

Atharva Raykar Aug. 10, 2021, 11:46 a.m. UTC

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, to suppress that warning when not needed, and then refactor
`resolve_relative_url()` by using this function, something we will do in
the next patch.

We also rename the local variable 'relurl' to avoid potential confusion
with the 'rel_url' parameter while we are at it.

Having this functionality factored out will be useful for converting the
rest of `submodule add` in subsequent patches.

Signed-off-by: Atharva Raykar <[e-mail]>
Mentored-by: Christian Couder <[e-mail]>
Mentored-by: Shourya Shukla <[e-mail]>
---
 builtin/submodule--helper.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

Comments

Bagas Sanjaya Aug. 11, 2021, 6:44 a.m. UTC | #1

On 10/08/21 18.46, Atharva Raykar wrote:
>   	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();
>   	}

Why did you split warning message? We could keep that in one line.

Atharva Raykar Aug. 11, 2021, 10:30 a.m. UTC | #2

Bagas Sanjaya <[e-mail]> writes:

> On 10/08/21 18.46, Atharva Raykar wrote:
>>   	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();
>>   	}
>
> Why did you split warning message? We could keep that in one line.

That line was too long, and given that I was moving the function and
changing it a little bit, I decided to make it adhere more closely to
the CodingGuidelines [1] and local convention.

[1] https://github.com/git/git/blob/6c85aac65fb455af85745130ce35ddae4678db84/Documentation/CodingGuidelines#L190

Patch

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 6891480013..d850b30fce 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -590,24 +590,28 @@  static int module_foreach(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
-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;
+	char *remoteurl, *resolved_url;
 	char *remote = get_default_remote();
 	struct strbuf remotesb = STRBUF_INIT;
 
 	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);
+	resolved_url = relative_url(remoteurl, rel_url, up_path);
 
 	free(remote);
 	free(remoteurl);
 	strbuf_release(&remotesb);
 
-	return relurl;
+	return resolved_url;
 }
 
 struct init_cb {
@@ -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);
 		}
 
@@ -2134,7 +2138,7 @@  static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
 	if (repo_config_get_string_tmp(the_repository, sb.buf, &url)) {
 		if (starts_with_dot_slash(sub->url) ||
 		    starts_with_dot_dot_slash(sub->url)) {
-			url = compute_submodule_clone_url(sub->url);
+			url = compute_submodule_clone_url(sub->url, NULL, 0);
 			need_free_url = 1;
 		} else
 			url = sub->url;