threads / patch / 53464

patch, 2 partssubmodule: port subcommand 'set-branch' from shell to C

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

## tl;dr

14 messages between May 13, 2020 and May 17, 2020. Diffs are folded; open one to read it.

replies: 13people: 5as markdown or json

Shourya Shukla· May 13, 2020, 20:17 UTC · lore

Convert submodule subcommand 'set-branch' to a builtin. Port 'set-branch' to 'submodule--helper.c' and call the latter via 'git-submodule.sh'.

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>
---
Here is another conversion, this time it is 'set-branch'. It passes all
the tests in t7419. I am aware that there are some repetitve parts in
the conversion as well as variables which can be named better. I would
love everyone's suggestion on this and how this can be made better.

The extra '$branch' on line 752 was because of Christian's help after reference from TLDP's Parameter Subsitution documentation: https://www.tldp.org/LDP/abs/html/parameter-substitution.html

Similarly, I had to change a coouple of other lines in the shell version so as to make it compatible with the C version.

Thank you so much Christian and Kaartic for the mentoring, this wouldn't have been possible otherwise :)

 builtin/submodule--helper.c | 58 +++++++++++++++++++++++++++++++++++++
 git-submodule.sh            | 31 ++------------------
 2 files changed, 60 insertions(+), 29 deletions(-)
Show changes to 2 files +60 −29

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

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index f50745a03f..5a8815b76e 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2284,6 +2284,63 @@ 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 quiet = 0, opt_branch = 0, opt_default = 0;
+	const char *newbranch;
+	const char *path;
+	char *config_name;
+
+	struct option options[] = {
+		OPT__QUIET(&quiet, N_("Suppress output for setting default tracking branch of a submodule")),
+		OPT_BOOL(0, "default", &opt_default, N_("Set the default tracking branch to master")),
+		OPT_BOOL(0, "branch", &opt_branch, N_("Set the default tracking branch to the one specified")),
+		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(_("At least one of --branch and --default required"));
+
+	if (opt_branch) {
+		if (opt_default)
+			die(_("--branch and --default do not make sense"));
+
+		newbranch = argv[0];
+		path = argv[1];
+
+		if (argc != 2 || !(newbranch = argv[0]) || !(path = argv[1]))
+			usage_with_options(usage, options);
+
+		config_name = xstrfmt("submodule.%s.branch", path);
+		config_set_in_gitmodules_file_gently(config_name, newbranch);
+
+		printf(_("Default tracking branch set to '%s' successfully\n"), newbranch);
+		free(config_name);
+	}
+
+	if (opt_default) {
+		path = argv[0];
+
+		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, NULL);
+
+		printf(_("Default tracking branch set to 'master' successfully\n"));
+		free(config_name);
+	}
+
+	return 0;
+}
+
 #define SUPPORT_SUPER_PREFIX (1<<0)
 
 struct cmd_struct {
@@ -2315,6 +2372,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..2438ef576e 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -719,7 +719,6 @@ cmd_update()
 # $@ = requested path
 #
 cmd_set_branch() {
-	unset_branch=false
 	branch=
 
 	while test $# -ne 0
@@ -729,7 +728,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 +749,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
Shourya Shukla· May 13, 2020, 20:17 UTC · re: Shourya Shukla · lore

[PATCH 2/2] submodule: Add 'quiet' option in subcommand 'set-branch'

The subcommand 'set-branch' had the 'quiet' option which was introduced in b57e8119e6 by Denton Liu but was never utilised due to not setting of the 'GIT_QUIET' variable. Add functionality to utilise the 'quiet' function.

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>
---
There was an existence of the `quiet` option in the shell version of
'set-branch' but it was not used anywhere. I decided to add a utility
of the option here by setting GIT_QUIET to 1 in case of a `quiet` as
well as ensure proper functioning in the C version regarding the same.
The if-statement is inspired from what Junio suggested me in my previous
conversion of 'set-url'.
 builtin/submodule--helper.c | 6 ++++--
 git-submodule.sh            | 2 +-
 2 files changed, 5 insertions(+), 3 deletions(-)
Show changes to 2 files +5 −3

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

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 5a8815b76e..36b69df5c4 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2321,7 +2321,8 @@ static int module_set_branch(int argc, const char **argv, const char *prefix)
 		config_name = xstrfmt("submodule.%s.branch", path);
 		config_set_in_gitmodules_file_gently(config_name, newbranch);
 
-		printf(_("Default tracking branch set to '%s' successfully\n"), newbranch);
+		if (!(quiet ? OPT_QUIET : 0))
+			printf(_("Default tracking branch set to '%s' successfully\n"), newbranch);
 		free(config_name);
 	}
 
@@ -2334,7 +2335,8 @@ static int module_set_branch(int argc, const char **argv, const char *prefix)
 		config_name = xstrfmt("submodule.%s.branch", path);
 		config_set_in_gitmodules_file_gently(config_name, NULL);
 
-		printf(_("Default tracking branch set to 'master' successfully\n"));
+		if (!(quiet ? OPT_QUIET : 0))
+			printf(_("Default tracking branch set to 'master' successfully\n"));
 		free(config_name);
 	}
 
diff --git a/git-submodule.sh b/git-submodule.sh
index 2438ef576e..0cdc77ace6 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -725,7 +725,7 @@ cmd_set_branch() {
 	do
 		case "$1" in
 		-q|--quiet)
-			# we don't do anything with this but we need to accept it
+			GIT_QUIET=1
 			;;
 		-d|--default)
 			default=1
-- 
2.26.2
Denton Liu· May 14, 2020, 10:15 UTC · re: Shourya Shukla · lore

Re: [PATCH 2/2] submodule: Add 'quiet' option in subcommand 'set-branch'

Hi Shourya,

I'm not really sure if we should have this patch at all since I don't think that set-branch should be printing anything at all.

But I'll give some comments anyway. Hopefully they'll be enlightening.
On Thu, May 14, 2020 at 01:47:37AM +0530, Shourya Shukla wrote:
> The subcommand 'set-branch' had the 'quiet' option which was
> introduced in b57e8119e6 by Denton Liu but was never utilised due to

We typically refer to commits by the "reference" format. You can get that as follows:

	$ git show --pretty=ref -s b57e8119e6
	b57e8119e6 (submodule: teach set-branch subcommand, 2019-02-08)

In addition, I don't think it's necessary to mention me by name in this case.

Show 28 quoted lines
> not setting of the 'GIT_QUIET' variable. Add functionality to
> utilise the 'quiet' function.
> 
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
> Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>
> ---
> There was an existence of the `quiet` option in the shell version of
> 'set-branch' but it was not used anywhere. I decided to add a utility
> of the option here by setting GIT_QUIET to 1 in case of a `quiet` as
> well as ensure proper functioning in the C version regarding the same.
> The if-statement is inspired from what Junio suggested me in my previous
> conversion of 'set-url'.
> 
>  builtin/submodule--helper.c | 6 ++++--
>  git-submodule.sh            | 2 +-
>  2 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 5a8815b76e..36b69df5c4 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2321,7 +2321,8 @@ static int module_set_branch(int argc, const char **argv, const char *prefix)
>  		config_name = xstrfmt("submodule.%s.branch", path);
>  		config_set_in_gitmodules_file_gently(config_name, newbranch);
>  
> -		printf(_("Default tracking branch set to '%s' successfully\n"), newbranch);
> +		if (!(quiet ? OPT_QUIET : 0))
This is needlessly complicated... Can't this just be written as
	if (!quiet)
Show 30 quoted lines
> +			printf(_("Default tracking branch set to '%s' successfully\n"), newbranch);
>  		free(config_name);
>  	}
>  
> @@ -2334,7 +2335,8 @@ static int module_set_branch(int argc, const char **argv, const char *prefix)
>  		config_name = xstrfmt("submodule.%s.branch", path);
>  		config_set_in_gitmodules_file_gently(config_name, NULL);
>  
> -		printf(_("Default tracking branch set to 'master' successfully\n"));
> +		if (!(quiet ? OPT_QUIET : 0))
> +			printf(_("Default tracking branch set to 'master' successfully\n"));
>  		free(config_name);
>  	}
>  
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 2438ef576e..0cdc77ace6 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -725,7 +725,7 @@ cmd_set_branch() {
>  	do
>  		case "$1" in
>  		-q|--quiet)
> -			# we don't do anything with this but we need to accept it
> +			GIT_QUIET=1
>  			;;
>  		-d|--default)
>  			default=1
> -- 
> 2.26.2
> 
Shourya Shukla· May 16, 2020, 05:50 UTC · re: Denton Liu · lore

Re: [PATCH 2/2] submodule: Add 'quiet' option in subcommand 'set-branch'

On 14/05 06:15, Denton Liu wrote:
> Hi Shourya,
> 
> I'm not really sure if we should have this patch at all since I don't
> think that set-branch should be printing anything at all.

I thought that the Documentation has the mention of the `quiet` and it wouldn't harm printing something when the branch is set. Is this not the right way to tackle this?

Show 14 quoted lines
> But I'll give some comments anyway. Hopefully they'll be enlightening.
> 
> On Thu, May 14, 2020 at 01:47:37AM +0530, Shourya Shukla wrote:
> > The subcommand 'set-branch' had the 'quiet' option which was
> > introduced in b57e8119e6 by Denton Liu but was never utilised due to
> 
> We typically refer to commits by the "reference" format. You can get
> that as follows:
> 
> 	$ git show --pretty=ref -s b57e8119e6
> 	b57e8119e6 (submodule: teach set-branch subcommand, 2019-02-08)
> 
> In addition, I don't think it's necessary to mention me by name in this
> case.
Okay, I did not know that, will change in the next version.
Show 6 quoted lines
> > -		printf(_("Default tracking branch set to '%s' successfully\n"), newbranch);
> > +		if (!(quiet ? OPT_QUIET : 0))
> 
> This is needlessly complicated... Can't this just be written as
> 
> 	if (!quiet)
Okay, will do.
Denton Liu· May 16, 2020, 08:56 UTC · re: Shourya Shukla · lore

Re: [PATCH 2/2] submodule: Add 'quiet' option in subcommand 'set-branch'

On Sat, May 16, 2020 at 11:20:16AM +0530, Shourya Shukla wrote:
Show 9 quoted lines
> On 14/05 06:15, Denton Liu wrote:
> > Hi Shourya,
> > 
> > I'm not really sure if we should have this patch at all since I don't
> > think that set-branch should be printing anything at all.
> 
> I thought that the Documentation has the mention of the `quiet` and it
> wouldn't harm printing something when the branch is set. Is this not the
> right way to tackle this?

I would argue that it's unnecessary to print anything. If a user provides an invalid option, then an error message will be printed. If no error is provided, then a user would assume that the command succeeded. Take, for example, `git submodule set-url` which behaves similarly. It's silent on success. Printing on success would just be noise.

Shourya Shukla· May 16, 2020, 10:40 UTC · re: Denton Liu · lore

Re: [PATCH 2/2] submodule: Add 'quiet' option in subcommand 'set-branch'

On 16/05 04:56, Denton Liu wrote:
Show 5 quoted lines
> I would argue that it's unnecessary to print anything. If a user
> provides an invalid option, then an error message will be printed. If no
> error is provided, then a user would assume that the command succeeded.
> Take, for example, `git submodule set-url` which behaves similarly. It's
> silent on success. Printing on success would just be noise.

Okay, I have dropped the commit. When exactly is printing on success not deemed as noise?

Denton Liu· May 16, 2020, 11:06 UTC · re: Shourya Shukla · lore

Re: [PATCH 2/2] submodule: Add 'quiet' option in subcommand 'set-branch'

On Sat, May 16, 2020 at 04:10:15PM +0530, Shourya Shukla wrote:
Show 9 quoted lines
> On 16/05 04:56, Denton Liu wrote:
> > I would argue that it's unnecessary to print anything. If a user
> > provides an invalid option, then an error message will be printed. If no
> > error is provided, then a user would assume that the command succeeded.
> > Take, for example, `git submodule set-url` which behaves similarly. It's
> > silent on success. Printing on success would just be noise.
> 
> Okay, I have dropped the commit. When exactly is printing on success not
> deemed as noise?

There's not really a hard rule on this. As a rule of thumb, though, if it's between a success and a failure, the failure should print and the success should be silent. If the user is requesting some information, then it should always print the information. There are probably some commands that are exceptions to these rules, though.

Kaartic Sivaraam· May 14, 2020, 09:07 UTC · re: Shourya Shukla · lore

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

-Cc: chriscool@tuxfamily.org (seems redundant; Also, .mailmap confirms that christian.couder@gmail.com is Christian's preference)

On 14-05-2020 01:47, Shourya Shukla wrote:
Show 5 quoted lines
> 
> The extra '$branch' on line 752 was because of Christian's help after
> reference from TLDP's Parameter Subsitution documentation:
> https://www.tldp.org/LDP/abs/html/parameter-substitution.html
>

For those who lack the context, during the conversion of the script Shourya faced an issue where the '--branch' argument did not work as expected. He described the issue in a private e-mail where Christian pointed out the following (quoting his reply hoping he doesn't mind):

 > On Tue, May 12, 2020 at 5:55 PM Christian Couder
<christian.couder@gmail.com> wrote:
 >>
 >> In your commit (in submodule.sh line 781) there is:
 >>
 >>         `git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix
 >> "$prefix"} submodule--helper set-branch ${GIT_QUIET:+--quiet}
 >> ${branch:+--branch} ${default:+--default} -- "$@"`
 >
 > Actually the issue might come from the above. I think it should use 
${branch:+--branch $branch} instead of ${branch:+--branch}.
 >
 > See: https://www.tldp.org/LDP/abs/html/parameter-substitution.html

That's why Shourya mentions the '$branch' as extra. Of course, that's how it is supposed to be in the first place :)

-- 
Sivaraam
Junio C Hamano· May 17, 2020, 15:06 UTC · re: Kaartic Sivaraam · lore

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

Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes:
Show 21 quoted lines
> For those who lack the context, during the conversion of the script
> Shourya faced an issue where the '--branch' argument did not work as
> expected. He described the issue in a private e-mail where Christian
> pointed out the following (quoting his reply hoping he doesn't mind):
>
>> On Tue, May 12, 2020 at 5:55 PM Christian Couder
> <christian.couder@gmail.com> wrote:
>>>
>>> In your commit (in submodule.sh line 781) there is:
>>>
>>>         `git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix
>>> "$prefix"} submodule--helper set-branch ${GIT_QUIET:+--quiet}
>>> ${branch:+--branch} ${default:+--default} -- "$@"`
>>
>> Actually the issue might come from the above. I think it should use 
> ${branch:+--branch $branch} instead of ${branch:+--branch}.
>>
>> See: https://www.tldp.org/LDP/abs/html/parameter-substitution.html
>
> That's why Shourya mentions the '$branch' as extra. Of course, that's
> how it is supposed to be in the first place :)

Perhaps all of that should be removed from the log message and instead go after the three-dash line then.

Thanks.
Kaartic Sivaraam· May 17, 2020, 15:21 UTC · re: Junio C Hamano · lore

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

On 17-05-2020 20:36, Junio C Hamano wrote:
Show 27 quoted lines
> Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes:
> 
>> For those who lack the context, during the conversion of the script
>> Shourya faced an issue where the '--branch' argument did not work as
>> expected. He described the issue in a private e-mail where Christian
>> pointed out the following (quoting his reply hoping he doesn't mind):
>>
>>> On Tue, May 12, 2020 at 5:55 PM Christian Couder
>> <christian.couder@gmail.com> wrote:
>>>>
>>>> In your commit (in submodule.sh line 781) there is:
>>>>
>>>>          `git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix
>>>> "$prefix"} submodule--helper set-branch ${GIT_QUIET:+--quiet}
>>>> ${branch:+--branch} ${default:+--default} -- "$@"`
>>>
>>> Actually the issue might come from the above. I think it should use
>> ${branch:+--branch $branch} instead of ${branch:+--branch}.
>>>
>>> See: https://www.tldp.org/LDP/abs/html/parameter-substitution.html
>>
>> That's why Shourya mentions the '$branch' as extra. Of course, that's
>> how it is supposed to be in the first place :)
> 
> Perhaps all of that should be removed from the log message and
> instead go after the three-dash line then.
> 
I believe it's already after the three-dash line. The log message reads:
 > Convert submodule subcommand 'set-branch' to a builtin. Port 'set-branch'
 > to 'submodule--helper.c' and call the latter via 'git-submodule.sh'.
 >
 > Mentored-by: Christian Couder<chriscool@tuxfamily.org>
 > Mentored-by: Kaartic Sivaraam<kaartic.sivaraam@gmail.com>
 > Signed-off-by: Shourya Shukla<shouryashukla.oo@gmail.com>
It might use more explanation, maybe.
-- 
Sivaraam
Denton Liu· May 14, 2020, 10:10 UTC · re: Shourya Shukla · lore

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

Hi Shourya,
On Thu, May 14, 2020 at 01:47:36AM +0530, Shourya Shukla wrote:
Show 38 quoted lines
> Convert submodule subcommand 'set-branch' to a builtin. Port 'set-branch'
> to 'submodule--helper.c' and call the latter via 'git-submodule.sh'.
> 
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
> Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>
> ---
> Here is another conversion, this time it is 'set-branch'. It passes all
> the tests in t7419. I am aware that there are some repetitve parts in
> the conversion as well as variables which can be named better. I would
> love everyone's suggestion on this and how this can be made better.
> 
> The extra '$branch' on line 752 was because of Christian's help after
> reference from TLDP's Parameter Subsitution documentation:
> https://www.tldp.org/LDP/abs/html/parameter-substitution.html
> 
> Similarly, I had to change a coouple of other lines in the shell version
> so as to make it compatible with the C version.
> 
> Thank you so much Christian and Kaartic for the mentoring, this wouldn't
> have been possible otherwise :)
> 
>  builtin/submodule--helper.c | 58 +++++++++++++++++++++++++++++++++++++
>  git-submodule.sh            | 31 ++------------------
>  2 files changed, 60 insertions(+), 29 deletions(-)
> 
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index f50745a03f..5a8815b76e 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2284,6 +2284,63 @@ 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 quiet = 0, opt_branch = 0, opt_default = 0;
> +	const char *newbranch;
nit: I would call this new_branch
Show 7 quoted lines
> +	const char *path;
> +	char *config_name;
> +
> +	struct option options[] = {
> +		OPT__QUIET(&quiet, N_("Suppress output for setting default tracking branch of a submodule")),
> +		OPT_BOOL(0, "default", &opt_default, N_("Set the default tracking branch to master")),
> +		OPT_BOOL(0, "branch", &opt_branch, N_("Set the default tracking branch to the one specified")),

This should use OPT_STRING and accept a string argument instead of using the implicit command-line ordering.

Show 12 quoted lines
> +		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(_("At least one of --branch and --default required"));

Error messages in Git are generally written without capitalising the first letter of the sentence.

> +
> +	if (opt_branch) {
> +		if (opt_default)
> +			die(_("--branch and --default do not make sense"));

This error message should be qualified, perhaps with something like "do not make sense together".

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

These assignments are incorrect since we haven't check argc yet. Also, they're redundant since you have the assignments in the if statement below.

Also, if you do the OPT_STRING thing as described above, you can do the `path = ...` outside of the if-statement since it'll be common to both -d and -b.

Show 8 quoted lines
> +
> +		if (argc != 2 || !(newbranch = argv[0]) || !(path = argv[1]))
> +			usage_with_options(usage, options);
> +
> +		config_name = xstrfmt("submodule.%s.branch", path);
> +		config_set_in_gitmodules_file_gently(config_name, newbranch);
> +
> +		printf(_("Default tracking branch set to '%s' successfully\n"), newbranch);

The original function did not print anything. We shouldn't alter the behaviour if we're just porting it over so we should delete this.

Show 15 quoted lines
> +		free(config_name);
> +	}
> +
> +	if (opt_default) {
> +		path = argv[0];
> +
> +		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, NULL);
> +
> +		printf(_("Default tracking branch set to 'master' successfully\n"));
> +		free(config_name);
> +	}

The same arguments for the above apply to this case too. Actually, the only place where they both really differ is in the call to config_set_in_gitmodules_file_gently(). Can you do all of your argument checks together? Something like

	if (!!new_branch == opt_default)
		usage_with_options(usage, options);

Then the call to config_set_in_gitmodules_file_gently() could look like this:

	config_name = xstrfmt("submodule.%s.branch", path);
	config_set_in_gitmodules_file_gently(config_name, new_branch ? new_branch : NULL);
	free(config_name);
and we wouldn't need the ifs at all.
Show 68 quoted lines
> +
> +	return 0;
> +}
> +
>  #define SUPPORT_SUPER_PREFIX (1<<0)
>  
>  struct cmd_struct {
> @@ -2315,6 +2372,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..2438ef576e 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -719,7 +719,6 @@ cmd_update()
>  # $@ = requested path
>  #
>  cmd_set_branch() {
> -	unset_branch=false
>  	branch=
>  
>  	while test $# -ne 0
> @@ -729,7 +728,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 +749,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} -- "$@"
The shell script portion looks good.
Thanks,
Denton
Show 6 quoted lines
>  }
>  
>  #
> -- 
> 2.26.2
> 
Shourya Shukla· May 16, 2020, 10:37 UTC · re: Denton Liu · lore

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

On 14/05 06:10, Denton Liu wrote:
Show 10 quoted lines
> > +	const char *path;
> > +	char *config_name;
> > +
> > +	struct option options[] = {
> > +		OPT__QUIET(&quiet, N_("Suppress output for setting default tracking branch of a submodule")),
> > +		OPT_BOOL(0, "default", &opt_default, N_("Set the default tracking branch to master")),
> > +		OPT_BOOL(0, "branch", &opt_branch, N_("Set the default tracking branch to the one specified")),
> 
> This should use OPT_STRING and accept a string argument instead of using
> the implicit command-line ordering.

I actually was not able to understand the point of this change until I tried it out myself. It has made the code more aethetic as well as less redundant. Thanks a ton!

Show 15 quoted lines
> > +		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(_("At least one of --branch and --default required"));
> 
> Error messages in Git are generally written without capitalising the
> first letter of the sentence.

Corrected. BTW, many other subcommands have this problem (the error messages as well as the options start with a caps and end with a fullstop). Should they be corrected or let them be as is?

Show 7 quoted lines
> > +
> > +	if (opt_branch) {
> > +		if (opt_default)
> > +			die(_("--branch and --default do not make sense"));
> 
> This error message should be qualified, perhaps with something like "do
> not make sense together".
Done!
Show 17 quoted lines
> The same arguments for the above apply to this case too. Actually, the
> only place where they both really differ is in the call to
> config_set_in_gitmodules_file_gently(). Can you do all of your argument
> checks together? Something like
> 
> 	if (!!new_branch == opt_default)
> 		usage_with_options(usage, options);
> 
> Then the call to config_set_in_gitmodules_file_gently() could look like
> this:
> 
> 	config_name = xstrfmt("submodule.%s.branch", path);
> 	config_set_in_gitmodules_file_gently(config_name, new_branch ? new_branch : NULL);
> 	free(config_name);
> 
> and we wouldn't need the ifs at all.
> 
Sure, I have made the changes.

Regards, Shourya Shukla

Denton Liu· May 16, 2020, 10:55 UTC · re: Shourya Shukla · lore

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

On Sat, May 16, 2020 at 04:07:44PM +0530, Shourya Shukla wrote:
Show 7 quoted lines
> On 14/05 06:10, Denton Liu wrote:
> > Error messages in Git are generally written without capitalising the
> > first letter of the sentence.
> 
> Corrected. BTW, many other subcommands have this problem (the error
> messages as well as the options start with a caps and end with a
> fullstop). Should they be corrected or let them be as is?

If it's already written, it's probably fine to leave it as is. Although if you're working in an area with some messages that could be fixed, it might be good to clean them up as a preparatory patch.

Đoàn Trần Công Danh· May 17, 2020, 16:11 UTC · re: Shourya Shukla · lore

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

On 2020-05-14 01:47:36+0530, Shourya Shukla <shouryashukla.oo@gmail.com> wrote:
Show 18 quoted lines
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 39ebdf25b5..2438ef576e 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -719,7 +719,6 @@ cmd_update()
>  # $@ = requested path
>  #
>  cmd_set_branch() {
> -	unset_branch=false
>  	branch=
>  
>  	while test $# -ne 0
> @@ -729,7 +728,7 @@ cmd_set_branch() {
>  			# we don't do anything with this but we need to accept it
>  			;;
>  		-d|--default)
> -			unset_branch=true
> +			default=1
Hi Shourya,
Thanks for your hard work,
I skimmed over the current code, it seems like this:
	default=1
is new.

If my understanding is correct, please reset its value in the beginning of this function, to avoid:

- a side effect from an assignment in get_submodule_config
  (if there's such a side effect, I'm NOT really sure).
- effect of an environment variable, c.f.
  https://lore.kernel.org/git/20200402084251.85840-1-zhiyou.jx@alibaba-inc.com/
-- 
Danh

← back to recent threads