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

["Đoàn Trần Công Danh" <congdanhqx@gmail.com>]

Hi Shourya,

On 2020-05-21 22:08:19+0530, Shourya Shukla <shouryashukla.oo@gmail.com> wrote:
> Thank you for the review Eric. I have changed the commit message,
> and the error prompts. Also, I have added a brief comment about
> the `quiet` option.
> 
> +	/*
> +	 * The `quiet` option is present for backward compatibility
> +	 * but is currently not used.
> +	 */
> +	int quiet = 0, opt_default = 0;
> +	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")),

IIUC, this option is provided to be backward compatible with old shell
version, and this option doesn't affect anything.

Would it make sense to hide quiet from default usage, via:

	OPT_NOOP_NOARG(0, "quiet")

I may missed some discussion related to the decision to keep it
OPT__QUIET.

> +		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>"),

And if above comment is applicable, remove `--quiet` from here.

> +	git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper set-branch ${GIT_QUIET:+--quiet} ${branch:+--branch $branch} ${default:+--default} -- "$@"

I think we need to quote `$branch`, no?

	${branch:+--branch "$branch"}

-- 
Danh