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

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

[Shourya Shukla]

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

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

[Shourya Shukla]

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(-)

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

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

[Kaartic Sivaraam]

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

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

[Denton Liu]

Hi Shourya,

On Thu, May 14, 2020 at 01:47:36AM +0530, Shourya Shukla wrote:
> 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

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

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

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

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

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

>  }
>  
>  #
> -- 
> 2.26.2
> 

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

[Denton Liu]

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.

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

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

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

[Shourya Shukla]

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?

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

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

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

[Denton Liu]

On Sat, May 16, 2020 at 11:20:16AM +0530, Shourya Shukla wrote:
> 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.

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

[Shourya Shukla]

On 14/05 06:10, Denton Liu wrote:
> > +	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!

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

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

> 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

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

[Shourya Shukla]

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?

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

[Denton Liu]

On Sat, May 16, 2020 at 04:07:44PM +0530, Shourya Shukla wrote:
> 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.

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

[Denton Liu]

On Sat, May 16, 2020 at 04:10:15PM +0530, Shourya Shukla wrote:
> 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.

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

[Junio C Hamano]

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.

Thanks.

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

[Kaartic Sivaraam]

On 17-05-2020 20:36, Junio C Hamano wrote:
> 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

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

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

On 2020-05-14 01:47:36+0530, Shourya Shukla <shouryashukla.oo@gmail.com> wrote:
> 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