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

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

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

> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 1a4b391c88..59334d4286 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2246,6 +2246,60 @@ static int module_config(int argc, const char **argv, const char *prefix)
>  	usage_with_options(git_submodule_helper_usage, module_config_options);
>  }
>  
> +struct set_url_cb {
> +	const char *prefix;
> +	unsigned int flags;
> +};
> +
> +#define SET_URL_CB_INIT { NULL, 0 }

Is any of the above even necessary?  I cannot see the place
info.prefix is used (I can see where it is set, but the only other
appearance of string 'prefix' is a call to sync_submodule() and
info.prefix is not used there at all), and info.flags is assigned
OPT_QUIET and passed to sync_submodule().  If you must have a
variable, a single "unsigned int" local variable in the function
would be sufficient, but it is unclear if even that is needed.

Worse yet, it claims to have something to do with a callback API by
having _cb suffix, but nobody calls anything with the data here.

> +static int module_set_url(int argc, const char **argv, const char *prefix)
> +{
> +	struct set_url_cb info = SET_URL_CB_INIT;

And this is the only place where SET_URL_CB_INIT is used.

I think all of the above should be discarded.

> +	int quiet = 0;

This is a good initialization, as parse_options() may not even see
any option related to this variable.

> +	const char *newurl = NULL;
> +	const char *path = NULL;

These are unnecessary, as we will _always_ assign to them before
doing anything else.  It is sufficient to say:

	const char *newurl, *path;

> +	struct strbuf entry = STRBUF_INIT;
> +

Why is this called entry?  All other occurences of string 'entry' in
thsi file always refer to a cache_entry.  Read on for a better name.

> +	struct option module_set_url_options[] = {
> +		OPT__QUIET(&quiet, N_("Suppress output for setting url of a submodule")),
> +		OPT_END()
> +	};

I think you are copying what other functions do, but for a local
variable to this function, I do not see the need to call this array
with such a long name.  Calling it "set_url_options[]" or even just
"options[]" is sufficient, because there is no other "struct option"
involved in this function.

> +	const char *const git_submodule_helper_usage[] = {
> +		N_("git submodule--helper set-url [--quiet] [<path>] [<newurl>]"),
> +		NULL
> +	};

Likewise.  "usage[]" should be plenty clear.

> +	argc = parse_options(argc, argv, NULL, module_set_url_options,
> +			     git_submodule_helper_usage, 0);

And this would become

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

without losing any clarity.

> +	info.prefix = prefix;
> +	if (quiet)
> +		info.flags |= OPT_QUIET;

Drop these three lines.

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

Wait.  If path is NULL, it is likely that argv[1] is an
out-of-bounds access.

> +	if(!path || !newurl){

Style.

	if (!path || !newurl) {

> +		usage_with_options(git_submodule_helper_usage,module_set_url_options);

This would become a short-and-sweet

		usage_with_options(usage, options);

Note that we require a SP after that comma.

> +		return 1;

OK.  This follows a (bit unnatural) convention used throughout this
file that module_<action> helpers signal a failure with positive 1,
and signal a success with 0.

> +	}
> +

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.

> +	strbuf_addstr(&entry, "submodule.");
> +	strbuf_addstr(&entry, path);
> +	strbuf_addstr(&entry, ".url");

The strbuf is to hold a configuration variable name and has nothing
to do with a cache entry.  Perhaps

	strbuf_addf(&config_name, "submodule.%s.url", path);

> +	/* Setting the new URL in .gitmodules */
> +	config_set_in_gitmodules_file_gently(entry.buf, newurl);  

It is dubious the comment above this call (and above the next call
to sync) adds any value.  The function name and its parameters tells
us enough, especially if you rename the strbuf variable and in a
single strbuf_addf(), which makes it pretty clear what the variable
contains (i.e. "submodule.<path>.url" variable in the .gitmodules
file).

> +	/* Syncing the updated URL */
> +	sync_submodule(path, prefix, info.flags);

This will become

	sync_submodule(path, prefix, quiet ? OPT_QUIET : 0);

So, there was no need for 'flags' variable, let alone set_url_cb
structure, at all.

> +	return 0;
> +}
> +
>  #define SUPPORT_SUPER_PREFIX (1<<0)
>  
>  struct cmd_struct {
> @@ -2276,6 +2330,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 89f915cae9..ea72d5a5f5 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -783,11 +783,13 @@ cmd_set_branch() {
>  # $@ = requested path, requested url
>  #
>  cmd_set_url() {
> +
>  	while test $# -ne 0
>  	do
>  		case "$1" in
>  		-q|--quiet)
>  			GIT_QUIET=1
> +			shift
>  			;;
>  		--)
>  			shift
> @@ -800,30 +802,9 @@ cmd_set_url() {
>  			break
>  			;;
>  		esac
> -		shift
>  	done

Why any of the above change?  It may not change the behaviour from
the current implementation, but why do we even want to touch it?

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

The loss of these lines is a good change ;-)  Very pleased to see
fewer lines of shell script.

Thanks.