* [PATCH v2] submodule: port subcommand 'set-branch' from shell to C
@ 2020-05-19 18:26 Shourya Shukla
2020-05-19 18:57 ` Eric Sunshine
0 siblings, 1 reply; 6+ messages in thread
From: Shourya Shukla @ 2020-05-19 18:26 UTC (permalink / raw)
To: git
Cc: christian.couder, kaartic.sivaraam, liu.denton, gitster,
congdanhqx, Shourya Shukla, Christian Couder
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(-)
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] submodule: port subcommand 'set-branch' from shell to C
2020-05-19 18:26 [PATCH v2] submodule: port subcommand 'set-branch' from shell to C Shourya Shukla
@ 2020-05-19 18:57 ` Eric Sunshine
2020-05-20 12:15 ` Shourya Shukla
0 siblings, 1 reply; 6+ messages in thread
From: Eric Sunshine @ 2020-05-19 18:57 UTC (permalink / raw)
To: Shourya Shukla
Cc: Git List, Christian Couder, Kaartic Sivaraam, Denton Liu,
Junio C Hamano, Doan Tran Cong Danh, Christian Couder
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'.
> 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")),
> + 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"));
> + 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] submodule: port subcommand 'set-branch' from shell to C
2020-05-19 18:57 ` Eric Sunshine
@ 2020-05-20 12:15 ` Shourya Shukla
2020-05-20 13:12 ` Kaartic Sivaraam
2020-05-20 14:45 ` Eric Sunshine
0 siblings, 2 replies; 6+ messages in thread
From: Shourya Shukla @ 2020-05-20 12:15 UTC (permalink / raw)
To: Eric Sunshine
Cc: christian.couder, kaartic.sivaraam, gitster, liu.denton, git,
congdanhqx
Hello Eric!
On 19/05 02:57, Eric Sunshine wrote:
> 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!
> > + 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/
> 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
> > + 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!
> > + 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.
> > + 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?
> > + 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] submodule: port subcommand 'set-branch' from shell to C
2020-05-20 12:15 ` Shourya Shukla
@ 2020-05-20 13:12 ` Kaartic Sivaraam
2020-05-20 14:37 ` Junio C Hamano
2020-05-20 14:45 ` Eric Sunshine
1 sibling, 1 reply; 6+ messages in thread
From: Kaartic Sivaraam @ 2020-05-20 13:12 UTC (permalink / raw)
To: Shourya Shukla
Cc: Eric Sunshine, christian.couder, gitster, liu.denton, git,
congdanhqx
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.
--
Sivaraam
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] submodule: port subcommand 'set-branch' from shell to C
2020-05-20 13:12 ` Kaartic Sivaraam
@ 2020-05-20 14:37 ` Junio C Hamano
0 siblings, 0 replies; 6+ messages in thread
From: Junio C Hamano @ 2020-05-20 14:37 UTC (permalink / raw)
To: Kaartic Sivaraam
Cc: Shourya Shukla, Eric Sunshine, christian.couder, liu.denton, git,
congdanhqx
Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes:
> 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] submodule: port subcommand 'set-branch' from shell to C
2020-05-20 12:15 ` Shourya Shukla
2020-05-20 13:12 ` Kaartic Sivaraam
@ 2020-05-20 14:45 ` Eric Sunshine
1 sibling, 0 replies; 6+ messages in thread
From: Eric Sunshine @ 2020-05-20 14:45 UTC (permalink / raw)
To: Shourya Shukla
Cc: Christian Couder, Kaartic Sivaraam, Junio C Hamano, Denton Liu,
Git List, Doan Tran Cong Danh
On Wed, May 20, 2020 at 8:23 AM Shourya Shukla
<shouryashukla.oo@gmail.com> 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:
> > > + 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
> > 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;
> > > + 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...
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-05-20 14:46 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-19 18:26 [PATCH v2] submodule: port subcommand 'set-branch' from shell to C Shourya Shukla
2020-05-19 18:57 ` Eric Sunshine
2020-05-20 12:15 ` Shourya Shukla
2020-05-20 13:12 ` Kaartic Sivaraam
2020-05-20 14:37 ` Junio C Hamano
2020-05-20 14:45 ` Eric Sunshine
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).