{"thread":{"id":"53503","subject":"[PATCH v2] submodule: port subcommand 'set-branch' from shell to C","startedAt":"2020-05-19T18:27:17Z","lastAt":"2020-05-20T14:46:08Z","messageCount":6,"participants":["Shourya Shukla","Eric Sunshine","Kaartic Sivaraam","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"398206","messageId":"20200519182654.33318-1-shouryashukla.oo@gmail.com","threadId":"53503","inReplyTo":null,"subject":"[PATCH v2] submodule: port subcommand 'set-branch' from shell to C","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-05-19T18:26:54Z","receivedAt":"2020-05-19T18:27:17Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"Convert submodule subcommand 'set-branch' to a builtin. Port 'set-branch'\nto 'submodule--helper.c' and call the latter via 'git-submodule.sh'.\n\nMentored-by: Christian Couder <chriscool@tuxfamily.org>\nMentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>\nHelped-by: Denton Liu <liu.denton@gmail.com>\nSigned-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>\n---\nAn improvement over the previous version, with a lot less clutter and\nredundancy. This version also covers the side-effect pointed out by\nCông Danh in (thanks to Kaartic for pointing it out):\nhttps://lore.kernel.org/git/20200517161151.GA30938@danh.dev/\n\nI have refrained from using the `newbranch` variable because using\nonly `opt_branch` simplified things even further (thanks to Christian).\nI think a similar improvement could be made to `set-url`, but let's leave\nthat for 'leftoverbits' maybe?\n\nThank you Denton, Christian and Kaartic for the reviews! :)\nNext step is conversion of `summary` to C (after the review of `set-branch`\nis done).\n\n builtin/submodule--helper.c | 41 +++++++++++++++++++++++++++++++++++++\n git-submodule.sh            | 32 +++--------------------------\n 2 files changed, 44 insertions(+), 29 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex f50745a03f..5cd7dc84c6 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2284,6 +2284,46 @@ static int module_set_url(int argc, const char **argv, const char *prefix)\n \treturn 0;\n }\n \n+static int module_set_branch(int argc, const char **argv, const char *prefix)\n+{\n+\tint quiet = 0, opt_default = 0;\n+\tchar *opt_branch = NULL;\n+\tconst char *path;\n+\tchar *config_name;\n+\n+\tstruct option options[] = {\n+\t\tOPT__QUIET(&quiet,\n+\t\t\tN_(\"suppress output for setting default tracking branch of a submodule\")),\n+\t\tOPT_BOOL(0, \"default\", &opt_default,\n+\t\t\tN_(\"set the default tracking branch to master\")),\n+\t\tOPT_STRING(0, \"branch\", &opt_branch, N_(\"branch\"),\n+\t\t\tN_(\"set the default tracking branch to the one specified\")),\n+\t\tOPT_END()\n+\t};\n+\tconst char *const usage[] = {\n+\t\tN_(\"git submodule--helper set-branch [--quiet] (-d|--default) <path>\"),\n+\t\tN_(\"git submodule--helper set-branch [--quiet] (-b|--branch) <branch> <path>\"),\n+\t\tNULL\n+\t};\n+\n+\targc = parse_options(argc, argv, prefix, options, usage, 0);\n+\n+\tif (!opt_branch && !opt_default)\n+\t\tdie(_(\"at least one of --branch and --default required\"));\n+\n+\tif (opt_branch && opt_default)\n+\t\tdie(_(\"--branch and --default do not make sense together\"));\n+\n+\tif (argc != 1 || !(path = argv[0]))\n+\t\tusage_with_options(usage, options);\n+\n+\tconfig_name = xstrfmt(\"submodule.%s.branch\", path);\n+\tconfig_set_in_gitmodules_file_gently(config_name, opt_branch);\n+\n+\tfree(config_name);\n+\treturn 0;\n+}\n+\n #define SUPPORT_SUPER_PREFIX (1<<0)\n \n struct cmd_struct {\n@@ -2315,6 +2355,7 @@ static struct cmd_struct commands[] = {\n \t{\"check-name\", check_name, 0},\n \t{\"config\", module_config, 0},\n \t{\"set-url\", module_set_url, 0},\n+\t{\"set-branch\", module_set_branch, 0},\n };\n \n int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 39ebdf25b5..8c56191f77 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -719,7 +719,7 @@ cmd_update()\n # $@ = requested path\n #\n cmd_set_branch() {\n-\tunset_branch=false\n+\tdefault=\n \tbranch=\n \n \twhile test $# -ne 0\n@@ -729,7 +729,7 @@ cmd_set_branch() {\n \t\t\t# we don't do anything with this but we need to accept it\n \t\t\t;;\n \t\t-d|--default)\n-\t\t\tunset_branch=true\n+\t\t\tdefault=1\n \t\t\t;;\n \t\t-b|--branch)\n \t\t\tcase \"$2\" in '') usage ;; esac\n@@ -750,33 +750,7 @@ cmd_set_branch() {\n \t\tshift\n \tdone\n \n-\tif test $# -ne 1\n-\tthen\n-\t\tusage\n-\tfi\n-\n-\t# we can't use `git submodule--helper name` here because internally, it\n-\t# hashes the path so a trailing slash could lead to an unintentional no match\n-\tname=\"$(git submodule--helper list \"$1\" | cut -f2)\"\n-\tif test -z \"$name\"\n-\tthen\n-\t\texit 1\n-\tfi\n-\n-\ttest -n \"$branch\"; has_branch=$?\n-\ttest \"$unset_branch\" = true; has_unset_branch=$?\n-\n-\tif test $((!$has_branch != !$has_unset_branch)) -eq 0\n-\tthen\n-\t\tusage\n-\tfi\n-\n-\tif test $has_branch -eq 0\n-\tthen\n-\t\tgit submodule--helper config submodule.\"$name\".branch \"$branch\"\n-\telse\n-\t\tgit submodule--helper config --unset submodule.\"$name\".branch\n-\tfi\n+\tgit ${wt_prefix:+-C \"$wt_prefix\"} ${prefix:+--super-prefix \"$prefix\"} submodule--helper set-branch ${GIT_QUIET:+--quiet} ${branch:+--branch $branch} ${default:+--default} -- \"$@\"\n }\n \n #\n-- \n2.26.2\n\n"},{"id":"398210","messageId":"CAPig+cSKFBFdNXA52f5f0q3SetA2btmkXeqyHNw-qwJ5ECq5mQ@mail.gmail.com","threadId":"53503","inReplyTo":"20200519182654.33318-1-shouryashukla.oo@gmail.com","subject":"Re: [PATCH v2] submodule: port subcommand 'set-branch' from shell to C","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-05-19T18:57:57Z","receivedAt":"2020-05-19T18:58:14Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, May 19, 2020 at 2:27 PM Shourya Shukla\n<shouryashukla.oo@gmail.com> wrote:\n> Convert submodule subcommand 'set-branch' to a builtin. Port 'set-branch'\n> to 'submodule--helper.c' and call the latter via 'git-submodule.sh'.\n\nYou can reduce the redundancy by writing this as:\n\n    Convert git-submodule subcommand 'set-branch' to a builtin and\n    call it via 'git-submodule.sh'.\n\n> Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>\n> ---\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> @@ -2284,6 +2284,46 @@ static int module_set_url(int argc, const char **argv, const char *prefix)\n> +static int module_set_branch(int argc, const char **argv, const char *prefix)\n> +{\n> +       int quiet = 0, opt_default = 0;\n> +       char *opt_branch = NULL;\n\nThis can be 'const const *', can't it?\n\n> +       struct option options[] = {\n> +               OPT__QUIET(&quiet,\n> +                       N_(\"suppress output for setting default tracking branch of a submodule\")),\n\nThis is unusually verbose for a _short_ description of the option.\nOther commands use simpler descriptions. Perhaps take a hint from the\ngit-submodule man page:\n\n    N(\"only print error messages\")),\n\nHowever, the bigger question is: Why is the --quiet option even here?\nNone of the code in this function ever consults the 'quiet' variable,\nso its presence seems pointless.\n\nLooking at the git-submodule documentation, I see that it is already\ndocumented as accepted --quiet, so it may make some sense for you to\naccept the option here. However, it might be a good idea either to\nhave an in-code comment or a blurb in the commit message explaining\nthat this C rewrite accepts the option for backward-compatibility (and\nfor future extension), not because it is actually used presently.\n\n> +               OPT_BOOL(0, \"default\", &opt_default,\n> +                       N_(\"set the default tracking branch to master\")),\n\nWe can make this and the next short description more precise and\nconcise like this:\n\n    N_(\"reset the default tracking branch to master\")),\n\n> +               OPT_STRING(0, \"branch\", &opt_branch, N_(\"branch\"),\n> +                       N_(\"set the default tracking branch to the one specified\")),\n\nThen:\n\n    N_(\"set the default tracking branch\")),\n\n> +               OPT_END()\n> +       };\n> +       const char *const usage[] = {\n> +               N_(\"git submodule--helper set-branch [--quiet] (-d|--default) <path>\"),\n> +               N_(\"git submodule--helper set-branch [--quiet] (-b|--branch) <branch> <path>\"),\n> +               NULL\n> +       };\n> +\n> +       argc = parse_options(argc, argv, prefix, options, usage, 0);\n> +\n> +       if (!opt_branch && !opt_default)\n> +               die(_(\"at least one of --branch and --default required\"));\n\nThis wording makes no sense considering that --branch and --default\nare mutually exclusive. By writing \"at least one of\", you're saying\nthat you can use _more than one_, which is clearly incorrect. Reword\nit like this:\n\n    die(_(\"--branch or --default required\"));\n\n> +       if (opt_branch && opt_default)\n> +               die(_(\"--branch and --default do not make sense together\"));\n\nA more precise way to say this is:\n\n    die(_(\"--branch and --default are mutually exclusive\"));\n\n> +       if (argc != 1 || !(path = argv[0]))\n> +               usage_with_options(usage, options);\n> +\n> +       config_name = xstrfmt(\"submodule.%s.branch\", path);\n> +       config_set_in_gitmodules_file_gently(config_name, opt_branch);\n\nTracing through the config code, I see that\nconfig_set_in_gitmodules_file_gently() removes the key if 'opt_branch'\nis NULL, which mirrors the behavior of the shell code this is\nreplacing. Good.\n"},{"id":"398245","messageId":"20200520121530.GA7992@konoha","threadId":"53503","inReplyTo":"CAPig+cSKFBFdNXA52f5f0q3SetA2btmkXeqyHNw-qwJ5ECq5mQ@mail.gmail.com","subject":"Re: [PATCH v2] submodule: port subcommand 'set-branch' from shell to C","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-05-20T12:15:30Z","receivedAt":"2020-05-20T12:15:42Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"Hello Eric!\n\nOn 19/05 02:57, Eric Sunshine wrote:\n> On Tue, May 19, 2020 at 2:27 PM Shourya Shukla\n> <shouryashukla.oo@gmail.com> wrote:\n> > Convert submodule subcommand 'set-branch' to a builtin. Port 'set-branch'\n> > to 'submodule--helper.c' and call the latter via 'git-submodule.sh'.\n> \n> You can reduce the redundancy by writing this as:\n> \n>     Convert git-submodule subcommand 'set-branch' to a builtin and\n>     call it via 'git-submodule.sh'.\n\nSure! Will do!\n\n> > +       struct option options[] = {\n> > +               OPT__QUIET(&quiet,\n> > +                       N_(\"suppress output for setting default tracking branch of a submodule\")),\n> \n> This is unusually verbose for a _short_ description of the option.\n> Other commands use simpler descriptions. Perhaps take a hint from the\n> git-submodule man page:\n> \n>     N(\"only print error messages\")),\n> \n> However, the bigger question is: Why is the --quiet option even here?\n> None of the code in this function ever consults the 'quiet' variable,\n> so its presence seems pointless.\n\nI actually wanted to have *some* use of the `quiet` option and delivered\nit in the v1, but after some feedback had to scrap it. You may have a\nlook here:\nhttps://lore.kernel.org/git/20200513201737.55778-2-shouryashukla.oo@gmail.com/\n\n> Looking at the git-submodule documentation, I see that it is already\n> documented as accepted --quiet, so it may make some sense for you to\n> accept the option here. However, it might be a good idea either to\n> have an in-code comment or a blurb in the commit message explaining\n> that this C rewrite accepts the option for backward-compatibility (and\n> for future extension), not because it is actually used presently.\n\nThat seems like a better idea; should I add this comment just above the\n`options` array? BTW, the shell version has a comment about this,\nsee:\nhttps://github.com/git/git/blob/v2.26.2/git-submodule.sh#L727\n\n> > +               OPT_STRING(0, \"branch\", &opt_branch, N_(\"branch\"),\n> > +                       N_(\"set the default tracking branch to the one specified\")),\n> \n> Then:\n> \n>     N_(\"set the default tracking branch\")),\n\nSeems good!\n\n> > +               OPT_END()\n> > +       };\n> > +       const char *const usage[] = {\n> > +               N_(\"git submodule--helper set-branch [--quiet] (-d|--default) <path>\"),\n> > +               N_(\"git submodule--helper set-branch [--quiet] (-b|--branch) <branch> <path>\"),\n> > +               NULL\n> > +       };\n> > +\n> > +       argc = parse_options(argc, argv, prefix, options, usage, 0);\n> > +\n> > +       if (!opt_branch && !opt_default)\n> > +               die(_(\"at least one of --branch and --default required\"));\n> \n> This wording makes no sense considering that --branch and --default\n> are mutually exclusive. By writing \"at least one of\", you're saying\n> that you can use _more than one_, which is clearly incorrect. Reword\n> it like this:\n> \n>     die(_(\"--branch or --default required\"));\n\nYeah, I did not realize it until you mentioned this, will correct in the\nnext version.\n\n> > +       if (opt_branch && opt_default)\n> > +               die(_(\"--branch and --default do not make sense together\"));\n> \n> A more precise way to say this is:\n> \n>     die(_(\"--branch and --default are mutually exclusive\"));\n\nWill that be clear to everyone? What I mean is maybe a person from a\nnon-mathematical background (someone doing programming as a hobby maybe)\nwill not grasp at this at one go and will have to search it's meaning\nonline. Isn't it fine as-is?\n\n> > +       if (argc != 1 || !(path = argv[0]))\n> > +               usage_with_options(usage, options);\n> > +\n> > +       config_name = xstrfmt(\"submodule.%s.branch\", path);\n> > +       config_set_in_gitmodules_file_gently(config_name, opt_branch);\n> \n> Tracing through the config code, I see that\n> config_set_in_gitmodules_file_gently() removes the key if 'opt_branch'\n> is NULL, which mirrors the behavior of the shell code this is\n> replacing. Good.\n\nThanks! :)\n\nRegards,\nShourya Shukla\n"},{"id":"398247","messageId":"b96324d4-04bd-8f64-e334-d3613dc3be6f@gmail.com","threadId":"53503","inReplyTo":"20200520121530.GA7992@konoha","subject":"Re: [PATCH v2] submodule: port subcommand 'set-branch' from shell to C","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2020-05-20T13:12:48Z","receivedAt":"2020-05-20T13:12:57Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"Hi Shourya,\n\nOn 20-05-2020 17:45, Shourya Shukla wrote:\n> On 19/05 02:57, Eric Sunshine wrote:\n>> On Tue, May 19, 2020 at 2:27 PM Shourya Shukla\n>> <shouryashukla.oo@gmail.com> wrote:\n>>> +       if (opt_branch && opt_default)\n>>> +               die(_(\"--branch and --default do not make sense together\"));\n>>\n>> A more precise way to say this is:\n>>\n>>      die(_(\"--branch and --default are mutually exclusive\"));\n> \n> Will that be clear to everyone? What I mean is maybe a person from a\n> non-mathematical background (someone doing programming as a hobby maybe)\n> will not grasp at this at one go and will have to search it's meaning\n> online. Isn't it fine as-is?\n> \n\nWhile \"mutually exclusive\" might be prominently used in mathematics. I \ndon't think it is only understandable by people with a mathematical \nbackground.\n\nMoreover, I see 183 results in 36 files for \"mutually exclusive\" in \ngit.git (including translation files). So, this isn't anything new.\n\nI agree with Eric's suggestion. It makes the error message concise which \nis a nice side benefit.\n\n-- \nSivaraam\n"},{"id":"398253","messageId":"xmqqd06yy8nh.fsf@gitster.c.googlers.com","threadId":"53503","inReplyTo":"b96324d4-04bd-8f64-e334-d3613dc3be6f@gmail.com","subject":"Re: [PATCH v2] submodule: port subcommand 'set-branch' from shell to C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-20T14:37:38Z","receivedAt":"2020-05-20T14:37:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes:\n\n> Hi Shourya,\n>\n> On 20-05-2020 17:45, Shourya Shukla wrote:\n>> On 19/05 02:57, Eric Sunshine wrote:\n>>> On Tue, May 19, 2020 at 2:27 PM Shourya Shukla\n>>> <shouryashukla.oo@gmail.com> wrote:\n>>>> +       if (opt_branch && opt_default)\n>>>> +               die(_(\"--branch and --default do not make sense together\"));\n>>>\n>>> A more precise way to say this is:\n>>>\n>>>      die(_(\"--branch and --default are mutually exclusive\"));\n>>\n>> Will that be clear to everyone? What I mean is maybe a person from a\n>> non-mathematical background (someone doing programming as a hobby maybe)\n>> will not grasp at this at one go and will have to search it's meaning\n>> online. Isn't it fine as-is?\n>>\n>\n> While \"mutually exclusive\" might be prominently used in mathematics. I\n> don't think it is only understandable by people with a mathematical\n> background.\n>\n> Moreover, I see 183 results in 36 files for \"mutually exclusive\" in\n> git.git (including translation files). So, this isn't anything new.\n>\n> I agree with Eric's suggestion. It makes the error message concise\n> which is a nice side benefit.\n\nSure.  \n\nBut \"do not make sense together\" is forcing the reader to infer the\nimplication \"... hence cannot use at the same time\", so it is one\nstep detached from what we really want them to know, while giving\nhalf an explanation (it still invites \"why don't they make sense\ntogether?\") so why not say that conclusion more directly?  i.e.\n\"... cannot be used together\"?  Either way those who need to be told\nmore would ask \"why can't I use them together?\" anyway, so half an\nexplanation would not help all that much.\n\n\n"},{"id":"398257","messageId":"CAPig+cSr0_eOOi6xiOLY0bzJ33ewpib+MiMjmtay_bi4J9D_=w@mail.gmail.com","threadId":"53503","inReplyTo":"20200520121530.GA7992@konoha","subject":"Re: [PATCH v2] submodule: port subcommand 'set-branch' from shell to C","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-05-20T14:45:53Z","receivedAt":"2020-05-20T14:46:08Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, May 20, 2020 at 8:23 AM Shourya Shukla\n<shouryashukla.oo@gmail.com> wrote:\n> On 19/05 02:57, Eric Sunshine wrote:\n> > On Tue, May 19, 2020 at 2:27 PM Shourya Shukla\n> > <shouryashukla.oo@gmail.com> wrote:\n> > > +        OPT__QUIET(&quiet,\n> >\n> > However, the bigger question is: Why is the --quiet option even here?\n> > None of the code in this function ever consults the 'quiet' variable,\n> > so its presence seems pointless.\n>\n> I actually wanted to have *some* use of the `quiet` option and delivered\n> it in the v1, but after some feedback had to scrap it. You may have a\n> look here:\n> https://lore.kernel.org/git/20200513201737.55778-2-shouryashukla.oo@gmail.com/\n\nI agree with Denton's conclusion about not introducing needless noise\nmerely to give the --quiet option something to squelch. And, to answer\nyour question about when and when not to print something, a good \"Unix\nway\" guideline is that \"silence is golden\"[1].\n\n[1]: http://www.catb.org/esr/writings/taoup/html/ch01s06.html#id2878450\n\n> > Looking at the git-submodule documentation, I see that it is already\n> > documented as accepted --quiet, so it may make some sense for you to\n> > accept the option here. However, it might be a good idea either to\n> > have an in-code comment or a blurb in the commit message explaining\n> > that this C rewrite accepts the option for backward-compatibility (and\n> > for future extension), not because it is actually used presently.\n>\n> That seems like a better idea; should I add this comment just above the\n> `options` array? BTW, the shell version has a comment about this,\n> see:\n> https://github.com/git/git/blob/v2.26.2/git-submodule.sh#L727\n\nIt would be a good idea to attach a comment like that to the\ndeclaration of 'quiet' in the C code (rather than placing it above the\n'options' array). For instance:\n\n    /* for backward compatibility but not presently used */\n    int quiet = 0;\n\n> > > +        die(_(\"--branch and --default do not make sense together\"));\n> >\n> > A more precise way to say this is:\n> >\n> >   die(_(\"--branch and --default are mutually exclusive\"));\n>\n> Will that be clear to everyone? What I mean is maybe a person from a\n> non-mathematical background (someone doing programming as a hobby maybe)\n> will not grasp at this at one go and will have to search it's meaning\n> online. Isn't it fine as-is?\n\nOthers have already responded to this up-thread...\n"}]}