{"thread":{"id":"53464","subject":"[PATCH 1/2] submodule: port subcommand 'set-branch' from shell to C","startedAt":"2020-05-13T20:18:00Z","lastAt":"2020-05-17T16:11:56Z","messageCount":14,"participants":["Shourya Shukla","Kaartic Sivaraam","Denton Liu","Junio C Hamano","Đoàn Trần Công Danh"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"397761","messageId":"20200513201737.55778-1-shouryashukla.oo@gmail.com","threadId":"53464","inReplyTo":null,"subject":"[PATCH 1/2] submodule: port subcommand 'set-branch' from shell to C","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-05-13T20:17:36Z","receivedAt":"2020-05-13T20:18:00Z","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>\nSigned-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>\n---\nHere is another conversion, this time it is 'set-branch'. It passes all\nthe tests in t7419. I am aware that there are some repetitve parts in\nthe conversion as well as variables which can be named better. I would\nlove everyone's suggestion on this and how this can be made better.\n\nThe extra '$branch' on line 752 was because of Christian's help after\nreference from TLDP's Parameter Subsitution documentation:\nhttps://www.tldp.org/LDP/abs/html/parameter-substitution.html\n\nSimilarly, I had to change a coouple of other lines in the shell version\nso as to make it compatible with the C version.\n\nThank you so much Christian and Kaartic for the mentoring, this wouldn't\nhave been possible otherwise :)\n\n builtin/submodule--helper.c | 58 +++++++++++++++++++++++++++++++++++++\n git-submodule.sh            | 31 ++------------------\n 2 files changed, 60 insertions(+), 29 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex f50745a03f..5a8815b76e 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2284,6 +2284,63 @@ 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_branch = 0, opt_default = 0;\n+\tconst char *newbranch;\n+\tconst char *path;\n+\tchar *config_name;\n+\n+\tstruct option options[] = {\n+\t\tOPT__QUIET(&quiet, N_(\"Suppress output for setting default tracking branch of a submodule\")),\n+\t\tOPT_BOOL(0, \"default\", &opt_default, N_(\"Set the default tracking branch to master\")),\n+\t\tOPT_BOOL(0, \"branch\", &opt_branch, N_(\"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) {\n+\t\tif (opt_default)\n+\t\t\tdie(_(\"--branch and --default do not make sense\"));\n+\n+\t\tnewbranch = argv[0];\n+\t\tpath = argv[1];\n+\n+\t\tif (argc != 2 || !(newbranch = argv[0]) || !(path = argv[1]))\n+\t\t\tusage_with_options(usage, options);\n+\n+\t\tconfig_name = xstrfmt(\"submodule.%s.branch\", path);\n+\t\tconfig_set_in_gitmodules_file_gently(config_name, newbranch);\n+\n+\t\tprintf(_(\"Default tracking branch set to '%s' successfully\\n\"), newbranch);\n+\t\tfree(config_name);\n+\t}\n+\n+\tif (opt_default) {\n+\t\tpath = argv[0];\n+\n+\t\tif (argc != 1 || !(path = argv[0]))\n+\t\t\tusage_with_options(usage, options);\n+\n+\t\tconfig_name = xstrfmt(\"submodule.%s.branch\", path);\n+\t\tconfig_set_in_gitmodules_file_gently(config_name, NULL);\n+\n+\t\tprintf(_(\"Default tracking branch set to 'master' successfully\\n\"));\n+\t\tfree(config_name);\n+\t}\n+\n+\treturn 0;\n+}\n+\n #define SUPPORT_SUPER_PREFIX (1<<0)\n \n struct cmd_struct {\n@@ -2315,6 +2372,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..2438ef576e 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -719,7 +719,6 @@ cmd_update()\n # $@ = requested path\n #\n cmd_set_branch() {\n-\tunset_branch=false\n \tbranch=\n \n \twhile test $# -ne 0\n@@ -729,7 +728,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 +749,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":"397762","messageId":"20200513201737.55778-2-shouryashukla.oo@gmail.com","threadId":"53464","inReplyTo":"20200513201737.55778-1-shouryashukla.oo@gmail.com","subject":"[PATCH 2/2] submodule: Add 'quiet' option in subcommand 'set-branch'","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-05-13T20:17:37Z","receivedAt":"2020-05-13T20:18:04Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"The subcommand 'set-branch' had the 'quiet' option which was\nintroduced in b57e8119e6 by Denton Liu but was never utilised due to\nnot setting of the 'GIT_QUIET' variable. Add functionality to\nutilise the 'quiet' function.\n\nMentored-by: Christian Couder <chriscool@tuxfamily.org>\nMentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>\nSigned-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>\n---\nThere was an existence of the `quiet` option in the shell version of\n'set-branch' but it was not used anywhere. I decided to add a utility\nof the option here by setting GIT_QUIET to 1 in case of a `quiet` as\nwell as ensure proper functioning in the C version regarding the same.\nThe if-statement is inspired from what Junio suggested me in my previous\nconversion of 'set-url'.\n\n builtin/submodule--helper.c | 6 ++++--\n git-submodule.sh            | 2 +-\n 2 files changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 5a8815b76e..36b69df5c4 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2321,7 +2321,8 @@ static int module_set_branch(int argc, const char **argv, const char *prefix)\n \t\tconfig_name = xstrfmt(\"submodule.%s.branch\", path);\n \t\tconfig_set_in_gitmodules_file_gently(config_name, newbranch);\n \n-\t\tprintf(_(\"Default tracking branch set to '%s' successfully\\n\"), newbranch);\n+\t\tif (!(quiet ? OPT_QUIET : 0))\n+\t\t\tprintf(_(\"Default tracking branch set to '%s' successfully\\n\"), newbranch);\n \t\tfree(config_name);\n \t}\n \n@@ -2334,7 +2335,8 @@ static int module_set_branch(int argc, const char **argv, const char *prefix)\n \t\tconfig_name = xstrfmt(\"submodule.%s.branch\", path);\n \t\tconfig_set_in_gitmodules_file_gently(config_name, NULL);\n \n-\t\tprintf(_(\"Default tracking branch set to 'master' successfully\\n\"));\n+\t\tif (!(quiet ? OPT_QUIET : 0))\n+\t\t\tprintf(_(\"Default tracking branch set to 'master' successfully\\n\"));\n \t\tfree(config_name);\n \t}\n \ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 2438ef576e..0cdc77ace6 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -725,7 +725,7 @@ cmd_set_branch() {\n \tdo\n \t\tcase \"$1\" in\n \t\t-q|--quiet)\n-\t\t\t# we don't do anything with this but we need to accept it\n+\t\t\tGIT_QUIET=1\n \t\t\t;;\n \t\t-d|--default)\n \t\t\tdefault=1\n-- \n2.26.2\n\n"},{"id":"397802","messageId":"4e36c8a0-4f43-79f8-137f-5fd080066af6@gmail.com","threadId":"53464","inReplyTo":"20200513201737.55778-1-shouryashukla.oo@gmail.com","subject":"Re: [PATCH 1/2] submodule: port subcommand 'set-branch' from shell to C","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2020-05-14T09:07:37Z","receivedAt":"2020-05-14T09:07:48Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"-Cc: chriscool@tuxfamily.org (seems redundant; Also, .mailmap confirms \nthat christian.couder@gmail.com is Christian's preference)\n\nOn 14-05-2020 01:47, Shourya Shukla wrote:\n> \n> The extra '$branch' on line 752 was because of Christian's help after\n> reference from TLDP's Parameter Subsitution documentation:\n> https://www.tldp.org/LDP/abs/html/parameter-substitution.html\n>\n\nFor those who lack the context, during the conversion of the script \nShourya faced an issue where the '--branch' argument did not work as \nexpected. He described the issue in a private e-mail where Christian \npointed out the following (quoting his reply hoping he doesn't mind):\n\n > On Tue, May 12, 2020 at 5:55 PM Christian Couder\n<christian.couder@gmail.com> wrote:\n >>\n >> In your commit (in submodule.sh line 781) there is:\n >>\n >>         `git ${wt_prefix:+-C \"$wt_prefix\"} ${prefix:+--super-prefix\n >> \"$prefix\"} submodule--helper set-branch ${GIT_QUIET:+--quiet}\n >> ${branch:+--branch} ${default:+--default} -- \"$@\"`\n >\n > Actually the issue might come from the above. I think it should use \n${branch:+--branch $branch} instead of ${branch:+--branch}.\n >\n > See: https://www.tldp.org/LDP/abs/html/parameter-substitution.html\n\nThat's why Shourya mentions the '$branch' as extra. Of course, that's \nhow it is supposed to be in the first place :)\n\n-- \nSivaraam\n"},{"id":"397804","messageId":"20200514101013.GA28018@generichostname","threadId":"53464","inReplyTo":"20200513201737.55778-1-shouryashukla.oo@gmail.com","subject":"Re: [PATCH 1/2] submodule: port subcommand 'set-branch' from shell to C","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-05-14T10:10:13Z","receivedAt":"2020-05-14T10:10:20Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Shourya,\n\nOn Thu, May 14, 2020 at 01:47:36AM +0530, Shourya Shukla 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> Mentored-by: Christian Couder <chriscool@tuxfamily.org>\n> Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>\n> Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>\n> ---\n> Here is another conversion, this time it is 'set-branch'. It passes all\n> the tests in t7419. I am aware that there are some repetitve parts in\n> the conversion as well as variables which can be named better. I would\n> love everyone's suggestion on this and how this can be made better.\n> \n> The extra '$branch' on line 752 was because of Christian's help after\n> reference from TLDP's Parameter Subsitution documentation:\n> https://www.tldp.org/LDP/abs/html/parameter-substitution.html\n> \n> Similarly, I had to change a coouple of other lines in the shell version\n> so as to make it compatible with the C version.\n> \n> Thank you so much Christian and Kaartic for the mentoring, this wouldn't\n> have been possible otherwise :)\n> \n>  builtin/submodule--helper.c | 58 +++++++++++++++++++++++++++++++++++++\n>  git-submodule.sh            | 31 ++------------------\n>  2 files changed, 60 insertions(+), 29 deletions(-)\n> \n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index f50745a03f..5a8815b76e 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -2284,6 +2284,63 @@ 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_branch = 0, opt_default = 0;\n> +\tconst char *newbranch;\n\nnit: I would call this new_branch\n\n> +\tconst char *path;\n> +\tchar *config_name;\n> +\n> +\tstruct option options[] = {\n> +\t\tOPT__QUIET(&quiet, N_(\"Suppress output for setting default tracking branch of a submodule\")),\n> +\t\tOPT_BOOL(0, \"default\", &opt_default, N_(\"Set the default tracking branch to master\")),\n> +\t\tOPT_BOOL(0, \"branch\", &opt_branch, N_(\"Set the default tracking branch to the one specified\")),\n\nThis should use OPT_STRING and accept a string argument instead of using\nthe implicit command-line ordering.\n\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\nError messages in Git are generally written without capitalising the\nfirst letter of the sentence.\n\n> +\n> +\tif (opt_branch) {\n> +\t\tif (opt_default)\n> +\t\t\tdie(_(\"--branch and --default do not make sense\"));\n\nThis error message should be qualified, perhaps with something like \"do\nnot make sense together\".\n\n> +\n> +\t\tnewbranch = argv[0];\n> +\t\tpath = argv[1];\n\nThese assignments are incorrect since we haven't check argc yet. Also,\nthey're redundant since you have the assignments in the if statement\nbelow.\n\nAlso, if you do the OPT_STRING thing as described above, you can do the\n`path = ...` outside of the if-statement since it'll be common to both\n-d and -b.\n\n> +\n> +\t\tif (argc != 2 || !(newbranch = argv[0]) || !(path = argv[1]))\n> +\t\t\tusage_with_options(usage, options);\n> +\n> +\t\tconfig_name = xstrfmt(\"submodule.%s.branch\", path);\n> +\t\tconfig_set_in_gitmodules_file_gently(config_name, newbranch);\n> +\n> +\t\tprintf(_(\"Default tracking branch set to '%s' successfully\\n\"), newbranch);\n\nThe original function did not print anything. We shouldn't alter the\nbehaviour if we're just porting it over so we should delete this.\n\n> +\t\tfree(config_name);\n> +\t}\n> +\n> +\tif (opt_default) {\n> +\t\tpath = argv[0];\n> +\n> +\t\tif (argc != 1 || !(path = argv[0]))\n> +\t\t\tusage_with_options(usage, options);\n> +\n> +\t\tconfig_name = xstrfmt(\"submodule.%s.branch\", path);\n> +\t\tconfig_set_in_gitmodules_file_gently(config_name, NULL);\n> +\n> +\t\tprintf(_(\"Default tracking branch set to 'master' successfully\\n\"));\n> +\t\tfree(config_name);\n> +\t}\n\nThe same arguments for the above apply to this case too. Actually, the\nonly place where they both really differ is in the call to\nconfig_set_in_gitmodules_file_gently(). Can you do all of your argument\nchecks together? Something like\n\n\tif (!!new_branch == opt_default)\n\t\tusage_with_options(usage, options);\n\nThen the call to config_set_in_gitmodules_file_gently() could look like\nthis:\n\n\tconfig_name = xstrfmt(\"submodule.%s.branch\", path);\n\tconfig_set_in_gitmodules_file_gently(config_name, new_branch ? new_branch : NULL);\n\tfree(config_name);\n\nand we wouldn't need the ifs at all.\n\n> +\n> +\treturn 0;\n> +}\n> +\n>  #define SUPPORT_SUPER_PREFIX (1<<0)\n>  \n>  struct cmd_struct {\n> @@ -2315,6 +2372,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)\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index 39ebdf25b5..2438ef576e 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -719,7 +719,6 @@ cmd_update()\n>  # $@ = requested path\n>  #\n>  cmd_set_branch() {\n> -\tunset_branch=false\n>  \tbranch=\n>  \n>  \twhile test $# -ne 0\n> @@ -729,7 +728,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 +749,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\nThe shell script portion looks good.\n\nThanks,\n\nDenton\n\n>  }\n>  \n>  #\n> -- \n> 2.26.2\n> \n"},{"id":"397805","messageId":"20200514101534.GB28018@generichostname","threadId":"53464","inReplyTo":"20200513201737.55778-2-shouryashukla.oo@gmail.com","subject":"Re: [PATCH 2/2] submodule: Add 'quiet' option in subcommand 'set-branch'","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-05-14T10:15:34Z","receivedAt":"2020-05-14T10:15:41Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Shourya,\n\nI'm not really sure if we should have this patch at all since I don't\nthink that set-branch should be printing anything at all.\n\nBut I'll give some comments anyway. Hopefully they'll be enlightening.\n\nOn Thu, May 14, 2020 at 01:47:37AM +0530, Shourya Shukla wrote:\n> The subcommand 'set-branch' had the 'quiet' option which was\n> introduced in b57e8119e6 by Denton Liu but was never utilised due to\n\nWe typically refer to commits by the \"reference\" format. You can get\nthat as follows:\n\n\t$ git show --pretty=ref -s b57e8119e6\n\tb57e8119e6 (submodule: teach set-branch subcommand, 2019-02-08)\n\nIn addition, I don't think it's necessary to mention me by name in this\ncase.\n\n> not setting of the 'GIT_QUIET' variable. Add functionality to\n> utilise the 'quiet' function.\n> \n> Mentored-by: Christian Couder <chriscool@tuxfamily.org>\n> Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>\n> Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>\n> ---\n> There was an existence of the `quiet` option in the shell version of\n> 'set-branch' but it was not used anywhere. I decided to add a utility\n> of the option here by setting GIT_QUIET to 1 in case of a `quiet` as\n> well as ensure proper functioning in the C version regarding the same.\n> The if-statement is inspired from what Junio suggested me in my previous\n> conversion of 'set-url'.\n> \n>  builtin/submodule--helper.c | 6 ++++--\n>  git-submodule.sh            | 2 +-\n>  2 files changed, 5 insertions(+), 3 deletions(-)\n> \n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index 5a8815b76e..36b69df5c4 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -2321,7 +2321,8 @@ static int module_set_branch(int argc, const char **argv, const char *prefix)\n>  \t\tconfig_name = xstrfmt(\"submodule.%s.branch\", path);\n>  \t\tconfig_set_in_gitmodules_file_gently(config_name, newbranch);\n>  \n> -\t\tprintf(_(\"Default tracking branch set to '%s' successfully\\n\"), newbranch);\n> +\t\tif (!(quiet ? OPT_QUIET : 0))\n\nThis is needlessly complicated... Can't this just be written as\n\n\tif (!quiet)\n\n> +\t\t\tprintf(_(\"Default tracking branch set to '%s' successfully\\n\"), newbranch);\n>  \t\tfree(config_name);\n>  \t}\n>  \n> @@ -2334,7 +2335,8 @@ static int module_set_branch(int argc, const char **argv, const char *prefix)\n>  \t\tconfig_name = xstrfmt(\"submodule.%s.branch\", path);\n>  \t\tconfig_set_in_gitmodules_file_gently(config_name, NULL);\n>  \n> -\t\tprintf(_(\"Default tracking branch set to 'master' successfully\\n\"));\n> +\t\tif (!(quiet ? OPT_QUIET : 0))\n> +\t\t\tprintf(_(\"Default tracking branch set to 'master' successfully\\n\"));\n>  \t\tfree(config_name);\n>  \t}\n>  \n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index 2438ef576e..0cdc77ace6 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -725,7 +725,7 @@ cmd_set_branch() {\n>  \tdo\n>  \t\tcase \"$1\" in\n>  \t\t-q|--quiet)\n> -\t\t\t# we don't do anything with this but we need to accept it\n> +\t\t\tGIT_QUIET=1\n>  \t\t\t;;\n>  \t\t-d|--default)\n>  \t\t\tdefault=1\n> -- \n> 2.26.2\n> \n"},{"id":"397955","messageId":"20200516055016.GB3296@konoha","threadId":"53464","inReplyTo":"20200514101534.GB28018@generichostname","subject":"Re: [PATCH 2/2] submodule: Add 'quiet' option in subcommand 'set-branch'","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-05-16T05:50:16Z","receivedAt":"2020-05-16T05:50:28Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"On 14/05 06:15, Denton Liu wrote:\n> Hi Shourya,\n> \n> I'm not really sure if we should have this patch at all since I don't\n> think that set-branch should be printing anything at all.\n\nI thought that the Documentation has the mention of the `quiet` and it\nwouldn't harm printing something when the branch is set. Is this not the\nright way to tackle this?\n\n> But I'll give some comments anyway. Hopefully they'll be enlightening.\n> \n> On Thu, May 14, 2020 at 01:47:37AM +0530, Shourya Shukla wrote:\n> > The subcommand 'set-branch' had the 'quiet' option which was\n> > introduced in b57e8119e6 by Denton Liu but was never utilised due to\n> \n> We typically refer to commits by the \"reference\" format. You can get\n> that as follows:\n> \n> \t$ git show --pretty=ref -s b57e8119e6\n> \tb57e8119e6 (submodule: teach set-branch subcommand, 2019-02-08)\n> \n> In addition, I don't think it's necessary to mention me by name in this\n> case.\n\nOkay, I did not know that, will change in the next version.\n\n> > -\t\tprintf(_(\"Default tracking branch set to '%s' successfully\\n\"), newbranch);\n> > +\t\tif (!(quiet ? OPT_QUIET : 0))\n> \n> This is needlessly complicated... Can't this just be written as\n> \n> \tif (!quiet)\n\nOkay, will do.\n"},{"id":"397957","messageId":"20200516085642.GA16726@generichostname","threadId":"53464","inReplyTo":"20200516055016.GB3296@konoha","subject":"Re: [PATCH 2/2] submodule: Add 'quiet' option in subcommand 'set-branch'","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-05-16T08:56:42Z","receivedAt":"2020-05-16T08:56:48Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"On Sat, May 16, 2020 at 11:20:16AM +0530, Shourya Shukla wrote:\n> On 14/05 06:15, Denton Liu wrote:\n> > Hi Shourya,\n> > \n> > I'm not really sure if we should have this patch at all since I don't\n> > think that set-branch should be printing anything at all.\n> \n> I thought that the Documentation has the mention of the `quiet` and it\n> wouldn't harm printing something when the branch is set. Is this not the\n> right way to tackle this?\n\nI would argue that it's unnecessary to print anything. If a user\nprovides an invalid option, then an error message will be printed. If no\nerror is provided, then a user would assume that the command succeeded.\nTake, for example, `git submodule set-url` which behaves similarly. It's\nsilent on success. Printing on success would just be noise.\n"},{"id":"397958","messageId":"20200516103744.GA25211@konoha","threadId":"53464","inReplyTo":"20200514101013.GA28018@generichostname","subject":"Re: [PATCH 1/2] submodule: port subcommand 'set-branch' from shell to C","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-05-16T10:37:44Z","receivedAt":"2020-05-16T10:37:55Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"On 14/05 06:10, Denton Liu wrote:\n> > +\tconst char *path;\n> > +\tchar *config_name;\n> > +\n> > +\tstruct option options[] = {\n> > +\t\tOPT__QUIET(&quiet, N_(\"Suppress output for setting default tracking branch of a submodule\")),\n> > +\t\tOPT_BOOL(0, \"default\", &opt_default, N_(\"Set the default tracking branch to master\")),\n> > +\t\tOPT_BOOL(0, \"branch\", &opt_branch, N_(\"Set the default tracking branch to the one specified\")),\n> \n> This should use OPT_STRING and accept a string argument instead of using\n> the implicit command-line ordering.\n\nI actually was not able to understand the point of this change until I\ntried it out myself. It has made the code more aethetic as well as less\nredundant. Thanks a ton!\n\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> Error messages in Git are generally written without capitalising the\n> first letter of the sentence.\n\nCorrected. BTW, many other subcommands have this problem (the error\nmessages as well as the options start with a caps and end with a\nfullstop). Should they be corrected or let them be as is?\n\n> > +\n> > +\tif (opt_branch) {\n> > +\t\tif (opt_default)\n> > +\t\t\tdie(_(\"--branch and --default do not make sense\"));\n> \n> This error message should be qualified, perhaps with something like \"do\n> not make sense together\".\n\nDone!\n\n> The same arguments for the above apply to this case too. Actually, the\n> only place where they both really differ is in the call to\n> config_set_in_gitmodules_file_gently(). Can you do all of your argument\n> checks together? Something like\n> \n> \tif (!!new_branch == opt_default)\n> \t\tusage_with_options(usage, options);\n> \n> Then the call to config_set_in_gitmodules_file_gently() could look like\n> this:\n> \n> \tconfig_name = xstrfmt(\"submodule.%s.branch\", path);\n> \tconfig_set_in_gitmodules_file_gently(config_name, new_branch ? new_branch : NULL);\n> \tfree(config_name);\n> \n> and we wouldn't need the ifs at all.\n> \n\nSure, I have made the changes.\n\nRegards,\nShourya Shukla\n"},{"id":"397959","messageId":"20200516104015.GB25211@konoha","threadId":"53464","inReplyTo":"20200516085642.GA16726@generichostname","subject":"Re: [PATCH 2/2] submodule: Add 'quiet' option in subcommand 'set-branch'","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-05-16T10:40:15Z","receivedAt":"2020-05-16T10:40:25Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"On 16/05 04:56, Denton Liu wrote:\n> I would argue that it's unnecessary to print anything. If a user\n> provides an invalid option, then an error message will be printed. If no\n> error is provided, then a user would assume that the command succeeded.\n> Take, for example, `git submodule set-url` which behaves similarly. It's\n> silent on success. Printing on success would just be noise.\n\nOkay, I have dropped the commit. When exactly is printing on success not\ndeemed as noise?\n"},{"id":"397963","messageId":"20200516105528.GA22601@generichostname","threadId":"53464","inReplyTo":"20200516103744.GA25211@konoha","subject":"Re: [PATCH 1/2] submodule: port subcommand 'set-branch' from shell to C","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-05-16T10:55:28Z","receivedAt":"2020-05-16T10:55:35Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"On Sat, May 16, 2020 at 04:07:44PM +0530, Shourya Shukla wrote:\n> On 14/05 06:10, Denton Liu wrote:\n> > Error messages in Git are generally written without capitalising the\n> > first letter of the sentence.\n> \n> Corrected. BTW, many other subcommands have this problem (the error\n> messages as well as the options start with a caps and end with a\n> fullstop). Should they be corrected or let them be as is?\n\nIf it's already written, it's probably fine to leave it as is. Although\nif you're working in an area with some messages that could be fixed, it\nmight be good to clean them up as a preparatory patch.\n"},{"id":"397969","messageId":"20200516110604.GB22601@generichostname","threadId":"53464","inReplyTo":"20200516104015.GB25211@konoha","subject":"Re: [PATCH 2/2] submodule: Add 'quiet' option in subcommand 'set-branch'","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-05-16T11:06:04Z","receivedAt":"2020-05-16T11:06:10Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"On Sat, May 16, 2020 at 04:10:15PM +0530, Shourya Shukla wrote:\n> On 16/05 04:56, Denton Liu wrote:\n> > I would argue that it's unnecessary to print anything. If a user\n> > provides an invalid option, then an error message will be printed. If no\n> > error is provided, then a user would assume that the command succeeded.\n> > Take, for example, `git submodule set-url` which behaves similarly. It's\n> > silent on success. Printing on success would just be noise.\n> \n> Okay, I have dropped the commit. When exactly is printing on success not\n> deemed as noise?\n\nThere's not really a hard rule on this. As a rule of thumb, though, if\nit's between a success and a failure, the failure should print and the\nsuccess should be silent. If the user is requesting some information,\nthen it should always print the information. There are probably some\ncommands that are exceptions to these rules, though.\n"},{"id":"398007","messageId":"xmqqtv0e7g8q.fsf@gitster.c.googlers.com","threadId":"53464","inReplyTo":"4e36c8a0-4f43-79f8-137f-5fd080066af6@gmail.com","subject":"Re: [PATCH 1/2] submodule: port subcommand 'set-branch' from shell to C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-17T15:06:45Z","receivedAt":"2020-05-17T15:06:50Z","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> For those who lack the context, during the conversion of the script\n> Shourya faced an issue where the '--branch' argument did not work as\n> expected. He described the issue in a private e-mail where Christian\n> pointed out the following (quoting his reply hoping he doesn't mind):\n>\n>> On Tue, May 12, 2020 at 5:55 PM Christian Couder\n> <christian.couder@gmail.com> wrote:\n>>>\n>>> In your commit (in submodule.sh line 781) there is:\n>>>\n>>>         `git ${wt_prefix:+-C \"$wt_prefix\"} ${prefix:+--super-prefix\n>>> \"$prefix\"} submodule--helper set-branch ${GIT_QUIET:+--quiet}\n>>> ${branch:+--branch} ${default:+--default} -- \"$@\"`\n>>\n>> Actually the issue might come from the above. I think it should use \n> ${branch:+--branch $branch} instead of ${branch:+--branch}.\n>>\n>> See: https://www.tldp.org/LDP/abs/html/parameter-substitution.html\n>\n> That's why Shourya mentions the '$branch' as extra. Of course, that's\n> how it is supposed to be in the first place :)\n\nPerhaps all of that should be removed from the log message and\ninstead go after the three-dash line then.\n\nThanks.\n\n"},{"id":"398010","messageId":"b9c28f5c-d0da-7b54-7e9b-2e8cdce8b838@gmail.com","threadId":"53464","inReplyTo":"xmqqtv0e7g8q.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 1/2] submodule: port subcommand 'set-branch' from shell to C","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2020-05-17T15:21:16Z","receivedAt":"2020-05-17T15:21:28Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"On 17-05-2020 20:36, Junio C Hamano wrote:\n> Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes:\n> \n>> For those who lack the context, during the conversion of the script\n>> Shourya faced an issue where the '--branch' argument did not work as\n>> expected. He described the issue in a private e-mail where Christian\n>> pointed out the following (quoting his reply hoping he doesn't mind):\n>>\n>>> On Tue, May 12, 2020 at 5:55 PM Christian Couder\n>> <christian.couder@gmail.com> wrote:\n>>>>\n>>>> In your commit (in submodule.sh line 781) there is:\n>>>>\n>>>>          `git ${wt_prefix:+-C \"$wt_prefix\"} ${prefix:+--super-prefix\n>>>> \"$prefix\"} submodule--helper set-branch ${GIT_QUIET:+--quiet}\n>>>> ${branch:+--branch} ${default:+--default} -- \"$@\"`\n>>>\n>>> Actually the issue might come from the above. I think it should use\n>> ${branch:+--branch $branch} instead of ${branch:+--branch}.\n>>>\n>>> See: https://www.tldp.org/LDP/abs/html/parameter-substitution.html\n>>\n>> That's why Shourya mentions the '$branch' as extra. Of course, that's\n>> how it is supposed to be in the first place :)\n> \n> Perhaps all of that should be removed from the log message and\n> instead go after the three-dash line then.\n> \n\nI believe it's already after the three-dash line. The log message reads:\n\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 > Mentored-by: Christian Couder<chriscool@tuxfamily.org>\n > Mentored-by: Kaartic Sivaraam<kaartic.sivaraam@gmail.com>\n > Signed-off-by: Shourya Shukla<shouryashukla.oo@gmail.com>\n\nIt might use more explanation, maybe.\n\n-- \nSivaraam\n"},{"id":"398014","messageId":"20200517161151.GA30938@danh.dev","threadId":"53464","inReplyTo":"20200513201737.55778-1-shouryashukla.oo@gmail.com","subject":"Re: [PATCH 1/2] submodule: port subcommand 'set-branch' from shell to C","fromName":"Đoàn Trần Công Danh","fromEmail":"congdanhqx@gmail.com","sentAt":"2020-05-17T16:11:51Z","receivedAt":"2020-05-17T16:11:56Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"On 2020-05-14 01:47:36+0530, Shourya Shukla <shouryashukla.oo@gmail.com> wrote:\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index 39ebdf25b5..2438ef576e 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -719,7 +719,6 @@ cmd_update()\n>  # $@ = requested path\n>  #\n>  cmd_set_branch() {\n> -\tunset_branch=false\n>  \tbranch=\n>  \n>  \twhile test $# -ne 0\n> @@ -729,7 +728,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\nHi Shourya,\n\nThanks for your hard work,\n\nI skimmed over the current code, it seems like this:\n\n\tdefault=1\n\nis new.\n\nIf my understanding is correct, please reset its value\nin the beginning of this function, to avoid:\n\n- a side effect from an assignment in get_submodule_config\n  (if there's such a side effect, I'm NOT really sure).\n- effect of an environment variable, c.f.\n  https://lore.kernel.org/git/20200402084251.85840-1-zhiyou.jx@alibaba-inc.com/\n\n\n-- \nDanh\n"}]}