Re: [PATCH v3] 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'.

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.