git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / Atom feed
* [PATCH v3] submodule: port subcommand 'set-branch' from shell to C
@ 2020-05-21 16:38 Shourya Shukla
  2020-05-21 18:44 ` Junio C Hamano
                   ` (3 more replies)
  0 siblings, 4 replies; 17+ messages in thread
From: Shourya Shukla @ 2020-05-21 16:38 UTC (permalink / raw)
  To: git
  Cc: christian.couder, kaartic.sivaraam, liu.denton, gitster,
	congdanhqx, sunshine, Shourya Shukla, Christian Couder

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

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
Helped-by: Denton Liu <liu.denton@gmail.com>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>
---
Thank you for the review Eric. I have changed the commit message,
and the error prompts. Also, I have added a brief comment about
the `quiet` option.

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

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index f50745a03f..d14b9856a3 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2284,6 +2284,50 @@ static int module_set_url(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
+static int module_set_branch(int argc, const char **argv, const char *prefix)
+{
+	/*
+	 * The `quiet` option is present for backward compatibility
+	 * but is currently not used.
+	 */
+	int quiet = 0, opt_default = 0;
+	const char *opt_branch = NULL;
+	const char *path;
+	char *config_name;
+
+	struct option options[] = {
+		OPT__QUIET(&quiet,
+			N_("suppress output for setting default tracking branch")),
+		OPT_BOOL(0, "default", &opt_default,
+			N_("set the default tracking branch to master")),
+		OPT_STRING(0, "branch", &opt_branch, N_("branch"),
+			N_("set the default tracking branch")),
+		OPT_END()
+	};
+	const char *const usage[] = {
+		N_("git submodule--helper set-branch [--quiet] (-d|--default) <path>"),
+		N_("git submodule--helper set-branch [--quiet] (-b|--branch) <branch> <path>"),
+		NULL
+	};
+
+	argc = parse_options(argc, argv, prefix, options, usage, 0);
+
+	if (!opt_branch && !opt_default)
+		die(_("--branch or --default required"));
+
+	if (opt_branch && opt_default)
+		die(_("--branch and --default are mutually exclusive"));
+
+	if (argc != 1 || !(path = argv[0]))
+		usage_with_options(usage, options);
+
+	config_name = xstrfmt("submodule.%s.branch", path);
+	config_set_in_gitmodules_file_gently(config_name, opt_branch);
+
+	free(config_name);
+	return 0;
+}
+
 #define SUPPORT_SUPER_PREFIX (1<<0)
 
 struct cmd_struct {
@@ -2315,6 +2359,7 @@ static struct cmd_struct commands[] = {
 	{"check-name", check_name, 0},
 	{"config", module_config, 0},
 	{"set-url", module_set_url, 0},
+	{"set-branch", module_set_branch, 0},
 };
 
 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
diff --git a/git-submodule.sh b/git-submodule.sh
index 39ebdf25b5..8c56191f77 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -719,7 +719,7 @@ cmd_update()
 # $@ = requested path
 #
 cmd_set_branch() {
-	unset_branch=false
+	default=
 	branch=
 
 	while test $# -ne 0
@@ -729,7 +729,7 @@ cmd_set_branch() {
 			# we don't do anything with this but we need to accept it
 			;;
 		-d|--default)
-			unset_branch=true
+			default=1
 			;;
 		-b|--branch)
 			case "$2" in '') usage ;; esac
@@ -750,33 +750,7 @@ cmd_set_branch() {
 		shift
 	done
 
-	if test $# -ne 1
-	then
-		usage
-	fi
-
-	# we can't use `git submodule--helper name` here because internally, it
-	# hashes the path so a trailing slash could lead to an unintentional no match
-	name="$(git submodule--helper list "$1" | cut -f2)"
-	if test -z "$name"
-	then
-		exit 1
-	fi
-
-	test -n "$branch"; has_branch=$?
-	test "$unset_branch" = true; has_unset_branch=$?
-
-	if test $((!$has_branch != !$has_unset_branch)) -eq 0
-	then
-		usage
-	fi
-
-	if test $has_branch -eq 0
-	then
-		git submodule--helper config submodule."$name".branch "$branch"
-	else
-		git submodule--helper config --unset submodule."$name".branch
-	fi
+	git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper set-branch ${GIT_QUIET:+--quiet} ${branch:+--branch $branch} ${default:+--default} -- "$@"
 }
 
 #
-- 
2.26.2


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3] submodule: port subcommand 'set-branch' from shell to C
  2020-05-21 16:38 [PATCH v3] submodule: port subcommand 'set-branch' from shell to C Shourya Shukla
@ 2020-05-21 18:44 ` Junio C Hamano
  2020-05-21 19:03   ` Denton Liu
  2020-05-21 23:04 ` Đoàn Trần Công Danh
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 17+ messages in thread
From: Junio C Hamano @ 2020-05-21 18:44 UTC (permalink / raw)
  To: Shourya Shukla
  Cc: git, christian.couder, kaartic.sivaraam, liu.denton, congdanhqx,
	sunshine, Christian Couder

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?





^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3] submodule: port subcommand 'set-branch' from shell to C
  2020-05-21 18:44 ` Junio C Hamano
@ 2020-05-21 19:03   ` Denton Liu
  2020-05-21 19:50     ` Junio C Hamano
  0 siblings, 1 reply; 17+ messages in thread
From: Denton Liu @ 2020-05-21 19:03 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Shourya Shukla, git, christian.couder, kaartic.sivaraam,
	congdanhqx, sunshine, Christian Couder

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

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

	$ git submodule -q sync

or
	$ git submodule sync -q

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

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3] submodule: port subcommand 'set-branch' from shell to C
  2020-05-21 19:03   ` Denton Liu
@ 2020-05-21 19:50     ` Junio C Hamano
  2020-05-22 19:39       ` Shourya Shukla
  0 siblings, 1 reply; 17+ messages in thread
From: Junio C Hamano @ 2020-05-21 19:50 UTC (permalink / raw)
  To: Denton Liu
  Cc: Shourya Shukla, git, christian.couder, kaartic.sivaraam,
	congdanhqx, sunshine, Christian Couder

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

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

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

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


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3] submodule: port subcommand 'set-branch' from shell to C
  2020-05-21 16:38 [PATCH v3] submodule: port subcommand 'set-branch' from shell to C Shourya Shukla
  2020-05-21 18:44 ` Junio C Hamano
@ 2020-05-21 23:04 ` Đoàn Trần Công Danh
  2020-05-22 22:21 ` Johannes Schindelin
  2020-05-23 16:39 ` [PATCH v4] " Shourya Shukla
  3 siblings, 0 replies; 17+ messages in thread
From: Đoàn Trần Công Danh @ 2020-05-21 23:04 UTC (permalink / raw)
  To: Shourya Shukla
  Cc: git, christian.couder, kaartic.sivaraam, liu.denton, gitster,
	sunshine, Christian Couder

Hi Shourya,

On 2020-05-21 22:08:19+0530, Shourya Shukla <shouryashukla.oo@gmail.com> wrote:
> Thank you for the review Eric. I have changed the commit message,
> and the error prompts. Also, I have added a brief comment about
> the `quiet` option.
> 
> +	/*
> +	 * The `quiet` option is present for backward compatibility
> +	 * but is currently not used.
> +	 */
> +	int quiet = 0, opt_default = 0;
> +	const char *opt_branch = NULL;
> +	const char *path;
> +	char *config_name;
> +
> +	struct option options[] = {
> +		OPT__QUIET(&quiet,
> +			N_("suppress output for setting default tracking branch")),

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

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

	OPT_NOOP_NOARG(0, "quiet")

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

> +		OPT_BOOL(0, "default", &opt_default,
> +			N_("set the default tracking branch to master")),
> +		OPT_STRING(0, "branch", &opt_branch, N_("branch"),
> +			N_("set the default tracking branch")),
> +		OPT_END()
> +	};
> +	const char *const usage[] = {
> +		N_("git submodule--helper set-branch [--quiet] (-d|--default) <path>"),
> +		N_("git submodule--helper set-branch [--quiet] (-b|--branch) <branch> <path>"),

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

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

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

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

-- 
Danh

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3] submodule: port subcommand 'set-branch' from shell to C
  2020-05-21 19:50     ` Junio C Hamano
@ 2020-05-22 19:39       ` Shourya Shukla
  2020-05-24 16:07         ` Junio C Hamano
  0 siblings, 1 reply; 17+ messages in thread
From: Shourya Shukla @ 2020-05-22 19:39 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: git, christian.couder, kaartic.sivaraam, congdanhqx, sunshine,
	liu.denton

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

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3] submodule: port subcommand 'set-branch' from shell to C
  2020-05-21 16:38 [PATCH v3] submodule: port subcommand 'set-branch' from shell to C Shourya Shukla
  2020-05-21 18:44 ` Junio C Hamano
  2020-05-21 23:04 ` Đoàn Trần Công Danh
@ 2020-05-22 22:21 ` Johannes Schindelin
  2020-05-24 23:15   ` Junio C Hamano
  2020-05-23 16:39 ` [PATCH v4] " Shourya Shukla
  3 siblings, 1 reply; 17+ messages in thread
From: Johannes Schindelin @ 2020-05-22 22:21 UTC (permalink / raw)
  To: Shourya Shukla
  Cc: git, christian.couder, kaartic.sivaraam, liu.denton, gitster,
	congdanhqx, sunshine, Christian Couder

Hi Shourya,

On Thu, 21 May 2020, Shourya Shukla wrote:

> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index f50745a03f..d14b9856a3 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2284,6 +2284,50 @@ static int module_set_url(int argc, const char **argv, const char *prefix)
>  	return 0;
>  }
>
> +static int module_set_branch(int argc, const char **argv, const char *prefix)
> +{
> +	/*
> +	 * The `quiet` option is present for backward compatibility
> +	 * but is currently not used.
> +	 */
> +	int quiet = 0, opt_default = 0;
> +	const char *opt_branch = NULL;
> +	const char *path;
> +	char *config_name;
> +
> +	struct option options[] = {
> +		OPT__QUIET(&quiet,
> +			N_("suppress output for setting default tracking branch")),
> +		OPT_BOOL(0, "default", &opt_default,
> +			N_("set the default tracking branch to master")),
> +		OPT_STRING(0, "branch", &opt_branch, N_("branch"),
> +			N_("set the default tracking branch")),
> +		OPT_END()
> +	};
> +	const char *const usage[] = {
> +		N_("git submodule--helper set-branch [--quiet] (-d|--default) <path>"),
> +		N_("git submodule--helper set-branch [--quiet] (-b|--branch) <branch> <path>"),
> +		NULL
> +	};
> +
> +	argc = parse_options(argc, argv, prefix, options, usage, 0);
> +
> +	if (!opt_branch && !opt_default)
> +		die(_("--branch or --default required"));
> +
> +	if (opt_branch && opt_default)
> +		die(_("--branch and --default are mutually exclusive"));
> +
> +	if (argc != 1 || !(path = argv[0]))
> +		usage_with_options(usage, options);
> +
> +	config_name = xstrfmt("submodule.%s.branch", path);
> +	config_set_in_gitmodules_file_gently(config_name, opt_branch);

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

In other words, you need something like this:

	int ret;

	[...]

	ret = config_set_in_gitmodules_file_gently(config_name, opt_branch);

	free(config_name);
	return ret;

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

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

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

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

Ciao,
Dscho

>  };
>
>  int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 39ebdf25b5..8c56191f77 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -719,7 +719,7 @@ cmd_update()
>  # $@ = requested path
>  #
>  cmd_set_branch() {
> -	unset_branch=false
> +	default=
>  	branch=
>
>  	while test $# -ne 0
> @@ -729,7 +729,7 @@ cmd_set_branch() {
>  			# we don't do anything with this but we need to accept it
>  			;;
>  		-d|--default)
> -			unset_branch=true
> +			default=1
>  			;;
>  		-b|--branch)
>  			case "$2" in '') usage ;; esac
> @@ -750,33 +750,7 @@ cmd_set_branch() {
>  		shift
>  	done
>
> -	if test $# -ne 1
> -	then
> -		usage
> -	fi
> -
> -	# we can't use `git submodule--helper name` here because internally, it
> -	# hashes the path so a trailing slash could lead to an unintentional no match
> -	name="$(git submodule--helper list "$1" | cut -f2)"
> -	if test -z "$name"
> -	then
> -		exit 1
> -	fi
> -
> -	test -n "$branch"; has_branch=$?
> -	test "$unset_branch" = true; has_unset_branch=$?
> -
> -	if test $((!$has_branch != !$has_unset_branch)) -eq 0
> -	then
> -		usage
> -	fi
> -
> -	if test $has_branch -eq 0
> -	then
> -		git submodule--helper config submodule."$name".branch "$branch"
> -	else
> -		git submodule--helper config --unset submodule."$name".branch
> -	fi
> +	git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper set-branch ${GIT_QUIET:+--quiet} ${branch:+--branch $branch} ${default:+--default} -- "$@"
>  }
>
>  #
> --
> 2.26.2
>
>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v4] submodule: port subcommand 'set-branch' from shell to C
  2020-05-21 16:38 [PATCH v3] submodule: port subcommand 'set-branch' from shell to C Shourya Shukla
                   ` (2 preceding siblings ...)
  2020-05-22 22:21 ` Johannes Schindelin
@ 2020-05-23 16:39 ` Shourya Shukla
  2020-05-23 18:49   ` Kaartic Sivaraam
  3 siblings, 1 reply; 17+ messages in thread
From: Shourya Shukla @ 2020-05-23 16:39 UTC (permalink / raw)
  To: shouryashukla.oo
  Cc: christian.couder, congdanhqx, git, gitster, kaartic.sivaraam,
	liu.denton, sunshine, Johannes.Schindelin, Christian Couder

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

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

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

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index f50745a03f..7e844e8971 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2284,6 +2284,50 @@ static int module_set_url(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
+static int module_set_branch(int argc, const char **argv, const char *prefix)
+{
+	/*
+	 * We accept the `quiet` option for uniformity across subcommands,
+	 * though there is nothing to make less verbose in this subcommand.
+	 */
+	int quiet = 0, opt_default = 0, ret;
+	const char *opt_branch = NULL;
+	const char *path;
+	char *config_name;
+
+	struct option options[] = {
+		OPT__QUIET(&quiet,
+			N_("suppress output for setting default tracking branch")),
+		OPT_BOOL(0, "default", &opt_default,
+			N_("set the default tracking branch to master")),
+		OPT_STRING(0, "branch", &opt_branch, N_("branch"),
+			N_("set the default tracking branch")),
+		OPT_END()
+	};
+	const char *const usage[] = {
+		N_("git submodule--helper set-branch [--quiet] (-d|--default) <path>"),
+		N_("git submodule--helper set-branch [--quiet] (-b|--branch) <branch> <path>"),
+		NULL
+	};
+
+	argc = parse_options(argc, argv, prefix, options, usage, 0);
+
+	if (!opt_branch && !opt_default)
+		die(_("--branch or --default required"));
+
+	if (opt_branch && opt_default)
+		die(_("--branch and --default are mutually exclusive"));
+
+	if (argc != 1 || !(path = argv[0]))
+		usage_with_options(usage, options);
+
+	config_name = xstrfmt("submodule.%s.branch", path);
+	ret = config_set_in_gitmodules_file_gently(config_name, opt_branch);
+
+	free(config_name);
+	return ret;
+}
+
 #define SUPPORT_SUPER_PREFIX (1<<0)
 
 struct cmd_struct {
@@ -2315,6 +2359,7 @@ static struct cmd_struct commands[] = {
 	{"check-name", check_name, 0},
 	{"config", module_config, 0},
 	{"set-url", module_set_url, 0},
+	{"set-branch", module_set_branch, 0},
 };
 
 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
diff --git a/git-submodule.sh b/git-submodule.sh
index 39ebdf25b5..8c56191f77 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -719,7 +719,7 @@ cmd_update()
 # $@ = requested path
 #
 cmd_set_branch() {
-	unset_branch=false
+	default=
 	branch=
 
 	while test $# -ne 0
@@ -729,7 +729,7 @@ cmd_set_branch() {
 			# we don't do anything with this but we need to accept it
 			;;
 		-d|--default)
-			unset_branch=true
+			default=1
 			;;
 		-b|--branch)
 			case "$2" in '') usage ;; esac
@@ -750,33 +750,7 @@ cmd_set_branch() {
 		shift
 	done
 
-	if test $# -ne 1
-	then
-		usage
-	fi
-
-	# we can't use `git submodule--helper name` here because internally, it
-	# hashes the path so a trailing slash could lead to an unintentional no match
-	name="$(git submodule--helper list "$1" | cut -f2)"
-	if test -z "$name"
-	then
-		exit 1
-	fi
-
-	test -n "$branch"; has_branch=$?
-	test "$unset_branch" = true; has_unset_branch=$?
-
-	if test $((!$has_branch != !$has_unset_branch)) -eq 0
-	then
-		usage
-	fi
-
-	if test $has_branch -eq 0
-	then
-		git submodule--helper config submodule."$name".branch "$branch"
-	else
-		git submodule--helper config --unset submodule."$name".branch
-	fi
+	git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper set-branch ${GIT_QUIET:+--quiet} ${branch:+--branch $branch} ${default:+--default} -- "$@"
 }
 
 #
-- 
2.26.2


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v4] submodule: port subcommand 'set-branch' from shell to C
  2020-05-23 16:39 ` [PATCH v4] " Shourya Shukla
@ 2020-05-23 18:49   ` Kaartic Sivaraam
  2020-05-23 23:18     ` Đoàn Trần Công Danh
  2020-05-27 17:13     ` Shourya Shukla
  0 siblings, 2 replies; 17+ messages in thread
From: Kaartic Sivaraam @ 2020-05-23 18:49 UTC (permalink / raw)
  To: Shourya Shukla
  Cc: christian.couder, congdanhqx, git, gitster, liu.denton, sunshine,
	Johannes.Schindelin, Christian Couder

Hi Shourya,

I believe you missed Danh's v3 comments[1]. I'm mentioning them inline 
with some additional comments.

On 23-05-2020 22:09, Shourya Shukla wrote:
> 
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index f50745a03f..7e844e8971 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2284,6 +2284,50 @@ static int module_set_url(int argc, const char **argv, const char *prefix)
>   	return 0;
>   }
>   
> +static int module_set_branch(int argc, const char **argv, const char *prefix)
> +{
> +	/*
> +	 * We accept the `quiet` option for uniformity across subcommands,
> +	 * though there is nothing to make less verbose in this subcommand.
> +	 */
> +	int quiet = 0, opt_default = 0, ret;
> +	const char *opt_branch = NULL;
> +	const char *path;
> +	char *config_name;
> +
> +	struct option options[] = {
> +		OPT__QUIET(&quiet,
> +			N_("suppress output for setting default tracking branch")),

As '--quiet' in 'set-branch' is a no-op and is being accepted only for 
uniformity, I think it makes sense to use OPT_NOOP_NOARG instead of 
OPT__QUIET for specifying it, as suggested by Danh.

Also, the description "suppress output for setting default tracking 
branch" doesn't seem to be valid anymore as we don't print anything when 
set-branch succeeds.

> +		OPT_BOOL(0, "default", &opt_default,
> +			N_("set the default tracking branch to master")),
> +		OPT_STRING(0, "branch", &opt_branch, N_("branch"),
> +			N_("set the default tracking branch")),
> +		OPT_END()
> +	};
> +	const char *const usage[] = {
> +		N_("git submodule--helper set-branch [--quiet] (-d|--default) <path>"),
> +		N_("git submodule--helper set-branch [--quiet] (-b|--branch) <branch> <path>"),
> +		NULL
> +	};
> +

I also agree with the Danh here that '--quiet' could be removed from 
usage. There's no point in mentioning '--quiet' in the usage when it has 
no effect.

> diff --git a/git-submodule.sh b/git-submodule.sh
> index 39ebdf25b5..8c56191f77 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -750,33 +750,7 @@ cmd_set_branch() {
>   		shift
>   	done
>   
> -	if test $# -ne 1
> -	then
> -		usage
> -	fi
> -
> -	# we can't use `git submodule--helper name` here because internally, it
> -	# hashes the path so a trailing slash could lead to an unintentional no match
> -	name="$(git submodule--helper list "$1" | cut -f2)"
> -	if test -z "$name"
> -	then
> -		exit 1
> -	fi
> -
> -	test -n "$branch"; has_branch=$?
> -	test "$unset_branch" = true; has_unset_branch=$?
> -
> -	if test $((!$has_branch != !$has_unset_branch)) -eq 0
> -	then
> -		usage
> -	fi
> -
> -	if test $has_branch -eq 0
> -	then
> -		git submodule--helper config submodule."$name".branch "$branch"
> -	else
> -		git submodule--helper config --unset submodule."$name".branch
> -	fi
> +	git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper set-branch ${GIT_QUIET:+--quiet} ${branch:+--branch $branch} ${default:+--default} -- "$@"
>   }
>

Danh questioned whether '$branch' needs to be quoted here. I too think 
it needs to be quoted unless I'm missing something.


---
Footnotes:
[1]: 
https://lore.kernel.org/git/nycvar.QRO.7.76.6.2005230012090.56@tvgsbejvaqbjf.bet/T/#maf26182b084087ed08a2a72d3da2ee2026b1618e

Thanks,
Sivaraam

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v4] submodule: port subcommand 'set-branch' from shell to C
  2020-05-23 18:49   ` Kaartic Sivaraam
@ 2020-05-23 23:18     ` Đoàn Trần Công Danh
  2020-05-27 17:13     ` Shourya Shukla
  1 sibling, 0 replies; 17+ messages in thread
From: Đoàn Trần Công Danh @ 2020-05-23 23:18 UTC (permalink / raw)
  To: Kaartic Sivaraam
  Cc: Shourya Shukla, christian.couder, git, gitster, liu.denton,
	sunshine, Johannes.Schindelin, Christian Couder

Hi Kaartic,

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

Thanks for checking this.

> On 23-05-2020 22:09, Shourya Shukla wrote:
> > 
> > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> > index f50745a03f..7e844e8971 100644
> > --- a/builtin/submodule--helper.c
> > +++ b/builtin/submodule--helper.c
> > @@ -2284,6 +2284,50 @@ static int module_set_url(int argc, const char **argv, const char *prefix)
> >   	return 0;
> >   }
> > +static int module_set_branch(int argc, const char **argv, const char *prefix)
> > +{
> > +	/*
> > +	 * We accept the `quiet` option for uniformity across subcommands,
> > +	 * though there is nothing to make less verbose in this subcommand.
> > +	 */
> > +	int quiet = 0, opt_default = 0, ret;
> > +	const char *opt_branch = NULL;
> > +	const char *path;
> > +	char *config_name;
> > +
> > +	struct option options[] = {
> > +		OPT__QUIET(&quiet,
> > +			N_("suppress output for setting default tracking branch")),
> 
> As '--quiet' in 'set-branch' is a no-op and is being accepted only for
> uniformity, I think it makes sense to use OPT_NOOP_NOARG instead of
> OPT__QUIET for specifying it, as suggested by Danh.

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

	OPT_NOOP_NOARG('q', "quiet")

> Also, the description "suppress output for setting default tracking branch"
> doesn't seem to be valid anymore as we don't print anything when set-branch
> succeeds.

OPT_NOOP_NOARG will take care of description itself. Even if we choose
to not use OPT_NOOP_NOARG, a better description should be provided.

> > +		OPT_BOOL(0, "default", &opt_default,
> > +			N_("set the default tracking branch to master")),
> > +		OPT_STRING(0, "branch", &opt_branch, N_("branch"),
> > +			N_("set the default tracking branch")),
> > +		OPT_END()
> > +	};
> > +	const char *const usage[] = {
> > +		N_("git submodule--helper set-branch [--quiet] (-d|--default) <path>"),
> > +		N_("git submodule--helper set-branch [--quiet] (-b|--branch) <branch> <path>"),
> > +		NULL
> > +	};
> > +
> 
> I also agree with the Danh here that '--quiet' could be removed from usage.
> There's no point in mentioning '--quiet' in the usage when it has no effect.
> 
> > diff --git a/git-submodule.sh b/git-submodule.sh
> > index 39ebdf25b5..8c56191f77 100755
> > --- a/git-submodule.sh
> > +++ b/git-submodule.sh
> > @@ -750,33 +750,7 @@ cmd_set_branch() {
> >   		shift
> >   	done
> > -	if test $# -ne 1
> > -	then
> > -		usage
> > -	fi
> > -
> > -	# we can't use `git submodule--helper name` here because internally, it
> > -	# hashes the path so a trailing slash could lead to an unintentional no match
> > -	name="$(git submodule--helper list "$1" | cut -f2)"
> > -	if test -z "$name"
> > -	then
> > -		exit 1
> > -	fi
> > -
> > -	test -n "$branch"; has_branch=$?
> > -	test "$unset_branch" = true; has_unset_branch=$?
> > -
> > -	if test $((!$has_branch != !$has_unset_branch)) -eq 0
> > -	then
> > -		usage
> > -	fi
> > -
> > -	if test $has_branch -eq 0
> > -	then
> > -		git submodule--helper config submodule."$name".branch "$branch"
> > -	else
> > -		git submodule--helper config --unset submodule."$name".branch
> > -	fi
> > +	git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper set-branch ${GIT_QUIET:+--quiet} ${branch:+--branch $branch} ${default:+--default} -- "$@"
> >   }
> > 
> 
> Danh questioned whether '$branch' needs to be quoted here. I too think it
> needs to be quoted unless I'm missing something.
> 
> 
> ---
> Footnotes:
> [1]: https://lore.kernel.org/git/nycvar.QRO.7.76.6.2005230012090.56@tvgsbejvaqbjf.bet/T/#maf26182b084087ed08a2a72d3da2ee2026b1618e

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

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

-- 
Danh

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3] submodule: port subcommand 'set-branch' from shell to C
  2020-05-22 19:39       ` Shourya Shukla
@ 2020-05-24 16:07         ` Junio C Hamano
  0 siblings, 0 replies; 17+ messages in thread
From: Junio C Hamano @ 2020-05-24 16:07 UTC (permalink / raw)
  To: Shourya Shukla
  Cc: git, christian.couder, kaartic.sivaraam, congdanhqx, sunshine,
	liu.denton

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

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

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

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

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

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


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3] submodule: port subcommand 'set-branch' from shell to C
  2020-05-22 22:21 ` Johannes Schindelin
@ 2020-05-24 23:15   ` Junio C Hamano
  2020-05-24 23:18     ` Junio C Hamano
  0 siblings, 1 reply; 17+ messages in thread
From: Junio C Hamano @ 2020-05-24 23:15 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: Shourya Shukla, git, christian.couder, kaartic.sivaraam,
	liu.denton, congdanhqx, sunshine, Christian Couder

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

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

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

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

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

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

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

Thanks.


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3] submodule: port subcommand 'set-branch' from shell to C
  2020-05-24 23:15   ` Junio C Hamano
@ 2020-05-24 23:18     ` Junio C Hamano
  0 siblings, 0 replies; 17+ messages in thread
From: Junio C Hamano @ 2020-05-24 23:18 UTC (permalink / raw)
  To: Shourya Shukla
  Cc: Johannes Schindelin, git, christian.couder, kaartic.sivaraam,
	liu.denton, congdanhqx, sunshine, Christian Couder

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

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

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

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


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v4] submodule: port subcommand 'set-branch' from shell to C
  2020-05-23 18:49   ` Kaartic Sivaraam
  2020-05-23 23:18     ` Đoàn Trần Công Danh
@ 2020-05-27 17:13     ` Shourya Shukla
  2020-05-28 12:21       ` Đoàn Trần Công Danh
  1 sibling, 1 reply; 17+ messages in thread
From: Shourya Shukla @ 2020-05-27 17:13 UTC (permalink / raw)
  To: Kaartic Sivaraam
  Cc: git, christian.couder, congdanhqx, gitster, liu.denton, sunshine,
	Johannes.Schindelin

On 24/05 12:19, Kaartic Sivaraam wrote:
> As '--quiet' in 'set-branch' is a no-op and is being accepted only for
> uniformity, I think it makes sense to use OPT_NOOP_NOARG instead of
> OPT__QUIET for specifying it, as suggested by Danh.
> 
> Also, the description "suppress output for setting default tracking branch"
> doesn't seem to be valid anymore as we don't print anything when set-branch
> succeeds.

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

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

> Danh questioned whether '$branch' needs to be quoted here. I too think it
> needs to be quoted unless I'm missing something.

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v4] submodule: port subcommand 'set-branch' from shell to C
  2020-05-27 17:13     ` Shourya Shukla
@ 2020-05-28 12:21       ` Đoàn Trần Công Danh
  2020-05-28 14:01         ` Đoàn Trần Công Danh
  0 siblings, 1 reply; 17+ messages in thread
From: Đoàn Trần Công Danh @ 2020-05-28 12:21 UTC (permalink / raw)
  To: Shourya Shukla
  Cc: Kaartic Sivaraam, git, christian.couder, gitster, liu.denton,
	sunshine, Johannes.Schindelin

On 2020-05-27 22:43:58+0530, Shourya Shukla <shouryashukla.oo@gmail.com> wrote:
> On 24/05 12:19, Kaartic Sivaraam wrote:
> > As '--quiet' in 'set-branch' is a no-op and is being accepted only for
> > uniformity, I think it makes sense to use OPT_NOOP_NOARG instead of
> > OPT__QUIET for specifying it, as suggested by Danh.
> > 
> > Also, the description "suppress output for setting default tracking branch"
> > doesn't seem to be valid anymore as we don't print anything when set-branch
> > succeeds.
> 
> I think it will all boil down to the consistency of all the subcommands.
> Changing this would require making changes in various places: the C code
> (obviously), the shell script (not only the cmd_set_branch() function
> but the part for accepting user input as well) and the Documentation (I
> might have maybe missed a couple of other changes to list here too). Its

I don't think this is a valid argument.

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

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

The only necessary change in in that C code.

> not that I don't want to do this, but it would add unnecessary changes
> don't you think? I would love it if others could weigh in their opinions
> too about this.
> 
>  > +	git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper set-branch ${GIT_QUIET:+--quiet} ${branch:+--branch $branch} ${default:+--default} -- "$@"
> 
> > Danh questioned whether '$branch' needs to be quoted here. I too think it
> > needs to be quoted unless I'm missing something.
> 
> We want to do this because $branch is an argument right?

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

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

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

This version will run:

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

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

-- 
Danh

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v4] submodule: port subcommand 'set-branch' from shell to C
  2020-05-28 12:21       ` Đoàn Trần Công Danh
@ 2020-05-28 14:01         ` Đoàn Trần Công Danh
  2020-05-28 15:55           ` Đoàn Trần Công Danh
  0 siblings, 1 reply; 17+ messages in thread
From: Đoàn Trần Công Danh @ 2020-05-28 14:01 UTC (permalink / raw)
  To: Shourya Shukla
  Cc: Kaartic Sivaraam, git, christian.couder, gitster, liu.denton,
	sunshine, Johannes.Schindelin

On 2020-05-28 19:21:47+0700, Đoàn Trần Công Danh <congdanhqx@gmail.com> wrote:
> On 2020-05-27 22:43:58+0530, Shourya Shukla <shouryashukla.oo@gmail.com> wrote:
> >  > +	git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper set-branch ${GIT_QUIET:+--quiet} ${branch:+--branch $branch} ${default:+--default} -- "$@"
> > 
> > > Danh questioned whether '$branch' needs to be quoted here. I too think it
> > > needs to be quoted unless I'm missing something.
> > 
> > We want to do this because $branch is an argument right?
> 
> We want to do this because we don't want to whitespace-split "$branch"
> 
> Let's say, for some reason, this command was run:
> 
> 	git submodule set-branch --branch "a-branch --branch another" a-submodule

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

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

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

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

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

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

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

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

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

After this change, those commands will success:

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

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

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

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

-- 
Danh

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v4] submodule: port subcommand 'set-branch' from shell to C
  2020-05-28 14:01         ` Đoàn Trần Công Danh
@ 2020-05-28 15:55           ` Đoàn Trần Công Danh
  0 siblings, 0 replies; 17+ messages in thread
From: Đoàn Trần Công Danh @ 2020-05-28 15:55 UTC (permalink / raw)
  To: Shourya Shukla
  Cc: Kaartic Sivaraam, git, christian.couder, gitster, liu.denton,
	sunshine, Johannes.Schindelin

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

This should be read:

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

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

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

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

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

	git submodule set-branch

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

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

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

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



-- 
Danh

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, back to index

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-21 16:38 [PATCH v3] submodule: port subcommand 'set-branch' from shell to C Shourya Shukla
2020-05-21 18:44 ` Junio C Hamano
2020-05-21 19:03   ` Denton Liu
2020-05-21 19:50     ` Junio C Hamano
2020-05-22 19:39       ` Shourya Shukla
2020-05-24 16:07         ` Junio C Hamano
2020-05-21 23:04 ` Đoàn Trần Công Danh
2020-05-22 22:21 ` Johannes Schindelin
2020-05-24 23:15   ` Junio C Hamano
2020-05-24 23:18     ` Junio C Hamano
2020-05-23 16:39 ` [PATCH v4] " Shourya Shukla
2020-05-23 18:49   ` Kaartic Sivaraam
2020-05-23 23:18     ` Đoàn Trần Công Danh
2020-05-27 17:13     ` Shourya Shukla
2020-05-28 12:21       ` Đoàn Trần Công Danh
2020-05-28 14:01         ` Đoàn Trần Công Danh
2020-05-28 15:55           ` Đoàn Trần Công Danh

git@vger.kernel.org list mirror (unofficial, one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Example config snippet for mirrors

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git