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

[PATCH v4] 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 for the review! :)
BTW, how detailed should the commit message be about the
patch?

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

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 1a4b391c88..f50745a03f 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2246,6 +2246,44 @@ 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;
+	const char *path;
+	struct strbuf config_name = 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 (argc!=2) {
+		usage_with_options(usage, set_url_options);
+		return 1;
+	}
+
+	path = argv[0];
+	newurl = argv[1];
+
+	strbuf_addf(&config_name, "submodule.%s.url", path);
+
+	config_set_in_gitmodules_file_gently(config_name.buf, newurl);
+	sync_submodule(path, prefix, quiet ? OPT_QUIET : 0);
+
+	strbuf_release(&config_name);
+
+	return 0;
+}
+
 #define SUPPORT_SUPER_PREFIX (1<<0)
 
 struct cmd_struct {
@@ -2276,6 +2314,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 v4] submodule: port subcommand 'set-url' from shell to C

[Christian Couder]

On Wed, May 6, 2020 at 9:37 AM Shourya Shukla
<shouryashukla.oo@gmail.com> wrote:
>
> Convert submodule subcommand 'set-url' to a builtin. Port 'set-url'to

There is a space missing between "'set-url'" and "to".

> 'submodule--helper.c' and call the latter via 'git-submodule.sh'.
>
> Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>
> ---
> Thank you Junio for the review! :)
> BTW, how detailed should the commit message be about the
> patch?

It looks good to me. Maybe it could more explicitely state that the
larger goal is to convert shell code in 'git-submodule.sh' to C code
in 'submodule--helper.c'. It can be guessed from the subject though.

>  builtin/submodule--helper.c | 39 +++++++++++++++++++++++++++++++++++++
>  git-submodule.sh            | 22 +--------------------
>  2 files changed, 40 insertions(+), 21 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 1a4b391c88..f50745a03f 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2246,6 +2246,44 @@ 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;
> +       const char *path;
> +       struct strbuf config_name = 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 (argc!=2) {

Please add space chars around "!=" like "argc != 2".

> +               usage_with_options(usage, set_url_options);
> +               return 1;
> +       }
> +
> +       path = argv[0];
> +       newurl = argv[1];
> +
> +       strbuf_addf(&config_name, "submodule.%s.url", path);
> +
> +       config_set_in_gitmodules_file_gently(config_name.buf, newurl);
> +       sync_submodule(path, prefix, quiet ? OPT_QUIET : 0);
> +
> +       strbuf_release(&config_name);

Nit: it might be a bit simpler to define config_name as a "char *",
and then use xstrfmt() and free() instead of strbuf_addf() and
strbuf_release().

> +       return 0;
> +}

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

[Shourya Shukla]

On 06/05 10:09, Christian Couder wrote:
> > +       strbuf_addf(&config_name, "submodule.%s.url", path);
> > +
> > +       config_set_in_gitmodules_file_gently(config_name.buf, newurl);
> > +       sync_submodule(path, prefix, quiet ? OPT_QUIET : 0);
> > +
> > +       strbuf_release(&config_name);
> 
> Nit: it might be a bit simpler to define config_name as a "char *",
> and then use xstrfmt() and free() instead of strbuf_addf() and
> strbuf_release().

Apart from the simplicity purposes, does doing this aid in performance
in any way?

> > +       return 0;
> > +}

Re: [PATCH v4] 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'.
>
> Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>
> ---
> Thank you Junio for the review! :)
> BTW, how detailed should the commit message be about the
> patch?
>
>  builtin/submodule--helper.c | 39 +++++++++++++++++++++++++++++++++++++
>  git-submodule.sh            | 22 +--------------------
>  2 files changed, 40 insertions(+), 21 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 1a4b391c88..f50745a03f 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2246,6 +2246,44 @@ 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;
> +	const char *path;
> +	struct strbuf config_name = 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
> +	};

Hmph, do we really want all the blank lines in the above?

There is only one "struct option" the code in this function needs to
be aware of and worried about.  Isn't naming it set_url_options[]
overly redundant?  Calling it just options[] would save lines here ;-)

> +	argc = parse_options(argc, argv, prefix, set_url_options,
> +			     usage, 0);

	argc = parse_options(argc, argv, prefix, options, usage, 0);

> +	if (argc!=2) {

Style.  SP around all binary operators like !=, i.e.

	if (argc != 2) {

By the way, looking at print_default_remote() that takes no
arguments wants argc to be 1, and resolve_relative_url() that takes
only one or two arguments checks for 2 or 3, shouldn't this be
checking if argc is 3, not 2?

I thought I pointed it out in my very first review of this series.

	... tries to go back and check, notices that this v4 is not
        ... a reply to v3 or earlier and feels somewhat irritated.
	... then finally finds the following in the v2 review.

> Taking all these together,
> 
>         if (argc != 3) {
>                 usage_with_options(usage, options);
>                 return 1;
>         }
>         path = argv[0];
>         newurl = argv[1];
> 
> If you feel paranoid, you can check these two are not NULL, too,
> i.e.
> 
>         if (argc != 3 || !(path = argv[0]) || !(newurl = argv[1])) {
>                 usage_with_options(usage, options);
>                 return 1;
>         }
> 
> I have no strong preference either way.  Perhaps the latter is more
> concise and more careful at the same time, so some people may prefer
> it.

Thanks.

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

[Junio C Hamano]

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

> On 06/05 10:09, Christian Couder wrote:
>> > +       strbuf_addf(&config_name, "submodule.%s.url", path);
>> > +
>> > +       config_set_in_gitmodules_file_gently(config_name.buf, newurl);
>> > +       sync_submodule(path, prefix, quiet ? OPT_QUIET : 0);
>> > +
>> > +       strbuf_release(&config_name);
>> 
>> Nit: it might be a bit simpler to define config_name as a "char *",
>> and then use xstrfmt() and free() instead of strbuf_addf() and
>> strbuf_release().
>
> Apart from the simplicity purposes, does doing this aid in performance
> in any way?

strbuf.c::xstrfmt() uses strbuf.c::xstrvfmt() that formats into a
temporary strbuf and returns the detached buffer as the result.

Compare it with what strbuf.c::strbuf_addf() and you can draw a
conclusion on your own ;-)

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

[Shourya Shukla]

On 06/05 10:12, Junio C Hamano wrote: 
> > +static int module_set_url(int argc, const char **argv, const char *prefix)
> > +{
> > +	int quiet = 0;
> > +	const char *newurl;
> > +	const char *path;
> > +	struct strbuf config_name = 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
> > +	};
> 
> Hmph, do we really want all the blank lines in the above?

Apologies,will amend.

> There is only one "struct option" the code in this function needs to
> be aware of and worried about.  Isn't naming it set_url_options[]
> overly redundant?  Calling it just options[] would save lines here ;-)

I was actually following the format of the other subcommands, will
surely change it.

> > +	argc = parse_options(argc, argv, prefix, set_url_options,
> > +			     usage, 0);
> 
> 	argc = parse_options(argc, argv, prefix, options, usage, 0);
> 
> > +	if (argc!=2) {
> 
> Style.  SP around all binary operators like !=, i.e.
> 
> 	if (argc != 2) {
> 
> By the way, looking at print_default_remote() that takes no
> arguments wants argc to be 1, and resolve_relative_url() that takes
> only one or two arguments checks for 2 or 3, shouldn't this be
> checking if argc is 3, not 2?

Aren't `path` and `newurl` the only arguments we should worry about
here as 'parse_options' will parse out the other arguments ('git
submodule--helper' and the 'quiet' option) leaving us with only the
aforementioned arguments. Am I missing something here?

To add on, checking for `argc!=3` results in a failure of t7420.
If we have anything but 2 arguments (either less or more) we should have
a failure.

I think that we will do a check for 3 if we pass the macro
`PARSE_OPT_KEEP_ARGV0` in `parse_options()`. So the final code segment
would look like:
	
	argc = parse_options(argc, argv, prefix, options,
			     usage, PARSE_OPT_KEEP_ARGV0);

	if (argc != 3) {
		usage_with_options(usage, options);
		return 1;
	}

	path = argv[1];
	newurl = argv[2];

which does pass t7420. Therefore a stricter check could be:
	
	argc = parse_options(argc, argv, prefix, options,
			     usage, 0);

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

	if (argc != 2 || path == NULL || newurl == NULL) {
		usage_with_options(usage, options);
		return 1;
	}
which passes t7420.

> I thought I pointed it out in my very first review of this series.
> 
> 	... tries to go back and check, notices that this v4 is not
>         ... a reply to v3 or earlier and feels somewhat irritated.
> 	... then finally finds the following in the v2 review.

I am very very sorry for this. I undestand how this must feel. Will
ensure this from the next version. :)

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

[Junio C Hamano]

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

>> By the way, looking at print_default_remote() that takes no
>> arguments wants argc to be 1, and resolve_relative_url() that takes
>> only one or two arguments checks for 2 or 3, shouldn't this be
>> checking if argc is 3, not 2?
>
> Aren't `path` and `newurl` the only arguments we should worry about
> here as 'parse_options' will parse out the other arguments ('git
> submodule--helper' and the 'quiet' option) leaving us with only the
> aforementioned arguments. Am I missing something here?

Ah, I misread those examples that suggested that you are supposed to
check for N+1 when you expect N arguments.   They are *not* using
parse_options() and that is where that funny numbering comes from.

This one uses "argc = parse_options(...)" so we should check for N
when we want N args.  Thanks.

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

[Shourya Shukla]

On 06/05 11:22, Junio C Hamano wrote: 
> Ah, I misread those examples that suggested that you are supposed to
> check for N+1 when you expect N arguments.   They are *not* using
> parse_options() and that is where that funny numbering comes from.
> 
> This one uses "argc = parse_options(...)" so we should check for N
> when we want N args.  Thanks.

No worries. BTW, should I include the `path == NULL` check in the
if-statement? I think the `argc` check would suffice but I would still
love to hear a final verdict from you and Christian :)

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

[Junio C Hamano]

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

> No worries. BTW, should I include the `path == NULL` check in the
> if-statement?

If I were writing this code, I would probably write it like so:

	if (!path || !newurl)
		oops;

Specifically, I would write "!path", not "path == NULL".  I thought
a rule for that is in the CodingGuidelines (I didn't double check,
though).

The comparison on argc is to see if we are even allowed to access
argv[0] and/or argv[1].  In practice, if what main() got from the
outside world in argv[] is passed directly to you, argv[n] would
never be NULL as long as n < argc, but there are a few levels of
callchain between main() and you (i.e. module_set_url()), so not
counting on that would be sensible.

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

[Shourya Shukla]

On 06/05 10:08, Junio C Hamano wrote:
> Shourya Shukla <shouryashukla.oo@gmail.com> writes:
> 
> > No worries. BTW, should I include the `path == NULL` check in the
> > if-statement?
> 
> If I were writing this code, I would probably write it like so:
> 
> 	if (!path || !newurl)
> 		oops;
> 
> Specifically, I would write "!path", not "path == NULL".  I thought
> a rule for that is in the CodingGuidelines (I didn't double check,
> though).

I could not find a rule like that in the CodingGuidelines.
Should I add it?
https://github.com/git/git/blob/master/Documentation/CodingGuidelines

> The comparison on argc is to see if we are even allowed to access
> argv[0] and/or argv[1].  In practice, if what main() got from the
> outside world in argv[] is passed directly to you, argv[n] would
> never be NULL as long as n < argc, but there are a few levels of
> callchain between main() and you (i.e. module_set_url()), so not
> counting on that would be sensible.

Understood. I will add the NULL check as well.

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

[Christian Couder]

On Fri, May 8, 2020 at 7:51 AM Shourya Shukla
<shouryashukla.oo@gmail.com> wrote:
>
> On 06/05 10:08, Junio C Hamano wrote:

> > Specifically, I would write "!path", not "path == NULL".  I thought
> > a rule for that is in the CodingGuidelines (I didn't double check,
> > though).
>
> I could not find a rule like that in the CodingGuidelines.
> Should I add it?
> https://github.com/git/git/blob/master/Documentation/CodingGuidelines

Sure.

> > The comparison on argc is to see if we are even allowed to access
> > argv[0] and/or argv[1].  In practice, if what main() got from the
> > outside world in argv[] is passed directly to you, argv[n] would
> > never be NULL as long as n < argc, but there are a few levels of
> > callchain between main() and you (i.e. module_set_url()), so not
> > counting on that would be sensible.
>
> Understood. I will add the NULL check as well.

Thanks,
Christian.

[PATCH v5] 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>
---
 builtin/submodule--helper.c | 37 +++++++++++++++++++++++++++++++++++++
 git-submodule.sh            | 22 +---------------------
 2 files changed, 38 insertions(+), 21 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 1a4b391c88..8bc7b4cfa6 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2246,6 +2246,42 @@ 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;
+	const char *path;
+	char* config_name;
+
+	struct option 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, options, usage, 0);
+
+	path = argv[0];
+	newurl = argv[1];
+
+	if (argc != 2 || !path || !newurl) {
+		usage_with_options(usage, options);
+		return 1;
+	}
+
+	config_name = xstrfmt("submodule.%s.url", path);
+
+	config_set_in_gitmodules_file_gently(config_name, newurl);
+	sync_submodule(path, prefix, quiet ? OPT_QUIET : 0);
+
+	free(config_name);
+
+	return 0;
+}
+
 #define SUPPORT_SUPER_PREFIX (1<<0)
 
 struct cmd_struct {
@@ -2276,6 +2312,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 v5] submodule: port subcommand 'set-url' from shell to C

[Denton Liu]

Hi Shourya,

It looks good to me except for one tiny nit:

On Fri, May 08, 2020 at 11:51:36AM +0530, Shourya Shukla wrote:
> 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>
> ---
>  builtin/submodule--helper.c | 37 +++++++++++++++++++++++++++++++++++++
>  git-submodule.sh            | 22 +---------------------
>  2 files changed, 38 insertions(+), 21 deletions(-)
> 
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 1a4b391c88..8bc7b4cfa6 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2246,6 +2246,42 @@ 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;
> +	const char *path;
> +	char* config_name;

The asterisk should be stuck with the name, not the type, similar to how
you wrote it above.

> +
> +	struct option 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, options, usage, 0);
> +
> +	path = argv[0];
> +	newurl = argv[1];
> +
> +	if (argc != 2 || !path || !newurl) {
> +		usage_with_options(usage, options);
> +		return 1;
> +	}
> +
> +	config_name = xstrfmt("submodule.%s.url", path);
> +
> +	config_set_in_gitmodules_file_gently(config_name, newurl);
> +	sync_submodule(path, prefix, quiet ? OPT_QUIET : 0);
> +
> +	free(config_name);
> +
> +	return 0;
> +}
> +
>  #define SUPPORT_SUPER_PREFIX (1<<0)
>  
>  struct cmd_struct {

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

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> On Fri, May 8, 2020 at 7:51 AM Shourya Shukla
> <shouryashukla.oo@gmail.com> wrote:
>>
>> On 06/05 10:08, Junio C Hamano wrote:
>
>> > Specifically, I would write "!path", not "path == NULL".  I thought
>> > a rule for that is in the CodingGuidelines (I didn't double check,
>> > though).
>>
>> I could not find a rule like that in the CodingGuidelines.
>> Should I add it?
>> https://github.com/git/git/blob/master/Documentation/CodingGuidelines
>
> Sure.

I'd rather not see too many unrelated things piled up on Shourya's
plate.  Without guidance, the new entry we'll see would be only
about comparing with NULL, and we'd need to spend review cycles
correcting that, too.

How about something like this, perhaps?

-- >8 --
CodingGuidelines: do not ==/!= compare with 0/NULL

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/CodingGuidelines | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
index 390ceece52..41a89dd845 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines
@@ -236,6 +236,19 @@ For C programs:
         while( condition )
 		func (bar+1);
 
+ - Do not explicitly compare an integral value with constant 0 or a
+   pointer value with constant NULL for equality; just say !value
+   instead.  To validate a counted array at ptr that has cnt elements
+   in it, write:
+
+	if (!ptr || !cnt)
+		BUG("array should not be empty at this point");
+
+   and not:
+
+	if (ptr == NULL || cnt == 0);
+		BUG("array should not be empty at this point");
+
  - We avoid using braces unnecessarily.  I.e.
 
 	if (bla) {

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

[Eric Sunshine]

On Fri, May 8, 2020 at 11:18 AM Junio C Hamano <gitster@pobox.com> wrote:
> + - Do not explicitly compare an integral value with constant 0 or a
> +   pointer value with constant NULL for equality; just say !value
> +   instead.  To validate a counted array at ptr that has cnt elements
> +   in it, write:
> +
> +       if (!ptr || !cnt)
> +               BUG("array should not be empty at this point");
> +
> +   and not:
> +
> +       if (ptr == NULL || cnt == 0);
> +               BUG("array should not be empty at this point");

This talks only about '=='. People might still use 0 or NULL with
'!='. I wonder if the example can include '!=', as well. Perhaps:

    if (!ptr)
        BUG("...");
    if (cnt)
        foo(ptr, cnt);

instead of:

    if (ptr == NULL)
        BUG("...");
    if (cnt != 0)
        foo(ptr, cnt);

or something.

Also, would you want to talk about not comparing against NUL character?

    if (*s)
        foo(s);

instead of:

    if (*s != '\0')
        foo(s);

Maybe that's overkill since NUL is an integral value which is already
covered by your earlier statement (but perhaps some people would
overlook that).

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

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Fri, May 8, 2020 at 11:18 AM Junio C Hamano <gitster@pobox.com> wrote:
>> + - Do not explicitly compare an integral value with constant 0 or a
>> +   pointer value with constant NULL for equality; just say !value
>> +   instead.  To validate a counted array at ptr that has cnt elements
>> +   in it, write:
>> +
>> +       if (!ptr || !cnt)
>> +               BUG("array should not be empty at this point");
>> +
>> +   and not:
>> +
>> +       if (ptr == NULL || cnt == 0);
>> +               BUG("array should not be empty at this point");
>
> This talks only about '=='.

Yup.  The text would need a matching change, though.

> People might still use 0 or NULL with
> '!='. I wonder if the example can include '!=', as well. Perhaps:
>
>     if (!ptr)
>         BUG("...");
>     if (cnt)
>         foo(ptr, cnt);
>
> instead of:
>
>     if (ptr == NULL)
>         BUG("...");
>     if (cnt != 0)
>         foo(ptr, cnt);
>
> or something.

Or more succinctly:

	if (!ptr || cnt)
		BUG("we must have an empty array at this point");

perhaps?

> Also, would you want to talk about not comparing against NUL character?
>
>     if (*s)
>         foo(s);
>
> instead of:
>
>     if (*s != '\0')
>         foo(s);
>
> Maybe that's overkill since NUL is an integral value which is already
> covered by your earlier statement (but perhaps some people would
> overlook that).

Yeah, it might be worth saying it explicitly.  I dunno.

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

[Eric Sunshine]

On Fri, May 08, 2020 at 08:57:09AM -0700, Junio C Hamano wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> > This talks only about '=='.
> 
> Yup.  The text would need a matching change, though.

Here's a re-roll with the necessary changes.

> >     if (!ptr)
> >         BUG("...");
> >     if (cnt)
> >         foo(ptr, cnt);
> 
> Or more succinctly:
> 
> 	if (!ptr || cnt)
> 		BUG("we must have an empty array at this point");

I considered that but thought it might be too "cute", however, seeing
it written out, it looks fine, so I used it in the re-roll.

> > Also, would you want to talk about not comparing against NUL character?
> 
> Yeah, it might be worth saying it explicitly.  I dunno.

Rather than giving this a separate example in the re-roll, I just
mentioned '\0' in the text.

--- >8 ---

From: Junio C Hamano <gitster@pobox.com>
Subject: [PATCH] CodingGuidelines: do not ==/!= compare with 0 or '\0' or NULL

Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 Documentation/CodingGuidelines | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
index 390ceece52..6dfc47ed7d 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines
@@ -236,6 +236,18 @@ For C programs:
         while( condition )
 		func (bar+1);
 
+ - Do not explicitly compare an integral value with constant 0 or '\0',
+   or a pointer value with constant NULL.  For instance, to validate a
+   counted array ptr that has cnt elements, write:
+
+	if (!ptr || cnt)
+		BUG("empty array expected");
+
+   and not:
+
+	if (ptr == NULL || cnt != 0);
+		BUG("empty array expected");
+
  - We avoid using braces unnecessarily.  I.e.
 
 	if (bla) {
-- 
2.26.2.717.g5cccb0e1a8

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

[Junio C Hamano]

Denton Liu <liu.denton@gmail.com> writes:

> Hi Shourya,
>
> It looks good to me except for one tiny nit:
>
> On Fri, May 08, 2020 at 11:51:36AM +0530, Shourya Shukla wrote:
>> 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>
>> ---
>>  builtin/submodule--helper.c | 37 +++++++++++++++++++++++++++++++++++++
>>  git-submodule.sh            | 22 +---------------------
>>  2 files changed, 38 insertions(+), 21 deletions(-)
>> 
>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
>> index 1a4b391c88..8bc7b4cfa6 100644
>> --- a/builtin/submodule--helper.c
>> +++ b/builtin/submodule--helper.c
>> @@ -2246,6 +2246,42 @@ 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;
>> +	const char *path;
>> +	char* config_name;
>
> The asterisk should be stuck with the name, not the type, similar to how
> you wrote it above.

Right.

>> +
>> +	struct option 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, options, usage, 0);
>> +
>> +	path = argv[0];
>> +	newurl = argv[1];
>> +
>> +	if (argc != 2 || !path || !newurl) {

Checking argc at this point is too late to protect against the
potential out-of-bounds access we have already made to argv[0]
and argv[1].

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

[Junio C Hamano]

Denton Liu <liu.denton@gmail.com> writes:

>> +	if (argc != 2 || !path || !newurl) {
>> +		usage_with_options(usage, options);
>> +		return 1;

It is embarrassing that nobody noticed that usage_with_options() is
NORETURN; return 1 has no effect here.

>> +	}
>> +
>> +	config_name = xstrfmt("submodule.%s.url", path);
>> +
>> +	config_set_in_gitmodules_file_gently(config_name, newurl);
>> +	sync_submodule(path, prefix, quiet ? OPT_QUIET : 0);
>> +
>> +	free(config_name);
>> +
>> +	return 0;
>> +}
>> +
>>  #define SUPPORT_SUPER_PREFIX (1<<0)
>>  
>>  struct cmd_struct {

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

[Junio C Hamano]

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

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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * Here is what I'll queue with fixups for now.

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

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 1a4b391c88..46c03d2a12 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2246,6 +2246,37 @@ 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;
+	const char *path;
+	char *config_name;
+
+	struct option 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, options, usage, 0);
+
+	if (argc != 2 || !(path = argv[0]) || !(newurl = argv[1]))
+		usage_with_options(usage, options);
+
+	config_name = xstrfmt("submodule.%s.url", path);
+
+	config_set_in_gitmodules_file_gently(config_name, newurl);
+	sync_submodule(path, prefix, quiet ? OPT_QUIET : 0);
+
+	free(config_name);
+
+	return 0;
+}
+
 #define SUPPORT_SUPER_PREFIX (1<<0)
 
 struct cmd_struct {
@@ -2276,6 +2307,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-561-g07d8ea56f2

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

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> From: Junio C Hamano <gitster@pobox.com>
> Subject: [PATCH] CodingGuidelines: do not ==/!= compare with 0 or '\0' or NULL
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> ---
>  Documentation/CodingGuidelines | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
> index 390ceece52..6dfc47ed7d 100644
> --- a/Documentation/CodingGuidelines
> +++ b/Documentation/CodingGuidelines
> @@ -236,6 +236,18 @@ For C programs:
>          while( condition )
>  		func (bar+1);
>  
> + - Do not explicitly compare an integral value with constant 0 or '\0',
> +   or a pointer value with constant NULL.  For instance, to validate a
> +   counted array ptr that has cnt elements, write:

I think this should be

      counted array <ptr, cnt> is initialized but has no elements, write:

> +
> +	if (!ptr || cnt)
> +		BUG("empty array expected");
> +
> +   and not:
> +
> +	if (ptr == NULL || cnt != 0);
> +		BUG("empty array expected");
> +
>   - We avoid using braces unnecessarily.  I.e.
>  
>  	if (bla) {

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

[Eric Sunshine]

On Fri, May 08, 2020 at 09:38:17AM -0700, Junio C Hamano wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> > + - Do not explicitly compare an integral value with constant 0 or '\0',
> > +   or a pointer value with constant NULL.  For instance, to validate a
> > +   counted array ptr that has cnt elements, write:
> 
> I think this should be
> 
>       counted array <ptr, cnt> is initialized but has no elements, write:

You're right. Here's a corrected version. I also applied s/a/that/ in
the second line to improve the grammar a bit.

--- >8 ---

From: Junio C Hamano <gitster@pobox.com>
Subject: [PATCH v3] CodingGuidelines: do not ==/!= compare with 0 or '\0' or
 NULL

Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 Documentation/CodingGuidelines | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
index 390ceece52..803a3b9bde 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines
@@ -236,6 +236,18 @@ For C programs:
         while( condition )
 		func (bar+1);
 
+ - Do not explicitly compare an integral value with constant 0 or '\0',
+   or a pointer value with constant NULL.  For instance, to validate that
+   counted array <ptr, cnt> is initialized but has no elements, write:
+
+	if (!ptr || cnt)
+		BUG("empty array expected");
+
+   and not:
+
+	if (ptr == NULL || cnt != 0);
+		BUG("empty array expected");
+
  - We avoid using braces unnecessarily.  I.e.
 
 	if (bla) {
-- 
2.26.2.737.gf3227dd3d3