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

[Junio C Hamano <gitster@pobox.com>]

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.