v4submodule: port subcommand 'set-url' from shell to C

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

> > +       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().

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

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

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

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

> > +	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.

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

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

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

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

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

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

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

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

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

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

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

>> +	}
>> +
>> +	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 {