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

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

[Shourya Shukla]

Convert submodule subcommand 'set-branch' to a builtin and call it via
'git-submodule.sh'.

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
Helped-by: Denton Liu <liu.denton@gmail.com>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>
---
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.

 builtin/submodule--helper.c | 45 +++++++++++++++++++++++++++++++++++++
 git-submodule.sh            | 32 +++-----------------------
 2 files changed, 48 insertions(+), 29 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index f50745a03f..d14b9856a3 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)
+{
+	/*
+	 * 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")),
+		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
+	};
+
+	argc = parse_options(argc, argv, prefix, options, usage, 0);
+
+	if (!opt_branch && !opt_default)
+		die(_("--branch or --default required"));
+
+	if (opt_branch && opt_default)
+		die(_("--branch and --default are mutually exclusive"));
+
+	if (argc != 1 || !(path = argv[0]))
+		usage_with_options(usage, options);
+
+	config_name = xstrfmt("submodule.%s.branch", path);
+	config_set_in_gitmodules_file_gently(config_name, opt_branch);
+
+	free(config_name);
+	return 0;
+}
+
 #define SUPPORT_SUPER_PREFIX (1<<0)
 
 struct cmd_struct {
@@ -2315,6 +2359,7 @@ static struct cmd_struct commands[] = {
 	{"check-name", check_name, 0},
 	{"config", module_config, 0},
 	{"set-url", module_set_url, 0},
+	{"set-branch", module_set_branch, 0},
 };
 
 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
diff --git a/git-submodule.sh b/git-submodule.sh
index 39ebdf25b5..8c56191f77 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -719,7 +719,7 @@ cmd_update()
 # $@ = requested path
 #
 cmd_set_branch() {
-	unset_branch=false
+	default=
 	branch=
 
 	while test $# -ne 0
@@ -729,7 +729,7 @@ cmd_set_branch() {
 			# we don't do anything with this but we need to accept it
 			;;
 		-d|--default)
-			unset_branch=true
+			default=1
 			;;
 		-b|--branch)
 			case "$2" in '') usage ;; esac
@@ -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} -- "$@"
 }
 
 #
-- 
2.26.2

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

[Junio C Hamano]

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

> Convert submodule subcommand 'set-branch' to a builtin and call it via
> 'git-submodule.sh'.
>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
> Helped-by: Denton Liu <liu.denton@gmail.com>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>
> ---
> 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.

Sorry, I may have missed the previous rounds of discussion, but the
comment adds more puzzles than it helps readers.  "is currently not
used" can be seen from the code, but it is totally unclear why it is
not used.  Is that a design decision to always keep quiet or always
talkative (if so, "suppress output..." is not a good description)?
Is that that this is a WIP patch that the behaviour the option aims
to achieve hasn't been implemented?  Is it that no existing callers
pass "-q" to the scripted version, so there is no need to support
it (if so, why do we even accept it in the first place)?  Is it that
all existing callers pass "-q" so we need to accept it, but there is
nothing we need to make verbose so the variable is not passed around
in the codepath?

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

[Denton Liu]

On Thu, May 21, 2020 at 11:44:22AM -0700, Junio C Hamano wrote:
> Shourya Shukla <shouryashukla.oo@gmail.com> writes:
> 
> > Convert submodule subcommand 'set-branch' to a builtin and call it via
> > 'git-submodule.sh'.
> >
> > Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> > Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
> > Helped-by: Denton Liu <liu.denton@gmail.com>
> > Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> > Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>
> > ---
> > 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.
> 
> Sorry, I may have missed the previous rounds of discussion, but the
> comment adds more puzzles than it helps readers.  "is currently not
> used" can be seen from the code, but it is totally unclear why it is
> not used.  Is that a design decision to always keep quiet or always
> talkative (if so, "suppress output..." is not a good description)?
> Is that that this is a WIP patch that the behaviour the option aims
> to achieve hasn't been implemented?  Is it that no existing callers
> pass "-q" to the scripted version, so there is no need to support
> it (if so, why do we even accept it in the first place)?  Is it that
> all existing callers pass "-q" so we need to accept it, but there is
> nothing we need to make verbose so the variable is not passed around
> in the codepath?

As the original author of the shell code, I had it accept -q because,
with the other subcommmands, you can pass -q either before or after the
subcommand such as

	$ git submodule -q sync

or
	$ git submodule sync -q

and I wanted set-branch to retain that behaviour even though -q
ultimately doesn't affect set-branch at all since it's already a quiet
command.

Perhaps as a follow-up to this patch, we could stop accepting -q in
set-branch. I highly doubt that anyone is using it anyway.

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

[Junio C Hamano]

Denton Liu <liu.denton@gmail.com> writes:

>> Sorry, I may have missed the previous rounds of discussion, but the
>> comment adds more puzzles than it helps readers.  "is currently not
>> used" can be seen from the code, but it is totally unclear why it is
>> not used.  Is that a design decision to always keep quiet or always
>> talkative (if so, "suppress output..." is not a good description)?
>> Is that that this is a WIP patch that the behaviour the option aims
>> to achieve hasn't been implemented?  Is it that no existing callers
>> pass "-q" to the scripted version, so there is no need to support
>> it (if so, why do we even accept it in the first place)?  Is it that
>> all existing callers pass "-q" so we need to accept it, but there is
>> nothing we need to make verbose so the variable is not passed around
>> in the codepath?
>
> As the original author of the shell code, I had it accept -q because,
> with the other subcommmands, you can pass -q either before or after the
> subcommand such as
>
> 	$ git submodule -q sync
>
> or
> 	$ git submodule sync -q
>
> and I wanted set-branch to retain that behaviour even though -q
> ultimately doesn't affect set-branch at all since it's already a quiet
> command.

OK, so "we accept -q for uniformity across subcommands, but there is
nothing to make less verbose in this subcommand" is the answer to my
question.

That cannot be read from "... is currently not used"; especially
with "currently", I expect that most readers would expect we would
start using it in the (near) future, and some other readers would
guess that something used to be talkative and we squelched it using
the option but there no longer is such need because that something
is now quiet by default and there is no option to make it talkative.

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

[Đoàn Trần Công Danh]

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

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

[Shourya Shukla]

On 21/05 12:50, Junio C Hamano wrote:
> OK, so "we accept -q for uniformity across subcommands, but there is
> nothing to make less verbose in this subcommand" is the answer to my
> question.
> 
> That cannot be read from "... is currently not used"; especially
> with "currently", I expect that most readers would expect we would
> start using it in the (near) future, and some other readers would
> guess that something used to be talkative and we squelched it using
> the option but there no longer is such need because that something
> is now quiet by default and there is no option to make it talkative.

What do you think should be the most apt comment here? Also, the rest of
the code is fine right?

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

[Johannes Schindelin]

Hi Shourya,

On Thu, 21 May 2020, Shourya Shukla wrote:

> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index f50745a03f..d14b9856a3 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)
> +{
> +	/*
> +	 * 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")),
> +		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
> +	};
> +
> +	argc = parse_options(argc, argv, prefix, options, usage, 0);
> +
> +	if (!opt_branch && !opt_default)
> +		die(_("--branch or --default required"));
> +
> +	if (opt_branch && opt_default)
> +		die(_("--branch and --default are mutually exclusive"));
> +
> +	if (argc != 1 || !(path = argv[0]))
> +		usage_with_options(usage, options);
> +
> +	config_name = xstrfmt("submodule.%s.branch", path);
> +	config_set_in_gitmodules_file_gently(config_name, opt_branch);

What happens if this fails? E.g. when the permission is denied or disk is
full? This C code would then still `return 0`, pretending that it
succeeded. But the original shell script calls `git submodule--helper
config [...]` which calls `module_config()`, which in turn passes through
the return value of the `config_set_in_gitmodules_file_gently()` call.

In other words, you need something like this:

	int ret;

	[...]

	ret = config_set_in_gitmodules_file_gently(config_name, opt_branch);

	free(config_name);
	return ret;

> +
> +	free(config_name);
> +	return 0;
> +}
> +
>  #define SUPPORT_SUPER_PREFIX (1<<0)
>
>  struct cmd_struct {
> @@ -2315,6 +2359,7 @@ static struct cmd_struct commands[] = {
>  	{"check-name", check_name, 0},
>  	{"config", module_config, 0},
>  	{"set-url", module_set_url, 0},
> +	{"set-branch", module_set_branch, 0},

BTW I just noticed that the return value of these helpers is returned by
the `cmd_submodule__helper()` function. That is not correct, as the
convention is for Git's functions to return negative values in case of
errors _except_ for `cmd_*()` functions, which need to return an exit code
(valid values are between 0 and 127).

So I think we'll also need this (it's unrelated to your patch, at least
unrelated enough that it merits its own, separate patch):

-                       return commands[i].fn(argc - 1, argv + 1, prefix);
+                       return !!commands[i].fn(argc - 1, argv + 1, prefix);

Ciao,
Dscho

>  };
>
>  int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 39ebdf25b5..8c56191f77 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -719,7 +719,7 @@ cmd_update()
>  # $@ = requested path
>  #
>  cmd_set_branch() {
> -	unset_branch=false
> +	default=
>  	branch=
>
>  	while test $# -ne 0
> @@ -729,7 +729,7 @@ cmd_set_branch() {
>  			# we don't do anything with this but we need to accept it
>  			;;
>  		-d|--default)
> -			unset_branch=true
> +			default=1
>  			;;
>  		-b|--branch)
>  			case "$2" in '') usage ;; esac
> @@ -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} -- "$@"
>  }
>
>  #
> --
> 2.26.2
>
>

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

[Shourya Shukla]

Convert submodule subcommand 'set-branch' to a builtin and call it via
'git-submodule.sh'.

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
Helped-by: Denton Liu <liu.denton@gmail.com>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>
---
Thank you for the review Junio and Johannes. I have made the requested
changes.

 builtin/submodule--helper.c | 45 +++++++++++++++++++++++++++++++++++++
 git-submodule.sh            | 32 +++-----------------------
 2 files changed, 48 insertions(+), 29 deletions(-)

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")),
+		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
+	};
+
+	argc = parse_options(argc, argv, prefix, options, usage, 0);
+
+	if (!opt_branch && !opt_default)
+		die(_("--branch or --default required"));
+
+	if (opt_branch && opt_default)
+		die(_("--branch and --default are mutually exclusive"));
+
+	if (argc != 1 || !(path = argv[0]))
+		usage_with_options(usage, options);
+
+	config_name = xstrfmt("submodule.%s.branch", path);
+	ret = config_set_in_gitmodules_file_gently(config_name, opt_branch);
+
+	free(config_name);
+	return ret;
+}
+
 #define SUPPORT_SUPER_PREFIX (1<<0)
 
 struct cmd_struct {
@@ -2315,6 +2359,7 @@ static struct cmd_struct commands[] = {
 	{"check-name", check_name, 0},
 	{"config", module_config, 0},
 	{"set-url", module_set_url, 0},
+	{"set-branch", module_set_branch, 0},
 };
 
 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
diff --git a/git-submodule.sh b/git-submodule.sh
index 39ebdf25b5..8c56191f77 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -719,7 +719,7 @@ cmd_update()
 # $@ = requested path
 #
 cmd_set_branch() {
-	unset_branch=false
+	default=
 	branch=
 
 	while test $# -ne 0
@@ -729,7 +729,7 @@ cmd_set_branch() {
 			# we don't do anything with this but we need to accept it
 			;;
 		-d|--default)
-			unset_branch=true
+			default=1
 			;;
 		-b|--branch)
 			case "$2" in '') usage ;; esac
@@ -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} -- "$@"
 }
 
 #
-- 
2.26.2

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

[Kaartic Sivaraam]

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@tvgsbejvaqbjf.bet/T/#maf26182b084087ed08a2a72d3da2ee2026b1618e

Thanks,
Sivaraam

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

[Đoàn Trần Công Danh]

Hi Kaartic,

On 2020-05-24 00:19:38+0530, Kaartic Sivaraam <kaartic.sivaraam@gmail.com> wrote:
> I believe you missed Danh's v3 comments[1]. I'm mentioning them inline with
> some additional comments.

Thanks for checking this.

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

Yay, I still think it's better to use OPT_NOOP_NOARG, (and with shortopt q,
which I forgot in previous reply.)

	OPT_NOOP_NOARG('q', "quiet")

> 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_NOOP_NOARG will take care of description itself. Even if we choose
to not use OPT_NOOP_NOARG, a better description should be provided.

> > +		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@tvgsbejvaqbjf.bet/T/#maf26182b084087ed08a2a72d3da2ee2026b1618e

For the better record, I think it's better to use a permenent link,
just in case lore.kernel.org go into the dust someday,
people can still have a reference if they have an archive.

https://lore.kernel.org/git/20200521230453.GB2042@danh.dev/

-- 
Danh

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

[Junio C Hamano]

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

> On 21/05 12:50, Junio C Hamano wrote:
>> OK, so "we accept -q for uniformity across subcommands, but there is
>> nothing to make less verbose in this subcommand" is the answer to my
>> question.
>> 
>> That cannot be read from "... is currently not used"; especially
>> with "currently", I expect that most readers would expect we would
>> start using it in the (near) future, and some other readers would
>> guess that something used to be talkative and we squelched it using
>> the option but there no longer is such need because that something
>> is now quiet by default and there is no option to make it talkative.
>
> What do you think should be the most apt comment here?

"we accept -q for uniformity across subcommands, but there is nothing
to make less verbose in this subcommand", perhaps?

> Also, the rest of the code is fine right?

I didn't spot anything bad worth pointing out when I sent the review
message, but that does not necessarily mean the code is "fine" ;-) 

I see you have v4 sent out already, which probably has more
improvements based on others' input.  Thanks for working on this
topic.

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

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> +	config_set_in_gitmodules_file_gently(config_name, opt_branch);
>
> What happens if this fails? E.g. when the permission is denied or disk is
> full? This C code would then still `return 0`, pretending that it
> succeeded. But the original shell script calls `git submodule--helper
> config [...]` which calls `module_config()`, which in turn passes through
> the return value of the `config_set_in_gitmodules_file_gently()` call.
>
> In other words, you need something like this:
>
> 	int ret;
>
> 	[...]
>
> 	ret = config_set_in_gitmodules_file_gently(config_name, opt_branch);
>
> 	free(config_name);
> 	return ret;

Making sure we check the return value of helper functions we call is
a good discipline, but this is not quite enough.

> So I think we'll also need this (it's unrelated to your patch, at least
> unrelated enough that it merits its own, separate patch):
>
> -                       return commands[i].fn(argc - 1, argv + 1, prefix);
> +                       return !!commands[i].fn(argc - 1, argv + 1, prefix);

I checked (not all but most of the) functions in that commands[]
table and they all seem to return 0 for success and positive
non-zero for failure.

config_set_in_gitmodules_file_gently() takes the return value of a
helper function in its 'ret', gives an warning if it is negative,
and returns that 'ret' literally to the caller.  You suggestion
allows module_set_branch() return a negative value as-is.  You'd
need to return !!ret from there.

The "unrelated" change becomes only necessary if you do not do the
!!ret in module_set_branch(); otherwise it is unneeded, I think.

Thanks.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>>> +	config_set_in_gitmodules_file_gently(config_name, opt_branch);
>>
>> What happens if this fails? E.g. when the permission is denied or disk is
>> full? This C code would then still `return 0`, pretending that it
>> succeeded. But the original shell script calls `git submodule--helper
>> config [...]` which calls `module_config()`, which in turn passes through
>> the return value of the `config_set_in_gitmodules_file_gently()` call.
>>
>> In other words, you need something like this:
>>
>> 	int ret;
>>
>> 	[...]
>>
>> 	ret = config_set_in_gitmodules_file_gently(config_name, opt_branch);
>>
>> 	free(config_name);
>> 	return ret;
>
> Making sure we check the return value of helper functions we call is
> a good discipline,...

By the way, another topic by you for set-url has exactly the same
issue.  Its call to config_set_in_gitmodules_file_gently() can fail.

So can the call to sync_submodule(), but when it fails it won't come
back, so we do not have to worry about not capturing its return
value ;-)

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

[Shourya Shukla]

On 24/05 12:19, Kaartic Sivaraam wrote:
> 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.

I think it will all boil down to the consistency of all the subcommands.
Changing this would require making changes in various places: the C code
(obviously), the shell script (not only the cmd_set_branch() function
but the part for accepting user input as well) and the Documentation (I
might have maybe missed a couple of other changes to list here too). Its
not that I don't want to do this, but it would add unnecessary changes
don't you think? I would love it if others could weigh in their opinions
too about this.

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

We want to do this because $branch is an argument right?

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

[Đoàn Trần Công Danh]

On 2020-05-27 22:43:58+0530, Shourya Shukla <shouryashukla.oo@gmail.com> wrote:
> On 24/05 12:19, Kaartic Sivaraam wrote:
> > 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.
> 
> I think it will all boil down to the consistency of all the subcommands.
> Changing this would require making changes in various places: the C code
> (obviously), the shell script (not only the cmd_set_branch() function
> but the part for accepting user input as well) and the Documentation (I
> might have maybe missed a couple of other changes to list here too). Its

I don't think this is a valid argument.

Using OPT_NOOP_NOARG doesn't require any change in shell script since
the binary still accepts -q|--quiet.

The documentation of --quiet is still valid (since it doesn't print
anything regardless)

The only necessary change in in that C code.

> not that I don't want to do this, but it would add unnecessary changes
> don't you think? I would love it if others could weigh in their opinions
> too about this.
> 
>  > +	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.
> 
> We want to do this because $branch is an argument right?

We want to do this because we don't want to whitespace-split "$branch"

Let's say, for some reason, this command was run:

	git submodule set-branch --branch "a-branch --branch another" a-submodule

This version will run:

	git submodule--helper --branch a-branch --branch another a-submodule

Which will success if there's a branch "another" in the "a-submodule".
While that command should fail because we don't accept refname with
space.

-- 
Danh

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

[Đoàn Trần Công Danh]

On 2020-05-28 19:21:47+0700, Đoàn Trần Công Danh <congdanhqx@gmail.com> wrote:
> On 2020-05-27 22:43:58+0530, Shourya Shukla <shouryashukla.oo@gmail.com> wrote:
> >  > +	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.
> > 
> > We want to do this because $branch is an argument right?
> 
> We want to do this because we don't want to whitespace-split "$branch"
> 
> Let's say, for some reason, this command was run:
> 
> 	git submodule set-branch --branch "a-branch --branch another" a-submodule

Anyway, after typing this.
I'm thinking a bit, then re-read gitcli(7),
I think git-submodule is quite broken regarding to Git's guidelines:

-----------8<----------

Here are the rules regarding the "flags" that you should follow when you are
scripting Git:

 * it's preferred to use the non-dashed form of Git commands, which means that
   you should prefer `git foo` to `git-foo`.

 * splitting short options to separate words (prefer `git foo -a -b`
   to `git foo -ab`, the latter may not even work).

 * when a command-line option takes an argument, use the 'stuck' form.  In
   other words, write `git foo -oArg` instead of `git foo -o Arg` for short
   options, and `git foo --long-opt=Arg` instead of `git foo --long-opt Arg`
   for long options.  An option that takes optional option-argument must be
   written in the 'stuck' form.
------------>8--------------

Current Git, with and without this change, this command will fail:

	git submodule set-branch --branch=a-branch a-submodule

Thus, a script conformed with gitcli(7) will fail.
(And our git-submodule(1) doesn't conform with gitcli(7), FWIW).

After this change, those commands will success:

	git submodule--helper set-branch --branch a-branch a-submodule
	git submodule set-branch --branch "a-branch --branch=another" a-submodule

(The second one was written for demonstration purpose only,
I don't expect it will success)

This isn't related to this change, and git-submodule(1) will be
rewritten in C in the very near future.
Just want to make sure it's awared.

> 
> This version will run:
> 
> 	git submodule--helper --branch a-branch --branch another a-submodule
> 
> Which will success if there's a branch "another" in the "a-submodule".
> While that command should fail because we don't accept refname with
> space.

-- 
Danh

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

[Đoàn Trần Công Danh]

On 2020-05-28 21:01:42+0700, Đoàn Trần Công Danh <congdanhqx@gmail.com> wrote:
> On 2020-05-28 19:21:47+0700, Đoàn Trần Công Danh <congdanhqx@gmail.com> wrote:
> > On 2020-05-27 22:43:58+0530, Shourya Shukla <shouryashukla.oo@gmail.com> wrote:
> > >  > +	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.
> > > 
> > > We want to do this because $branch is an argument right?
> > 
> > We want to do this because we don't want to whitespace-split "$branch"
> > 
> > Let's say, for some reason, this command was run:
> > 
> > 	git submodule set-branch --branch "a-branch --branch another" a-submodule
> 
> Anyway, after typing this.
> I'm thinking a bit, then re-read gitcli(7),
> I think git-submodule is quite broken regarding to Git's guidelines:
> 
> -----------8<----------
> 
> Here are the rules regarding the "flags" that you should follow when you are
> scripting Git:
> 
>  * it's preferred to use the non-dashed form of Git commands, which means that
>    you should prefer `git foo` to `git-foo`.
> 
>  * splitting short options to separate words (prefer `git foo -a -b`
>    to `git foo -ab`, the latter may not even work).
> 
>  * when a command-line option takes an argument, use the 'stuck' form.  In
>    other words, write `git foo -oArg` instead of `git foo -o Arg` for short
>    options, and `git foo --long-opt=Arg` instead of `git foo --long-opt Arg`
>    for long options.  An option that takes optional option-argument must be
>    written in the 'stuck' form.
> ------------>8--------------
> 
> Current Git, with and without this change, this command will fail:
> 
> 	git submodule set-branch --branch=a-branch a-submodule
> 
> Thus, a script conformed with gitcli(7) will fail.
> (And our git-submodule(1) doesn't conform with gitcli(7), FWIW).
> 
> After this change, those commands will success:
> 
> 	git submodule--helper set-branch --branch a-branch a-submodule

This should be read:

 	git submodule--helper set-branch --branch=a-branch a-submodule

> 	git submodule set-branch --branch "a-branch --branch=another" a-submodule
> 
> (The second one was written for demonstration purpose only,
> I don't expect it will success)
> 
> This isn't related to this change, and git-submodule(1) will be
> rewritten in C in the very near future.
> Just want to make sure it's awared.
> 
> > 
> > This version will run:
> > 
> > 	git submodule--helper --branch a-branch --branch another a-submodule
> > 
> > Which will success if there's a branch "another" in the "a-submodule".
> > While that command should fail because we don't accept refname with
> > space.

It's me being noisy again.
I'm still puzzled by this idea (and I drank too much coffee, today).

I think the day of conversion of submodule from shell to C finish,
we can use current git-submodule--helper as the new git-submodule.

With that idea, I think why don't we passed all arguments from

	git submodule set-branch

into git-submodule--helper.
(Yes, the idea is wrong because the usage output will have
git submodule--helper as $0)

I tried that idea and run the test.
To my surprise the test failed :(.

Turn out git-submodule--helper set-branch doesn't do its advertised job,
git-submodule--helper set-branch doesn't understand short options -d and -b

We'll need this fixup regardless of the agreement on my other concerns.
----------8<---------
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 305c9abb3b..64636161a7 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2291,9 +2291,9 @@ static int module_set_branch(int argc, const char **argv, const char *prefix)
 	struct option options[] = {
 		OPT__QUIET(&quiet,
 			N_("suppress output for setting default tracking branch")),
-		OPT_BOOL(0, "default", &opt_default,
+		OPT_BOOL('d', "default", &opt_default,
 			N_("set the default tracking branch to master")),
-		OPT_STRING(0, "branch", &opt_branch, N_("branch"),
+		OPT_STRING('b', "branch", &opt_branch, N_("branch"),
 			N_("set the default tracking branch")),
 		OPT_END()
 	};
------8<-----------



-- 
Danh

[GSoC][PATCH v5] submodule: port subcommand 'set-branch' from shell to C

[Shourya Shukla]

Convert submodule subcommand 'set-branch' to a builtin and call it via
'git-submodule.sh'.

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
Helped-by: Denton Liu <liu.denton@gmail.com>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Helped-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>
---
Here is the v5 of the subcommand. Thank you Danh for the feedback! I
apologise for not replying on time. I have taken into account Danh's
suggestions on the `quiet` option as well as done the fixup Dscho
suggested (fixed by Junio here:
https://github.com/gitster/git/commit/77ba62f66ff8e3de54d81c240542edb42a2711c7)

 builtin/submodule--helper.c | 44 +++++++++++++++++++++++++++++++++++++
 git-submodule.sh            | 32 +++------------------------
 2 files changed, 47 insertions(+), 29 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index f50745a03f..a974e17571 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2284,6 +2284,49 @@ 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)
+{
+	int opt_default = 0, ret;
+	const char *opt_branch = NULL;
+	const char *path;
+	char *config_name;
+
+	/*
+	 * We accept the `quiet` option for uniformity across subcommands,
+	 * though there is nothing to make less verbose in this subcommand.
+	 */
+	struct option options[] = {
+		OPT_NOOP_NOARG('q', "quiet"),
+		OPT_BOOL('d', "default", &opt_default,
+			N_("set the default tracking branch to master")),
+		OPT_STRING('b', "branch", &opt_branch, N_("branch"),
+			N_("set the default tracking branch")),
+		OPT_END()
+	};
+	const char *const usage[] = {
+		N_("git submodule--helper set-branch [-q|--quiet] (-d|--default) <path>"),
+		N_("git submodule--helper set-branch [-q|--quiet] (-b|--branch) <branch> <path>"),
+		NULL
+	};
+
+	argc = parse_options(argc, argv, prefix, options, usage, 0);
+
+	if (!opt_branch && !opt_default)
+		die(_("--branch or --default required"));
+
+	if (opt_branch && opt_default)
+		die(_("--branch and --default are mutually exclusive"));
+
+	if (argc != 1 || !(path = argv[0]))
+		usage_with_options(usage, options);
+
+	config_name = xstrfmt("submodule.%s.branch", path);
+	ret = config_set_in_gitmodules_file_gently(config_name, opt_branch);
+
+	free(config_name);
+	return !!ret;
+}
+
 #define SUPPORT_SUPER_PREFIX (1<<0)
 
 struct cmd_struct {
@@ -2315,6 +2358,7 @@ static struct cmd_struct commands[] = {
 	{"check-name", check_name, 0},
 	{"config", module_config, 0},
 	{"set-url", module_set_url, 0},
+	{"set-branch", module_set_branch, 0},
 };
 
 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
diff --git a/git-submodule.sh b/git-submodule.sh
index 39ebdf25b5..43eb6051d2 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -719,7 +719,7 @@ cmd_update()
 # $@ = requested path
 #
 cmd_set_branch() {
-	unset_branch=false
+	default=
 	branch=
 
 	while test $# -ne 0
@@ -729,7 +729,7 @@ cmd_set_branch() {
 			# we don't do anything with this but we need to accept it
 			;;
 		-d|--default)
-			unset_branch=true
+			default=1
 			;;
 		-b|--branch)
 			case "$2" in '') usage ;; esac
@@ -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} -- "$@"
 }
 
 #
-- 
2.26.2

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

[Junio C Hamano]

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

> Convert submodule subcommand 'set-branch' to a builtin and call it via
> 'git-submodule.sh'.
>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
> Helped-by: Denton Liu <liu.denton@gmail.com>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Helped-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
> Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>
> ---
> Here is the v5 of the subcommand. Thank you Danh for the feedback! I
> apologise for not replying on time. I have taken into account Danh's
> suggestions on the `quiet` option as well as done the fixup Dscho
> suggested (fixed by Junio here:
> https://github.com/gitster/git/commit/77ba62f66ff8e3de54d81c240542edb42a2711c7)
>
>  builtin/submodule--helper.c | 44 +++++++++++++++++++++++++++++++++++++
>  git-submodule.sh            | 32 +++------------------------
>  2 files changed, 47 insertions(+), 29 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index f50745a03f..a974e17571 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2284,6 +2284,49 @@ 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)
> +{
> +	int opt_default = 0, ret;
> +	const char *opt_branch = NULL;
> +	const char *path;
> +	char *config_name;
> +
> +	/*
> +	 * We accept the `quiet` option for uniformity across subcommands,
> +	 * though there is nothing to make less verbose in this subcommand.
> +	 */
> +	struct option options[] = {
> +		OPT_NOOP_NOARG('q', "quiet"),
> +		OPT_BOOL('d', "default", &opt_default,
> +			N_("set the default tracking branch to master")),
> +		OPT_STRING('b', "branch", &opt_branch, N_("branch"),
> +			N_("set the default tracking branch")),
> ...
> +		OPT_END()
> +	};
> +	const char *const usage[] = {
> +		N_("git submodule--helper set-branch [-q|--quiet] (-d|--default) <path>"),
> +		N_("git submodule--helper set-branch [-q|--quiet] (-b|--branch) <branch> <path>"),


I notice that we gained back -d and -b shorthands that was
advertised but not implemented the previous rounds.  It is a bit
curious that we are adding these short-hands that nobody uses,
though.  

Will queue.  Thanks.

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

[Kaartic Sivaraam]

On 02-06-2020 22:05, Shourya Shukla wrote:
> Convert submodule subcommand 'set-branch' to a builtin and call it via
> 'git-submodule.sh'.
> 
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
> Helped-by: Denton Liu <liu.denton@gmail.com>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Helped-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
> Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>
> ---
> Here is the v5 of the subcommand. Thank you Danh for the feedback! I
> apologise for not replying on time. I have taken into account Danh's
> suggestions on the `quiet` option as well as done the fixup Dscho
> suggested (fixed by Junio here:
> https://github.com/gitster/git/commit/77ba62f66ff8e3de54d81c240542edb42a2711c7)
> 
>   builtin/submodule--helper.c | 44 +++++++++++++++++++++++++++++++++++++
>   git-submodule.sh            | 32 +++------------------------
>   2 files changed, 47 insertions(+), 29 deletions(-)
> 
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index f50745a03f..a974e17571 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2284,6 +2284,49 @@ 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)
> +{
> +	int opt_default = 0, ret;
> +	const char *opt_branch = NULL;
> +	const char *path;
> +	char *config_name;
> +
> +	/*
> +	 * We accept the `quiet` option for uniformity across subcommands,
> +	 * though there is nothing to make less verbose in this subcommand.
> +	 */
> +	struct option options[] = {
> +		OPT_NOOP_NOARG('q', "quiet"),
> +		OPT_BOOL('d', "default", &opt_default,
> +			N_("set the default tracking branch to master")),
> +		OPT_STRING('b', "branch", &opt_branch, N_("branch"),
> +			N_("set the default tracking branch")),
> +		OPT_END()
> +	};
> +	const char *const usage[] = {
> +		N_("git submodule--helper set-branch [-q|--quiet] (-d|--default) <path>"),
> +		N_("git submodule--helper set-branch [-q|--quiet] (-b|--branch) <branch> <path>"),
> +		NULL
> +	};

I'm having second thoughts about my suggestion[1] to include
the short option for '--quiet' in the usage. This is the only
usage in submodule--helper that mentions that '-q' is a short
hand for '--quiet'. That seems inconsistent. I see two ways but
I'm not sure which one of these would be better:

A. Dropping the mention of '-q' in this usage thus making it consistent
    with the other usages printed by submodule--helper.

B. Fixing other usages of submodule--helper to mention that '-q' is
    shorthand for quiet. This has the benefit of properly advertising
    the shorthand.

C. Just ignore this?

I also noticed one other thing. A quote from
Documentation/CodingGuidelines regarding the usage for reference:

>  Optional parts are enclosed in square brackets:
>    [<extra>]
>    (Zero or one <extra>.)
> 
>    --exec-path[=<path>]
>    (Option with an optional argument.  Note that the "=" is inside the
>    brackets.)
> 
>    [<patch>...]
>    (Zero or more of <patch>.  Note that the dots are inside, not
>    outside the brackets.)
> 
>  Multiple alternatives are indicated with vertical bars:
>    [-q | --quiet]
>    [--utf8 | --no-utf8]
> 
>  Parentheses are used for grouping:
>    [(<rev> | <range>)...]
>    (Any number of either <rev> or <range>.  Parens are needed to make
>    it clear that "..." pertains to both <rev> and <range>.)
> 
>    [(-p <parent>)...]
>    (Any number of option -p, each with one <parent> argument.)
> 
>    git remote set-head <name> (-a | -d | <branch>)
>    (One and only one of "-a", "-d" or "<branch>" _must_ (no square
>    brackets) be provided.)

So, according to this, I think the usage should be ...

     git submodule--helper set-branch [-q | --quiet] [-d | --default] <path>

... and ...

     git submodule--helper set-branch [-q|--quiet] [-b | 
--branch]<branch> <path>

... respectively.

> +		NULL
> +	};

---
Footnotes:

[1]: 
https://github.com/periperidip/git/commit/9a8918bf0688c583740b3dddafdba82f47972442#r39606384

-- 
Sivaraam

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

[Kaartic Sivaraam]

On Wed, Jun 3, 2020 at 12:31 AM Kaartic Sivaraam
<kaartic.sivaraam@gmail.com> wrote:
>
> I also noticed one other thing. A quote from
> Documentation/CodingGuidelines regarding the usage for reference:
>
> >  Optional parts are enclosed in square brackets:
> >    [<extra>]
> >    (Zero or one <extra>.)
> >
> >    --exec-path[=<path>]
> >    (Option with an optional argument.  Note that the "=" is inside the
> >    brackets.)
> >
> >    [<patch>...]
> >    (Zero or more of <patch>.  Note that the dots are inside, not
> >    outside the brackets.)
> >
> >  Multiple alternatives are indicated with vertical bars:
> >    [-q | --quiet]
> >    [--utf8 | --no-utf8]
> >
> >  Parentheses are used for grouping:
> >    [(<rev> | <range>)...]
> >    (Any number of either <rev> or <range>.  Parens are needed to make
> >    it clear that "..." pertains to both <rev> and <range>.)
> >
> >    [(-p <parent>)...]
> >    (Any number of option -p, each with one <parent> argument.)
> >
> >    git remote set-head <name> (-a | -d | <branch>)
> >    (One and only one of "-a", "-d" or "<branch>" _must_ (no square
> >    brackets) be provided.)
>
> So, according to this, I think the usage should be ...
>
>      git submodule--helper set-branch [-q | --quiet] [-d | --default] <path>
>
> ... and ...
>
>      git submodule--helper set-branch [-q|--quiet] [-b |
> --branch]<branch> <path>
>

Apologies, my mail client messed a little with the formatting.
This should actually be:

    git submodule--helper set-branch [-q | --quiet] [-b | --branch]
<branch> <path>

> ... respectively.
>
> > +             NULL
> > +     };
>
> ---
> Footnotes:
>
> [1]:
> https://github.com/periperidip/git/commit/9a8918bf0688c583740b3dddafdba82f47972442#r39606384
>

-- 
Sivaraam

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

[Christian Couder]

On Tue, Jun 2, 2020 at 9:01 PM Kaartic Sivaraam
<kaartic.sivaraam@gmail.com> wrote:
>
> On 02-06-2020 22:05, Shourya Shukla wrote:
> > Convert submodule subcommand 'set-branch' to a builtin and call it via
> > 'git-submodule.sh'.
> >
> > Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> > Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
> > Helped-by: Denton Liu <liu.denton@gmail.com>
> > Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> > Helped-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
> > Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>
> > ---
> > Here is the v5 of the subcommand. Thank you Danh for the feedback! I
> > apologise for not replying on time. I have taken into account Danh's
> > suggestions on the `quiet` option as well as done the fixup Dscho
> > suggested (fixed by Junio here:
> > https://github.com/gitster/git/commit/77ba62f66ff8e3de54d81c240542edb42a2711c7)
> >
> >   builtin/submodule--helper.c | 44 +++++++++++++++++++++++++++++++++++++
> >   git-submodule.sh            | 32 +++------------------------
> >   2 files changed, 47 insertions(+), 29 deletions(-)
> >
> > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> > index f50745a03f..a974e17571 100644
> > --- a/builtin/submodule--helper.c
> > +++ b/builtin/submodule--helper.c
> > @@ -2284,6 +2284,49 @@ 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)
> > +{
> > +     int opt_default = 0, ret;
> > +     const char *opt_branch = NULL;
> > +     const char *path;
> > +     char *config_name;
> > +
> > +     /*
> > +      * We accept the `quiet` option for uniformity across subcommands,
> > +      * though there is nothing to make less verbose in this subcommand.
> > +      */
> > +     struct option options[] = {
> > +             OPT_NOOP_NOARG('q', "quiet"),
> > +             OPT_BOOL('d', "default", &opt_default,
> > +                     N_("set the default tracking branch to master")),
> > +             OPT_STRING('b', "branch", &opt_branch, N_("branch"),
> > +                     N_("set the default tracking branch")),
> > +             OPT_END()
> > +     };
> > +     const char *const usage[] = {
> > +             N_("git submodule--helper set-branch [-q|--quiet] (-d|--default) <path>"),
> > +             N_("git submodule--helper set-branch [-q|--quiet] (-b|--branch) <branch> <path>"),
> > +             NULL
> > +     };
>
> I'm having second thoughts about my suggestion[1] to include
> the short option for '--quiet' in the usage. This is the only
> usage in submodule--helper that mentions that '-q' is a short
> hand for '--quiet'. That seems inconsistent. I see two ways but
> I'm not sure which one of these would be better:
>
> A. Dropping the mention of '-q' in this usage thus making it consistent
>     with the other usages printed by submodule--helper.
>
> B. Fixing other usages of submodule--helper to mention that '-q' is
>     shorthand for quiet. This has the benefit of properly advertising
>     the shorthand.
>
> C. Just ignore this?

The `git submodule` documentation has:

-q::
--quiet::
        Only print error messages.

even though the Synopsis is:

'git submodule' [--quiet] [--cached]
'git submodule' [--quiet] add [<options>] [--] <repository> [<path>]
'git submodule' [--quiet] status [--cached] [--recursive] [--] [<path>...]
...

So I prefer B, and maybe updating the synopsis, as I think most Git
commands have '-q' meaning '--quiet'.

[...]

> So, according to this, I think the usage should be ...
>
>      git submodule--helper set-branch [-q | --quiet] [-d | --default] <path>
>
> ... and ...
>
>      git submodule--helper set-branch [-q|--quiet] [-b | --branch]<branch> <path>
>
> ... respectively.

I don't agree. I think `git submodule--helper set-branch ...` requires
either "-d | --default" or "-b | --branch", while for example:

git submodule--helper set-branch [-q | --quiet] [-d | --default] <path>

would mean that "git submodule--helper set-branch my/path" is valid.

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

[Đoàn Trần Công Danh]

On 2020-06-02 10:58:45-0700, Junio C Hamano <gitster@pobox.com> wrote:
> Shourya Shukla <shouryashukla.oo@gmail.com> writes:
> 
> > +	 * though there is nothing to make less verbose in this subcommand.
> > +	 */
> > +	struct option options[] = {
> > +		OPT_NOOP_NOARG('q', "quiet"),
> > +		OPT_BOOL('d', "default", &opt_default,
> > +			N_("set the default tracking branch to master")),
> > +		OPT_STRING('b', "branch", &opt_branch, N_("branch"),
> > +			N_("set the default tracking branch")),
> > ...
> > +		OPT_END()
> > +	};
> > +	const char *const usage[] = {
> > +		N_("git submodule--helper set-branch [-q|--quiet] (-d|--default) <path>"),
> > +		N_("git submodule--helper set-branch [-q|--quiet] (-b|--branch) <branch> <path>"),
> 
> 
> I notice that we gained back -d and -b shorthands that was
> advertised but not implemented the previous rounds.  It is a bit
> curious that we are adding these short-hands that nobody uses,
> though.  

I think a day will come, when all git-submodule functionalities will
run by calling git-submodule--helper.

In that day, we will use current git-submodule--helper as the new
git-submodule.

To me, it'll be less noise to just gs/--helper// from this file and use
it as the new git-submodule, instead of changing the OPT_* all over
places.

Or is that a complain for missing some tests?

-- 
Danh

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

[Junio C Hamano]

Đoàn Trần Công Danh  <congdanhqx@gmail.com> writes:

> On 2020-06-02 10:58:45-0700, Junio C Hamano <gitster@pobox.com> wrote:
>> Shourya Shukla <shouryashukla.oo@gmail.com> writes:
>> 
>> > +	 * though there is nothing to make less verbose in this subcommand.
>> > +	 */
>> > +	struct option options[] = {
>> > +		OPT_NOOP_NOARG('q', "quiet"),
>> > +		OPT_BOOL('d', "default", &opt_default,
>> > +			N_("set the default tracking branch to master")),
>> > +		OPT_STRING('b', "branch", &opt_branch, N_("branch"),
>> > +			N_("set the default tracking branch")),
>> > ...
>> > +		OPT_END()
>> > +	};
>> > +	const char *const usage[] = {
>> > +		N_("git submodule--helper set-branch [-q|--quiet] (-d|--default) <path>"),
>> > +		N_("git submodule--helper set-branch [-q|--quiet] (-b|--branch) <branch> <path>"),
>> 
>> 
>> I notice that we gained back -d and -b shorthands that was
>> advertised but not implemented the previous rounds.  It is a bit
>> curious that we are adding these short-hands that nobody uses,
>> though.  
>
> I think a day will come, when all git-submodule functionalities will
> run by calling git-submodule--helper.

I'd expect that when that day with no scripted parts of "git
submodule" remains comes, the main entry point functions in
builtin/submodule--helper.c (like module_list(), update_clone(),
module_set_branch(), etc.) will become helper functions that live in
submodule-lib.c and would be called from builtin/submodule.c.  And
the conversion would rip out calls to parse_options() in each of
these functions that would migrate to submodule-lib.c

    Side note: instead of adding submodule-lib.c, you could add them
    directly to submodule.c if they are small enough.  I am however
    modeling after how the "diff" family was converted to C; the
    diff-lib.c layer is "library-ish helpers that get pre-parsed
    command line arguments and performs a single unit of work" that
    utilizes service routines at the lower layer that are in diff.c
    and submodule-lib.c and submodule.c will be in a similar kind of
    relationship.

> In that day, we will use current git-submodule--helper as the new
> git-submodule.

No, I do not think so.  Most of the option parsers would be redone
in builtin/submodule.c; only some that can be used as-is may migrate
as a whole to builtin/submodule.c and its parse_options() stuff
reused, but most of what is in submodule--helper would have to lose
their parse_options() calls, as nobody would be using module_list()
when there is no scripted "git submodule" exists, for example.

> Or is that a complain for missing some tests?

No, it was "do the minimum necessary for an implementation detail,
as we'll discard that part later anyway".

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

[Shourya Shukla]

On 02/06 09:45, Christian Couder wrote:
> >
> > I'm having second thoughts about my suggestion[1] to include
> > the short option for '--quiet' in the usage. This is the only
> > usage in submodule--helper that mentions that '-q' is a short
> > hand for '--quiet'. That seems inconsistent. I see two ways but
> > I'm not sure which one of these would be better:
> >
> > A. Dropping the mention of '-q' in this usage thus making it consistent
> >     with the other usages printed by submodule--helper.
> >
> > B. Fixing other usages of submodule--helper to mention that '-q' is
> >     shorthand for quiet. This has the benefit of properly advertising
> >     the shorthand.
> >
> > C. Just ignore this?
> 
> The `git submodule` documentation has:
> 
> -q::
> --quiet::
>         Only print error messages.
> 
> even though the Synopsis is:
> 
> 'git submodule' [--quiet] [--cached]
> 'git submodule' [--quiet] add [<options>] [--] <repository> [<path>]
> 'git submodule' [--quiet] status [--cached] [--recursive] [--] [<path>...]
> ...
> 
> So I prefer B, and maybe updating the synopsis, as I think most Git
> commands have '-q' meaning '--quiet'.

Yep, (B) sounds good! Junio in one of the previous mails stated that
this thing will need to be done for all subcommands when the shell
script is to be demolished i.e., `git submodule` becomes a builtin
completely. Actually, vrious usages might need to be fixed, for many
subcommands because almost none of them have any info about the
shorthand usages of options in their `options` array.

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

[Shourya Shukla]

On 03/06 01:02, Junio C Hamano wrote:
> I'd expect that when that day with no scripted parts of "git
> submodule" remains comes, the main entry point functions in
> builtin/submodule--helper.c (like module_list(), update_clone(),
> module_set_branch(), etc.) will become helper functions that live in
> submodule-lib.c and would be called from builtin/submodule.c.  And
> the conversion would rip out calls to parse_options() in each of
> these functions that would migrate to submodule-lib.c
> 
>     Side note: instead of adding submodule-lib.c, you could add them
>     directly to submodule.c if they are small enough.  I am however
>     modeling after how the "diff" family was converted to C; the
>     diff-lib.c layer is "library-ish helpers that get pre-parsed
>     command line arguments and performs a single unit of work" that
>     utilizes service routines at the lower layer that are in diff.c
>     and submodule-lib.c and submodule.c will be in a similar kind of
>     relationship.

There does exist a `submodule.c` outside of `builtin/` which has various
helper functions. Will that require renaming to `submodule-lib.c`? BTW
`set-branch` is a subcommand of `git submodule` so do we have to put it
into `submodule-lib.c` if there were to be one?
What is the motivation behind modelling it on the diff-family?

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

[Christian Couder]

On Thu, Jun 4, 2020 at 9:17 AM Shourya Shukla
<shouryashukla.oo@gmail.com> wrote:
>
> On 03/06 01:02, Junio C Hamano wrote:
> > I'd expect that when that day with no scripted parts of "git
> > submodule" remains comes, the main entry point functions in
> > builtin/submodule--helper.c (like module_list(), update_clone(),
> > module_set_branch(), etc.) will become helper functions that live in
> > submodule-lib.c and would be called from builtin/submodule.c.  And
> > the conversion would rip out calls to parse_options() in each of
> > these functions that would migrate to submodule-lib.c
> >
> >     Side note: instead of adding submodule-lib.c, you could add them
> >     directly to submodule.c if they are small enough.  I am however
> >     modeling after how the "diff" family was converted to C; the
> >     diff-lib.c layer is "library-ish helpers that get pre-parsed
> >     command line arguments and performs a single unit of work" that
> >     utilizes service routines at the lower layer that are in diff.c
> >     and submodule-lib.c and submodule.c will be in a similar kind of
> >     relationship.
>
> There does exist a `submodule.c` outside of `builtin/` which has various
> helper functions. Will that require renaming to `submodule-lib.c`?

No, as Junio says that "submodule-lib.c and submodule.c will be in a
similar kind of relationship" as diff.c and diff-lib.c.

> BTW
> `set-branch` is a subcommand of `git submodule` so do we have to put it
> into `submodule-lib.c` if there were to be one?
> What is the motivation behind modelling it on the diff-family?

Maybe to separate helper functions for submodules from other submodule
functions.

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

[Junio C Hamano]

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

> On 03/06 01:02, Junio C Hamano wrote:
>> I'd expect that when that day with no scripted parts of "git
>> submodule" remains comes, the main entry point functions in
>> builtin/submodule--helper.c (like module_list(), update_clone(),
>> module_set_branch(), etc.) will become helper functions that live in
>> submodule-lib.c and would be called from builtin/submodule.c.  And
>> the conversion would rip out calls to parse_options() in each of
>> these functions that would migrate to submodule-lib.c
>> 
>>     Side note: instead of adding submodule-lib.c, you could add them
>>     directly to submodule.c if they are small enough.  I am however
>>     modeling after how the "diff" family was converted to C; the
>>     diff-lib.c layer is "library-ish helpers that get pre-parsed
>>     command line arguments and performs a single unit of work" that
>>     utilizes service routines at the lower layer that are in diff.c
>>     and submodule-lib.c and submodule.c will be in a similar kind of
>>     relationship.
>
> There does exist a `submodule.c` outside of `builtin/` which has various
> helper functions. Will that require renaming to `submodule-lib.c`?

No, that is different from what I wrote above.  Just like there is
the middle-layer diff-lib.c between the top-layer builtin/diff.c and
the low-level helper sets in diff.c, I envision that between the
top-layer builtin/submodule.c and the low-level helper sets in
submodule.c, there would be the middle layer submodule-lib.c.

If a single cmd_submodule_set_url() function implements the whole of
"git submoduel set-url" (by calling helper routines in submodule.c
and those currently in builtin/submodule--helper.c), I would expect
it to reside in builtin/submodule.c.

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

[Kaartic Sivaraam]

On 03-06-2020 01:15, Christian Couder wrote:
> On Tue, Jun 2, 2020 at 9:01 PM Kaartic Sivaraam
> <kaartic.sivaraam@gmail.com> wrote:
>>
>> I'm having second thoughts about my suggestion[1] to include
>> the short option for '--quiet' in the usage. This is the only
>> usage in submodule--helper that mentions that '-q' is a short
>> hand for '--quiet'. That seems inconsistent. I see two ways but
>> I'm not sure which one of these would be better:
>>
>> A. Dropping the mention of '-q' in this usage thus making it consistent
>>     with the other usages printed by submodule--helper.
>>
>> B. Fixing other usages of submodule--helper to mention that '-q' is
>>     shorthand for quiet. This has the benefit of properly advertising
>>     the shorthand.
>>
>> C. Just ignore this?
> 
> The `git submodule` documentation has:
> 
> -q::
> --quiet::
>         Only print error messages.
> 
> even though the Synopsis is:
> 
> 'git submodule' [--quiet] [--cached]
> 'git submodule' [--quiet] add [<options>] [--] <repository> [<path>]
> 'git submodule' [--quiet] status [--cached] [--recursive] [--] [<path>...]
> ...
> 
> So I prefer B, and maybe updating the synopsis, as I think most Git
> commands have '-q' meaning '--quiet'.
> 

Makes sense.

> [...]
> 
>> So, according to this, I think the usage should be ...
>>
>>      git submodule--helper set-branch [-q | --quiet] [-d | --default] <path>
>>
>> ... and ...
>>
>>      git submodule--helper set-branch [-q|--quiet] [-b | --branch]<branch> <path>
>>
>> ... respectively.
> 
> I don't agree. I think `git submodule--helper set-branch ...` requires
> either "-d | --default" or "-b | --branch", while for example:
> 
> git submodule--helper set-branch [-q | --quiet] [-d | --default] <path>
> 
> would mean that "git submodule--helper set-branch my/path" is valid.
> 

You're right. Even I thought about the same thing when I came up with
that suggestion after quoting that portion of the CodingGuidelines. But
it was also curious for me to observe that the original used parenthesis
to mention the short and long options of an argument:

> +	const char *const usage[] = {
> +		N_("git submodule--helper set-branch [-q|--quiet] (-d|--default) <path>"),
> +		N_("git submodule--helper set-branch [-q|--quiet] (-b|--branch) <branch> <path>"),
> +		NULL
> +	};

I've not seen such a usage before. That's what actually made me take a
look at the CodingGuidelines for this. As the CodingGuidelined doesn't
seem to be mentioning anything about this explicitly, let's see if I
could find something in the usage printed by other commands.

---
> git am -h
usage: git am [<options>] [(<mbox> | <Maildir>)...]
   or: git am [<options>] (--continue | --skip | --abort)

   <options snipped>

> git branch -h
usage: git branch [<options>] [-r | -a] [--merged | --no-merged]
   or: git branch [<options>] [-l] [-f] <branch-name> [<start-point>]
   or: git branch [<options>] [-r] (-d | -D) <branch-name>...
   or: git branch [<options>] (-m | -M) [<old-branch>] <new-branch>
   or: git branch [<options>] (-c | -C) [<old-branch>] <new-branch>
   or: git branch [<options>] [-r | -a] [--points-at]
   or: git branch [<options>] [-r | -a] [--format]

   <options snipped>

> git checkout -h
usage: git checkout [<options>] <branch>
   or: git checkout [<options>] [<branch>] -- <file>...

   <options snipped>

> git switch -h
usage: git switch [<options>] [<branch>]

   <options snipped>

---
Hmm. Looks like it's not common for us to mention both the short
and long options in the usage itself. This might be to avoid the
redundancy as the usage is usually followed by the list of options.

With this info, I think we could've just gone with the following as the
usage strings for the `set-branch` subcommand:

    git submodule--helper set-branch [<options>] -d <path>
    git submodule--helper set-branch [<options>] -b <branch> <path>

This also solves the problem with `--quiet` I mentioned earlier while
making it concise and inline with the usages printed by other commands.

All this said, I don't think it's worth a re-roll now for several
reasons.

-- 
Sivaraam