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

[Kaartic Sivaraam <kaartic.sivaraam@xxxxxxxxx>]

Hi Shourya,

I believe you missed Danh's v3 comments[1]. I'm mentioning them inline 
with some additional comments.

On 23-05-2020 22:09, Shourya Shukla wrote:
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index f50745a03f..7e844e8971 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2284,6 +2284,50 @@ static int module_set_url(int argc, const char **argv, const char *prefix)
>   	return 0;
>   }
>   
> +static int module_set_branch(int argc, const char **argv, const char *prefix)
> +{
> +	/*
> +	 * We accept the `quiet` option for uniformity across subcommands,
> +	 * though there is nothing to make less verbose in this subcommand.
> +	 */
> +	int quiet = 0, opt_default = 0, ret;
> +	const char *opt_branch = NULL;
> +	const char *path;
> +	char *config_name;
> +
> +	struct option options[] = {
> +		OPT__QUIET(&quiet,
> +			N_("suppress output for setting default tracking branch")),

As '--quiet' in 'set-branch' is a no-op and is being accepted only for 
uniformity, I think it makes sense to use OPT_NOOP_NOARG instead of 
OPT__QUIET for specifying it, as suggested by Danh.

Also, the description "suppress output for setting default tracking 
branch" doesn't seem to be valid anymore as we don't print anything when 
set-branch succeeds.

> +		OPT_BOOL(0, "default", &opt_default,
> +			N_("set the default tracking branch to master")),
> +		OPT_STRING(0, "branch", &opt_branch, N_("branch"),
> +			N_("set the default tracking branch")),
> +		OPT_END()
> +	};
> +	const char *const usage[] = {
> +		N_("git submodule--helper set-branch [--quiet] (-d|--default) <path>"),
> +		N_("git submodule--helper set-branch [--quiet] (-b|--branch) <branch> <path>"),
> +		NULL
> +	};
> +

I also agree with the Danh here that '--quiet' could be removed from 
usage. There's no point in mentioning '--quiet' in the usage when it has 
no effect.

> diff --git a/git-submodule.sh b/git-submodule.sh
> index 39ebdf25b5..8c56191f77 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -750,33 +750,7 @@ cmd_set_branch() {
>   		shift
>   	done
>   
> -	if test $# -ne 1
> -	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
> -
> -	test -n "$branch"; has_branch=$?
> -	test "$unset_branch" = true; has_unset_branch=$?
> -
> -	if test $((!$has_branch != !$has_unset_branch)) -eq 0
> -	then
> -		usage
> -	fi
> -
> -	if test $has_branch -eq 0
> -	then
> -		git submodule--helper config submodule."$name".branch "$branch"
> -	else
> -		git submodule--helper config --unset submodule."$name".branch
> -	fi
> +	git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper set-branch ${GIT_QUIET:+--quiet} ${branch:+--branch $branch} ${default:+--default} -- "$@"
>   }

Danh questioned whether '$branch' needs to be quoted here. I too think 
it needs to be quoted unless I'm missing something.


---
Footnotes:
[1]: 
https://lore.kernel.org/git/nycvar.QRO.7.76.6.2005230012090.56@xxxxxxxxxxxxxxxxx/T/#maf26182b084087ed08a2a72d3da2ee2026b1618e

Thanks,
Sivaraam