threads / patch / 53503

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

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

## tl;dr

6 messages between May 19, 2020 and May 20, 2020. Diffs are folded; open one to read it.

replies: 5people: 4as markdown or json

Shourya Shukla· May 19, 2020, 18:26 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>
Helped-by: Denton Liu <liu.denton@gmail.com>
Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>
---
An improvement over the previous version, with a lot less clutter and
redundancy. This version also covers the side-effect pointed out by
Công Danh in (thanks to Kaartic for pointing it out):
https://lore.kernel.org/git/20200517161151.GA30938@danh.dev/

I have refrained from using the `newbranch` variable because using only `opt_branch` simplified things even further (thanks to Christian). I think a similar improvement could be made to `set-url`, but let's leave that for 'leftoverbits' maybe?

Thank you Denton, Christian and Kaartic for the reviews! :) Next step is conversion of `summary` to C (after the review of `set-branch` is done).

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

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

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index f50745a03f..5cd7dc84c6 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2284,6 +2284,46 @@ 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_default = 0;
+	char *opt_branch = NULL;
+	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_STRING(0, "branch", &opt_branch, N_("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 && opt_default)
+		die(_("--branch and --default do not make sense together"));
+
+	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 +2355,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
Eric Sunshine· May 19, 2020, 18:57 UTC · re: Shourya Shukla · lore

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

On Tue, May 19, 2020 at 2:27 PM Shourya Shukla <shouryashukla.oo@gmail.com> wrote:

> Convert submodule subcommand 'set-branch' to a builtin. Port 'set-branch'
> to 'submodule--helper.c' and call the latter via 'git-submodule.sh'.
You can reduce the redundancy by writing this as:
    Convert git-submodule subcommand 'set-branch' to a builtin and
    call it via 'git-submodule.sh'.
Show 8 quoted lines
> Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>
> ---
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> @@ -2284,6 +2284,46 @@ static int module_set_url(int argc, const char **argv, const char *prefix)
> +static int module_set_branch(int argc, const char **argv, const char *prefix)
> +{
> +       int quiet = 0, opt_default = 0;
> +       char *opt_branch = NULL;
This can be 'const const *', can't it?
> +       struct option options[] = {
> +               OPT__QUIET(&quiet,
> +                       N_("suppress output for setting default tracking branch of a submodule")),

This is unusually verbose for a _short_ description of the option. Other commands use simpler descriptions. Perhaps take a hint from the git-submodule man page:

    N("only print error messages")),

However, the bigger question is: Why is the --quiet option even here? None of the code in this function ever consults the 'quiet' variable, so its presence seems pointless.

Looking at the git-submodule documentation, I see that it is already documented as accepted --quiet, so it may make some sense for you to accept the option here. However, it might be a good idea either to have an in-code comment or a blurb in the commit message explaining that this C rewrite accepts the option for backward-compatibility (and for future extension), not because it is actually used presently.

> +               OPT_BOOL(0, "default", &opt_default,
> +                       N_("set the default tracking branch to master")),

We can make this and the next short description more precise and concise like this:

    N_("reset the default tracking branch to master")),
> +               OPT_STRING(0, "branch", &opt_branch, N_("branch"),
> +                       N_("set the default tracking branch to the one specified")),
Then:
    N_("set the default tracking branch")),
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"));

This wording makes no sense considering that --branch and --default are mutually exclusive. By writing "at least one of", you're saying that you can use _more than one_, which is clearly incorrect. Reword it like this:

    die(_("--branch or --default required"));
> +       if (opt_branch && opt_default)
> +               die(_("--branch and --default do not make sense together"));
A more precise way to say this is:
    die(_("--branch and --default are mutually exclusive"));
Show 5 quoted lines
> +       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);

Tracing through the config code, I see that config_set_in_gitmodules_file_gently() removes the key if 'opt_branch' is NULL, which mirrors the behavior of the shell code this is replacing. Good.

Shourya Shukla· May 20, 2020, 12:15 UTC · re: Eric Sunshine · lore

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

Hello Eric!
On 19/05 02:57, Eric Sunshine wrote:
Show 9 quoted lines
> On Tue, May 19, 2020 at 2:27 PM Shourya Shukla
> <shouryashukla.oo@gmail.com> wrote:
> > Convert submodule subcommand 'set-branch' to a builtin. Port 'set-branch'
> > to 'submodule--helper.c' and call the latter via 'git-submodule.sh'.
> 
> You can reduce the redundancy by writing this as:
> 
>     Convert git-submodule subcommand 'set-branch' to a builtin and
>     call it via 'git-submodule.sh'.
Sure! Will do!
Show 13 quoted lines
> > +       struct option options[] = {
> > +               OPT__QUIET(&quiet,
> > +                       N_("suppress output for setting default tracking branch of a submodule")),
> 
> This is unusually verbose for a _short_ description of the option.
> Other commands use simpler descriptions. Perhaps take a hint from the
> git-submodule man page:
> 
>     N("only print error messages")),
> 
> However, the bigger question is: Why is the --quiet option even here?
> None of the code in this function ever consults the 'quiet' variable,
> so its presence seems pointless.

I actually wanted to have *some* use of the `quiet` option and delivered it in the v1, but after some feedback had to scrap it. You may have a look here: https://lore.kernel.org/git/20200513201737.55778-2-shouryashukla.oo@gmail.com/

Show 6 quoted lines
> Looking at the git-submodule documentation, I see that it is already
> documented as accepted --quiet, so it may make some sense for you to
> accept the option here. However, it might be a good idea either to
> have an in-code comment or a blurb in the commit message explaining
> that this C rewrite accepts the option for backward-compatibility (and
> for future extension), not because it is actually used presently.

That seems like a better idea; should I add this comment just above the `options` array? BTW, the shell version has a comment about this, see: https://github.com/git/git/blob/v2.26.2/git-submodule.sh#L727

Show 6 quoted lines
> > +               OPT_STRING(0, "branch", &opt_branch, N_("branch"),
> > +                       N_("set the default tracking branch to the one specified")),
> 
> Then:
> 
>     N_("set the default tracking branch")),
Seems good!
Show 19 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"));
> 
> This wording makes no sense considering that --branch and --default
> are mutually exclusive. By writing "at least one of", you're saying
> that you can use _more than one_, which is clearly incorrect. Reword
> it like this:
> 
>     die(_("--branch or --default required"));

Yeah, I did not realize it until you mentioned this, will correct in the next version.

Show 6 quoted lines
> > +       if (opt_branch && opt_default)
> > +               die(_("--branch and --default do not make sense together"));
> 
> A more precise way to say this is:
> 
>     die(_("--branch and --default are mutually exclusive"));

Will that be clear to everyone? What I mean is maybe a person from a non-mathematical background (someone doing programming as a hobby maybe) will not grasp at this at one go and will have to search it's meaning online. Isn't it fine as-is?

Show 10 quoted lines
> > +       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);
> 
> Tracing through the config code, I see that
> config_set_in_gitmodules_file_gently() removes the key if 'opt_branch'
> is NULL, which mirrors the behavior of the shell code this is
> replacing. Good.
Thanks! :)

Regards, Shourya Shukla

Kaartic Sivaraam· May 20, 2020, 13:12 UTC · re: Shourya Shukla · lore

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

Hi Shourya,
On 20-05-2020 17:45, Shourya Shukla wrote:
Show 15 quoted lines
> On 19/05 02:57, Eric Sunshine wrote:
>> On Tue, May 19, 2020 at 2:27 PM Shourya Shukla
>> <shouryashukla.oo@gmail.com> wrote:
>>> +       if (opt_branch && opt_default)
>>> +               die(_("--branch and --default do not make sense together"));
>>
>> A more precise way to say this is:
>>
>>      die(_("--branch and --default are mutually exclusive"));
> 
> Will that be clear to everyone? What I mean is maybe a person from a
> non-mathematical background (someone doing programming as a hobby maybe)
> will not grasp at this at one go and will have to search it's meaning
> online. Isn't it fine as-is?
> 

While "mutually exclusive" might be prominently used in mathematics. I don't think it is only understandable by people with a mathematical background.

Moreover, I see 183 results in 36 files for "mutually exclusive" in git.git (including translation files). So, this isn't anything new.

I agree with Eric's suggestion. It makes the error message concise which is a nice side benefit.

-- 
Sivaraam
Junio C Hamano· May 20, 2020, 14:37 UTC · re: Kaartic Sivaraam · lore

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

Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes:
Show 28 quoted lines
> Hi Shourya,
>
> On 20-05-2020 17:45, Shourya Shukla wrote:
>> On 19/05 02:57, Eric Sunshine wrote:
>>> On Tue, May 19, 2020 at 2:27 PM Shourya Shukla
>>> <shouryashukla.oo@gmail.com> wrote:
>>>> +       if (opt_branch && opt_default)
>>>> +               die(_("--branch and --default do not make sense together"));
>>>
>>> A more precise way to say this is:
>>>
>>>      die(_("--branch and --default are mutually exclusive"));
>>
>> Will that be clear to everyone? What I mean is maybe a person from a
>> non-mathematical background (someone doing programming as a hobby maybe)
>> will not grasp at this at one go and will have to search it's meaning
>> online. Isn't it fine as-is?
>>
>
> While "mutually exclusive" might be prominently used in mathematics. I
> don't think it is only understandable by people with a mathematical
> background.
>
> Moreover, I see 183 results in 36 files for "mutually exclusive" in
> git.git (including translation files). So, this isn't anything new.
>
> I agree with Eric's suggestion. It makes the error message concise
> which is a nice side benefit.
Sure.  

But "do not make sense together" is forcing the reader to infer the implication "... hence cannot use at the same time", so it is one step detached from what we really want them to know, while giving half an explanation (it still invites "why don't they make sense together?") so why not say that conclusion more directly? i.e. "... cannot be used together"? Either way those who need to be told more would ask "why can't I use them together?" anyway, so half an explanation would not help all that much.

Eric Sunshine· May 20, 2020, 14:45 UTC · re: Shourya Shukla · lore

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

On Wed, May 20, 2020 at 8:23 AM Shourya Shukla <shouryashukla.oo@gmail.com> wrote:

Show 13 quoted lines
> On 19/05 02:57, Eric Sunshine wrote:
> > On Tue, May 19, 2020 at 2:27 PM Shourya Shukla
> > <shouryashukla.oo@gmail.com> wrote:
> > > +        OPT__QUIET(&quiet,
> >
> > However, the bigger question is: Why is the --quiet option even here?
> > None of the code in this function ever consults the 'quiet' variable,
> > so its presence seems pointless.
>
> I actually wanted to have *some* use of the `quiet` option and delivered
> it in the v1, but after some feedback had to scrap it. You may have a
> look here:
> https://lore.kernel.org/git/20200513201737.55778-2-shouryashukla.oo@gmail.com/

I agree with Denton's conclusion about not introducing needless noise merely to give the --quiet option something to squelch. And, to answer your question about when and when not to print something, a good "Unix way" guideline is that "silence is golden"[1].

[1]: http://www.catb.org/esr/writings/taoup/html/ch01s06.html#id2878450
Show 11 quoted lines
> > Looking at the git-submodule documentation, I see that it is already
> > documented as accepted --quiet, so it may make some sense for you to
> > accept the option here. However, it might be a good idea either to
> > have an in-code comment or a blurb in the commit message explaining
> > that this C rewrite accepts the option for backward-compatibility (and
> > for future extension), not because it is actually used presently.
>
> That seems like a better idea; should I add this comment just above the
> `options` array? BTW, the shell version has a comment about this,
> see:
> https://github.com/git/git/blob/v2.26.2/git-submodule.sh#L727

It would be a good idea to attach a comment like that to the declaration of 'quiet' in the C code (rather than placing it above the 'options' array). For instance:

    /* for backward compatibility but not presently used */
    int quiet = 0;
Show 10 quoted lines
> > > +        die(_("--branch and --default do not make sense together"));
> >
> > A more precise way to say this is:
> >
> >   die(_("--branch and --default are mutually exclusive"));
>
> Will that be clear to everyone? What I mean is maybe a person from a
> non-mathematical background (someone doing programming as a hobby maybe)
> will not grasp at this at one go and will have to search it's meaning
> online. Isn't it fine as-is?
Others have already responded to this up-thread...

← back to recent threads