[PATCH v3] submodule: port subcommand 'set-url' from shell to C

[PATCH v3] submodule: port subcommand 'set-url' from shell to C

[Shourya Shukla]

Convert submodule subcommand 'set-url' to a builtin. Port 'set-url'to
'submodule--helper.c' and call the latter via 'git-submodule.sh'.

Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>
---
Thank you Junio and Denton for the review :)
Eliminated the whitespace errors and removed the callback structures
as well. I have made the commit message more abstract for now as pointed
out by Denton that the message seems to go into too much detail.

Also, the variable name for `&entry` has been changed to `config_entry` so
as to avoid confusion with `cache_entry`. The shell file has also been
amended.

 builtin/submodule--helper.c | 46 +++++++++++++++++++++++++++++++++++++
 git-submodule.sh            | 22 +-----------------
 2 files changed, 47 insertions(+), 21 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 1a4b391c88..6fd459988e 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2246,6 +2246,51 @@ static int module_config(int argc, const char **argv, const char *prefix)
 	usage_with_options(git_submodule_helper_usage, module_config_options);
 }
 
+static int module_set_url(int argc, const char **argv, const char *prefix)
+{
+	int quiet = 0;
+
+	const char *newurl = NULL;
+	const char *path = NULL;
+
+	struct strbuf config_entry = STRBUF_INIT;
+
+	struct option set_url_options[] = {
+		OPT__QUIET(&quiet, N_("Suppress output for setting url of a submodule")),
+		OPT_END()
+	};
+
+	const char *const usage[] = {
+		N_("git submodule--helper set-url [--quiet] <path> <newurl>"),
+		NULL
+	};
+
+	argc = parse_options(argc, argv, prefix, set_url_options,
+			     usage, 0);
+
+	if (quiet)
+		quiet |= OPT_QUIET;
+
+	if (argc!=2){
+		usage_with_options(usage, set_url_options);
+		return 1;
+	}
+
+	path = argv[0];
+	newurl = argv[1];
+
+	strbuf_addstr(&config_entry, "submodule.");
+	strbuf_addstr(&config_entry, path);
+	strbuf_addstr(&config_entry, ".url");
+
+	config_set_in_gitmodules_file_gently(config_entry.buf, newurl);
+	sync_submodule(path, prefix, quiet);
+
+	strbuf_release(&config_entry);
+
+	return 0;
+}
+
 #define SUPPORT_SUPER_PREFIX (1<<0)
 
 struct cmd_struct {
@@ -2276,6 +2321,7 @@ static struct cmd_struct commands[] = {
 	{"is-active", is_active, 0},
 	{"check-name", check_name, 0},
 	{"config", module_config, 0},
+	{"set-url", module_set_url, 0},
 };
 
 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
diff --git a/git-submodule.sh b/git-submodule.sh
index 08e0439df0..39ebdf25b5 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -805,27 +805,7 @@ cmd_set_url() {
 		shift
 	done
 
-	if test $# -ne 2
-	then
-		usage
-	fi
-
-	# we can't use `git submodule--helper name` here because internally, it
-	# hashes the path so a trailing slash could lead to an unintentional no match
-	name="$(git submodule--helper list "$1" | cut -f2)"
-	if test -z "$name"
-	then
-		exit 1
-	fi
-
-	url="$2"
-	if test -z "$url"
-	then
-		exit 1
-	fi
-
-	git submodule--helper config submodule."$name".url "$url"
-	git submodule--helper sync ${GIT_QUIET:+--quiet} "$name"
+	git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper set-url ${GIT_QUIET:+--quiet} -- "$@"
 }
 
 #
-- 
2.26.2

Re: [PATCH v3] submodule: port subcommand 'set-url' from shell to C

[Junio C Hamano]

Shourya Shukla <shouryashukla.oo@gmail.com> writes:

> Convert submodule subcommand 'set-url' to a builtin. Port 'set-url'to
> 'submodule--helper.c' and call the latter via 'git-submodule.sh'.

OK.

> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 1a4b391c88..6fd459988e 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2246,6 +2246,51 @@ static int module_config(int argc, const char **argv, const char *prefix)
>  	usage_with_options(git_submodule_helper_usage, module_config_options);
>  }
>  
> +static int module_set_url(int argc, const char **argv, const char *prefix)
> +{
> +	int quiet = 0;
> +
> +	const char *newurl = NULL;
> +	const char *path = NULL;
> +
> +	struct strbuf config_entry = STRBUF_INIT;
> +
> +	struct option set_url_options[] = {
> +		OPT__QUIET(&quiet, N_("Suppress output for setting url of a submodule")),
> +		OPT_END()
> +	};
> +
> +	const char *const usage[] = {
> +		N_("git submodule--helper set-url [--quiet] <path> <newurl>"),
> +		NULL
> +	};

I do not see much point in leaving blank lines between the above
variable declarations. In fact, it is somewhat irritating to see.
Drop them.

Initializing "quiet" to 0 is good, because parse_options() may not
see any "-q" or "--quiet" on the command line, and you do not want
to leave the variable uninitialized.

Initializing "newurl" and "path" on the other hand are totally
unnecessary.  In fact, it will defeat the chance in the future to be
helped by compiler warning when somebody accidentally loses the
assignment to one of these variables.  Don't initialize them
unnecessarily.

> +	argc = parse_options(argc, argv, prefix, set_url_options,
> +			     usage, 0);
> +
> +	if (quiet)
> +		quiet |= OPT_QUIET;

This is bogus.  "command --quiet --quiet" would count-up quiet twice
and would make it 2, and you or OPT_QUIET==1 in to make it 3, but
your intention is quite clear that you want to pass 1 to
sync_submodule() in such a case.

Didn't I give you a review a few days ago and suggested a way to
make this whole thing unnecessary?

> +	if (argc!=2){
> +		usage_with_options(usage, set_url_options);
> +		return 1;
> +	}

Style.

> +	path = argv[0];
> +	newurl = argv[1];

These assign to path and newurl before they are used below, which
means that they can be left uninitialized above without risking use
of an uninitialized variable.

> +	strbuf_addstr(&config_entry, "submodule.");
> +	strbuf_addstr(&config_entry, path);
> +	strbuf_addstr(&config_entry, ".url");

strbuf_addf()?

Usually an "entry" is not just the name of it but also its contents,
so unless you are handing a struct that holds a <name, value> tuple
i.e. ("submodule.<path>.url", newurl), it is better not to call this
anything-entry.  It is a name of a variable, and on the next line
you'll give the variable a vlue.  Perhaps config_name or something?

> +	config_set_in_gitmodules_file_gently(config_entry.buf, newurl);
> +	sync_submodule(path, prefix, quiet);
> +
> +	strbuf_release(&config_entry);
> +
> +	return 0;
> +}
> +

I get a feeling that perhaps my review message on the previous round
did not reach you?  It's here:

  https://lore.kernel.org/git/xmqq5zdfmryd.fsf@gitster.c.googlers.com/

Thanks.

Re: [PATCH v3] submodule: port subcommand 'set-url' from shell to C

[Shourya Shukla]

On 04/05 08:55, Junio C Hamano wrote: 
> > +	argc = parse_options(argc, argv, prefix, set_url_options,
> > +			     usage, 0);
> > +
> > +	if (quiet)
> > +		quiet |= OPT_QUIET;
> 
> This is bogus.  "command --quiet --quiet" would count-up quiet twice
> and would make it 2, and you or OPT_QUIET==1 in to make it 3, but
> your intention is quite clear that you want to pass 1 to
> sync_submodule() in such a case.

This is a grave mistake from my side. Though I do not understand how
will `quiet` be counted twice. Let's say we pass `--quiet` option in the
command, then `quiet` is set to 1. And now if we do `quiet|= OPT_QUIET`,
this will make `quiet` 1 again right? Could you please tell me what am I
missing out here?

The fix you suggested (quiet ? OPT_QUIET : 0), we use this because we
want to ensure `quiet` goes into sync as either 1/0 right? Not any other
non-zero positive integer right?

Re: [PATCH v3] submodule: port subcommand 'set-url' from shell to C

[Junio C Hamano]

Shourya Shukla <shouryashukla.oo@gmail.com> writes:

> On 04/05 08:55, Junio C Hamano wrote: 
>> > +	argc = parse_options(argc, argv, prefix, set_url_options,
>> > +			     usage, 0);
>> > +
>> > +	if (quiet)
>> > +		quiet |= OPT_QUIET;
>> 
>> This is bogus.  "command --quiet --quiet" would count-up quiet twice
>> and would make it 2, and you or OPT_QUIET==1 in to make it 3, but
>> your intention is quite clear that you want to pass 1 to
>> sync_submodule() in such a case.
>
> This is a grave mistake from my side. Though I do not understand how
> will `quiet` be counted twice.

The way I read the definition of OPT__QUIET() 

    #define OPT__QUIET(var, h)    OPT_COUNTUP('q', "quiet",   (var), (h))

is that it is OPT_COUNTUP() in disguise, and that is designed to
yield increased quietness when "-q" is given more than once.

> The fix you suggested (quiet ? OPT_QUIET : 0), we use this because we
> want to ensure `quiet` goes into sync as either 1/0 right? Not any other
> non-zero positive integer right?

The "if (quiet) quiet |= OPT_QUIET" does not make *any* sense, if
you are expecting quiet to be set to 1 or left as 0 as initialized
by parse_options() API.  You are defeating the whole point of using
preprocessor macro OPT_QUIET, as the correctness of the construct
heavily rely on OPT_QUIET defined to be 1.  If for any reason the
preprocessor macro gets redefined to 8, writing

	quiet ? OPT_QUIET : 0

would need *no* adjustment, while "if (quiet) quiet |= OPT_QUIET"
would require fixing.