threads / patch / 53523

v3submodule: port subcommand 'set-branch' from shell to C

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

## tl;dr

29 messages between May 21, 2020 and Jun 4, 2020. Diffs are folded; open one to read it.

replies: 28people: 7as markdown or json

Shourya Shukla· May 21, 2020, 16:38 UTC · lore

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(-)
Show changes to 2 files +48 −29

builtin/submodule--helper.c, git-submodule.sh

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
Junio C Hamano· May 21, 2020, 18:44 UTC · re: Shourya Shukla · lore

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

Shourya Shukla <shouryashukla.oo@gmail.com> writes:
Show 12 quoted lines
> 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?

Denton Liu· May 21, 2020, 19:03 UTC · re: Junio C Hamano · lore

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

On Thu, May 21, 2020 at 11:44:22AM -0700, Junio C Hamano wrote:
Show 27 quoted lines
> 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.

Junio C Hamano· May 21, 2020, 19:50 UTC · re: Denton Liu · lore

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

Denton Liu <liu.denton@gmail.com> writes:
Show 25 quoted lines
>> 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.

Shourya Shukla· May 22, 2020, 19:39 UTC · re: Junio C Hamano · lore

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

On 21/05 12:50, Junio C Hamano wrote:
Show 10 quoted lines
> 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?

Junio C Hamano· May 24, 2020, 16:07 UTC · re: Shourya Shukla · lore

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

Shourya Shukla <shouryashukla.oo@gmail.com> writes:
Show 13 quoted lines
> 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.

Đoàn Trần Công Danh· May 21, 2020, 23:04 UTC · re: Shourya Shukla · lore

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

Hi Shourya,
On 2020-05-21 22:08:19+0530, Shourya Shukla <shouryashukla.oo@gmail.com> wrote:
Show 16 quoted lines
> 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.

Show 9 quoted lines
> +		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
Johannes Schindelin· May 22, 2020, 22:21 UTC · re: Shourya Shukla · lore

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

Hi Shourya,
On Thu, 21 May 2020, Shourya Shukla wrote:
Show 47 quoted lines
> 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;
Show 13 quoted lines
> +
> +	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

Show 64 quoted lines
>  };
>
>  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
>
>
Junio C Hamano· May 24, 2020, 23:15 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 18 quoted lines
>> +	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.

Show 5 quoted lines
> 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.
Junio C Hamano· May 24, 2020, 23:18 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 23 quoted lines
> 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 ;-)

Shourya Shukla· May 23, 2020, 16:39 UTC · re: Shourya Shukla · lore

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

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(-)
Show changes to 2 files +48 −29

builtin/submodule--helper.c, git-submodule.sh

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
Kaartic Sivaraam· May 23, 2020, 18:49 UTC · re: Shourya Shukla · lore

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

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:
Show 23 quoted lines
> 
> 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.

Show 12 quoted lines
> +		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.

Show 38 quoted lines
> 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

Đoàn Trần Công Danh· May 23, 2020, 23:18 UTC · re: Kaartic Sivaraam · lore

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

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.
Show 27 quoted lines
> 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.

Show 61 quoted lines
> > +		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
Shourya Shukla· May 27, 2020, 17:13 UTC · re: Kaartic Sivaraam · lore

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

On 24/05 12:19, Kaartic Sivaraam wrote:
Show 7 quoted lines
> 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?
Đoàn Trần Công Danh· May 28, 2020, 12:21 UTC · re: Shourya Shukla · lore

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

On 2020-05-27 22:43:58+0530, Shourya Shukla <shouryashukla.oo@gmail.com> wrote:
Show 14 quoted lines
> 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.
Show 10 quoted lines
> 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
Đoàn Trần Công Danh· May 28, 2020, 14:01 UTC · re: Đoàn Trần Công Danh · lore

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

On 2020-05-28 19:21:47+0700, Đoàn Trần Công Danh <congdanhqx@gmail.com> wrote:
Show 13 quoted lines
> 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.

Show 8 quoted lines
> 
> 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
Đoàn Trần Công Danh· May 28, 2020, 15:55 UTC · re: Đoàn Trần Công Danh · lore

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

On 2020-05-28 21:01:42+0700, Đoàn Trần Công Danh <congdanhqx@gmail.com> wrote:
Show 47 quoted lines
> 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
Show 17 quoted lines
> 	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<---------

Show changes to builtin/submodule--helper.c +2 −2
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
Shourya Shukla· Jun 2, 2020, 16:35 UTC · re: Shourya Shukla · lore

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

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(-)
Show changes to 2 files +47 −29

builtin/submodule--helper.c, git-submodule.sh

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
Junio C Hamano· Jun 2, 2020, 17:58 UTC · re: Shourya Shukla · lore

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

Shourya Shukla <shouryashukla.oo@gmail.com> writes:
Show 51 quoted lines
> 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.
Đoàn Trần Công Danh· Jun 3, 2020, 00:12 UTC · re: Junio C Hamano · lore

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

On 2020-06-02 10:58:45-0700, Junio C Hamano <gitster@pobox.com> wrote:
Show 22 quoted lines
> 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
Junio C Hamano· Jun 3, 2020, 20:02 UTC · re: Đoàn Trần Công Danh · lore

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

Đoàn Trần Công Danh  <congdanhqx@gmail.com> writes:
Show 26 quoted lines
> 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".

Shourya Shukla· Jun 4, 2020, 07:17 UTC · re: Junio C Hamano · lore

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

On 03/06 01:02, Junio C Hamano wrote:
Show 16 quoted lines
> 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?

Christian Couder· Jun 4, 2020, 07:49 UTC · re: Shourya Shukla · lore

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

On Thu, Jun 4, 2020 at 9:17 AM Shourya Shukla <shouryashukla.oo@gmail.com> wrote:

Show 21 quoted lines
>
> 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.

Junio C Hamano· Jun 4, 2020, 15:03 UTC · re: Shourya Shukla · lore

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

Shourya Shukla <shouryashukla.oo@gmail.com> writes:
Show 20 quoted lines
> 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.

Kaartic Sivaraam· Jun 2, 2020, 19:01 UTC · re: Shourya Shukla · lore

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

On 02-06-2020 22:05, Shourya Shukla wrote:
Show 52 quoted lines
> 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:

Show 27 quoted lines
>  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
Kaartic Sivaraam· Jun 2, 2020, 19:10 UTC · re: Kaartic Sivaraam · lore

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

On Wed, Jun 3, 2020 at 12:31 AM Kaartic Sivaraam <kaartic.sivaraam@gmail.com> wrote:

Show 41 quoted lines
>
> 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>
Show 11 quoted lines
> ... respectively.
>
> > +             NULL
> > +     };
>
> ---
> Footnotes:
>
> [1]:
> https://github.com/periperidip/git/commit/9a8918bf0688c583740b3dddafdba82f47972442#r39606384
>
-- 
Sivaraam
Christian Couder· Jun 2, 2020, 19:45 UTC · re: Kaartic Sivaraam · lore

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

On Tue, Jun 2, 2020 at 9:01 PM Kaartic Sivaraam <kaartic.sivaraam@gmail.com> wrote:

Show 69 quoted lines
>
> 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'.

[...]
Show 9 quoted lines
> 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.
Shourya Shukla· Jun 4, 2020, 07:09 UTC · re: Christian Couder · lore

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

On 02/06 09:45, Christian Couder wrote:
Show 31 quoted lines
> >
> > 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.

Kaartic Sivaraam· Jun 4, 2020, 19:26 UTC · re: Christian Couder · lore

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

On 03-06-2020 01:15, Christian Couder wrote:
Show 34 quoted lines
> 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.
Show 19 quoted lines
> [...]
> 
>> 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:

Show 5 quoted lines
> +	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

← back to recent threads