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

From: Shourya Shukla <shouryashukla.oo@gmail.com>
Date: May 6, 2020, 18:12 UTC
Message-ID: <20200506181239.GA5683@konoha>
In-Reply-To: <xmqqtv0t6l84.fsf@gitster.c.googlers.com>

On 06/05 10:12, Junio C Hamano wrote:

Show 18 quoted lines

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

Show 15 quoted lines

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

Show 5 quoted lines

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