{"thread":{"id":"53523","subject":"[PATCH v3] submodule: port subcommand 'set-branch' from shell to C","startedAt":"2020-05-21T16:38:39Z","lastAt":"2020-06-04T19:26:35Z","messageCount":29,"participants":["Shourya Shukla","Junio C Hamano","Denton Liu","Đoàn Trần Công Danh","Johannes Schindelin","Kaartic Sivaraam","Christian Couder"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"398363","messageId":"20200521163819.12544-1-shouryashukla.oo@gmail.com","threadId":"53523","inReplyTo":null,"subject":"[PATCH v3] submodule: port subcommand 'set-branch' from shell to C","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-05-21T16:38:19Z","receivedAt":"2020-05-21T16:38:39Z","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 and call it via\n'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>\nHelped-by: Eric Sunshine <sunshine@sunshineco.com>\nSigned-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>\n---\nThank you for the review Eric. I have changed the commit message,\nand the error prompts. Also, I have added a brief comment about\nthe `quiet` option.\n\n builtin/submodule--helper.c | 45 +++++++++++++++++++++++++++++++++++++\n git-submodule.sh            | 32 +++-----------------------\n 2 files changed, 48 insertions(+), 29 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex f50745a03f..d14b9856a3 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2284,6 +2284,50 @@ 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+\t/*\n+\t * The `quiet` option is present for backward compatibility\n+\t * but is currently not used.\n+\t */\n+\tint quiet = 0, opt_default = 0;\n+\tconst char *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\")),\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\")),\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(_(\"--branch or --default required\"));\n+\n+\tif (opt_branch && opt_default)\n+\t\tdie(_(\"--branch and --default are mutually exclusive\"));\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 +2359,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":"398383","messageId":"xmqqk115ruux.fsf@gitster.c.googlers.com","threadId":"53523","inReplyTo":"20200521163819.12544-1-shouryashukla.oo@gmail.com","subject":"Re: [PATCH v3] submodule: port subcommand 'set-branch' from shell to C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-21T18:44:22Z","receivedAt":"2020-05-21T18:44:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shourya Shukla <shouryashukla.oo@gmail.com> writes:\n\n> Convert submodule subcommand 'set-branch' to a builtin and call it via\n> 'git-submodule.sh'.\n>\n> Mentored-by: Christian Couder <chriscool@tuxfamily.org>\n> Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>\n> Helped-by: Denton Liu <liu.denton@gmail.com>\n> Helped-by: Eric Sunshine <sunshine@sunshineco.com>\n> Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>\n> ---\n> Thank you for the review Eric. I have changed the commit message,\n> and the error prompts. Also, I have added a brief comment about\n> the `quiet` option.\n\nSorry, I may have missed the previous rounds of discussion, but the\ncomment adds more puzzles than it helps readers.  \"is currently not\nused\" can be seen from the code, but it is totally unclear why it is\nnot used.  Is that a design decision to always keep quiet or always\ntalkative (if so, \"suppress output...\" is not a good description)?\nIs that that this is a WIP patch that the behaviour the option aims\nto achieve hasn't been implemented?  Is it that no existing callers\npass \"-q\" to the scripted version, so there is no need to support\nit (if so, why do we even accept it in the first place)?  Is it that\nall existing callers pass \"-q\" so we need to accept it, but there is\nnothing we need to make verbose so the variable is not passed around\nin the codepath?\n\n\n\n\n"},{"id":"398392","messageId":"20200521190329.GB615266@generichostname","threadId":"53523","inReplyTo":"xmqqk115ruux.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3] submodule: port subcommand 'set-branch' from shell to C","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-05-21T19:03:29Z","receivedAt":"2020-05-21T19:03:35Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"On Thu, May 21, 2020 at 11:44:22AM -0700, Junio C Hamano wrote:\n> Shourya Shukla <shouryashukla.oo@gmail.com> writes:\n> \n> > Convert submodule subcommand 'set-branch' to a builtin and call it via\n> > 'git-submodule.sh'.\n> >\n> > Mentored-by: Christian Couder <chriscool@tuxfamily.org>\n> > Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>\n> > Helped-by: Denton Liu <liu.denton@gmail.com>\n> > Helped-by: Eric Sunshine <sunshine@sunshineco.com>\n> > Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>\n> > ---\n> > Thank you for the review Eric. I have changed the commit message,\n> > and the error prompts. Also, I have added a brief comment about\n> > the `quiet` option.\n> \n> Sorry, I may have missed the previous rounds of discussion, but the\n> comment adds more puzzles than it helps readers.  \"is currently not\n> used\" can be seen from the code, but it is totally unclear why it is\n> not used.  Is that a design decision to always keep quiet or always\n> talkative (if so, \"suppress output...\" is not a good description)?\n> Is that that this is a WIP patch that the behaviour the option aims\n> to achieve hasn't been implemented?  Is it that no existing callers\n> pass \"-q\" to the scripted version, so there is no need to support\n> it (if so, why do we even accept it in the first place)?  Is it that\n> all existing callers pass \"-q\" so we need to accept it, but there is\n> nothing we need to make verbose so the variable is not passed around\n> in the codepath?\n\nAs the original author of the shell code, I had it accept -q because,\nwith the other subcommmands, you can pass -q either before or after the\nsubcommand such as\n\n\t$ git submodule -q sync\n\nor\n\t$ git submodule sync -q\n\nand I wanted set-branch to retain that behaviour even though -q\nultimately doesn't affect set-branch at all since it's already a quiet\ncommand.\n\nPerhaps as a follow-up to this patch, we could stop accepting -q in\nset-branch. I highly doubt that anyone is using it anyway.\n"},{"id":"398396","messageId":"xmqqftbtrrt6.fsf@gitster.c.googlers.com","threadId":"53523","inReplyTo":"20200521190329.GB615266@generichostname","subject":"Re: [PATCH v3] submodule: port subcommand 'set-branch' from shell to C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-21T19:50:13Z","receivedAt":"2020-05-21T19:50:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu <liu.denton@gmail.com> writes:\n\n>> Sorry, I may have missed the previous rounds of discussion, but the\n>> comment adds more puzzles than it helps readers.  \"is currently not\n>> used\" can be seen from the code, but it is totally unclear why it is\n>> not used.  Is that a design decision to always keep quiet or always\n>> talkative (if so, \"suppress output...\" is not a good description)?\n>> Is that that this is a WIP patch that the behaviour the option aims\n>> to achieve hasn't been implemented?  Is it that no existing callers\n>> pass \"-q\" to the scripted version, so there is no need to support\n>> it (if so, why do we even accept it in the first place)?  Is it that\n>> all existing callers pass \"-q\" so we need to accept it, but there is\n>> nothing we need to make verbose so the variable is not passed around\n>> in the codepath?\n>\n> As the original author of the shell code, I had it accept -q because,\n> with the other subcommmands, you can pass -q either before or after the\n> subcommand such as\n>\n> \t$ git submodule -q sync\n>\n> or\n> \t$ git submodule sync -q\n>\n> and I wanted set-branch to retain that behaviour even though -q\n> ultimately doesn't affect set-branch at all since it's already a quiet\n> command.\n\nOK, so \"we accept -q for uniformity across subcommands, but there is\nnothing to make less verbose in this subcommand\" is the answer to my\nquestion.\n\nThat cannot be read from \"... is currently not used\"; especially\nwith \"currently\", I expect that most readers would expect we would\nstart using it in the (near) future, and some other readers would\nguess that something used to be talkative and we squelched it using\nthe option but there no longer is such need because that something\nis now quiet by default and there is no option to make it talkative.\n\n"},{"id":"398407","messageId":"20200521230453.GB2042@danh.dev","threadId":"53523","inReplyTo":"20200521163819.12544-1-shouryashukla.oo@gmail.com","subject":"Re: [PATCH v3] submodule: port subcommand 'set-branch' from shell to C","fromName":"Đoàn Trần Công Danh","fromEmail":"congdanhqx@gmail.com","sentAt":"2020-05-21T23:04:53Z","receivedAt":"2020-05-21T23:04:59Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"Hi Shourya,\n\nOn 2020-05-21 22:08:19+0530, Shourya Shukla <shouryashukla.oo@gmail.com> wrote:\n> Thank you for the review Eric. I have changed the commit message,\n> and the error prompts. Also, I have added a brief comment about\n> the `quiet` option.\n> \n> +\t/*\n> +\t * The `quiet` option is present for backward compatibility\n> +\t * but is currently not used.\n> +\t */\n> +\tint quiet = 0, opt_default = 0;\n> +\tconst char *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\")),\n\nIIUC, this option is provided to be backward compatible with old shell\nversion, and this option doesn't affect anything.\n\nWould it make sense to hide quiet from default usage, via:\n\n\tOPT_NOOP_NOARG(0, \"quiet\")\n\nI may missed some discussion related to the decision to keep it\nOPT__QUIET.\n\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\")),\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\nAnd if above comment is applicable, remove `--quiet` from here.\n\n> +\tgit ${wt_prefix:+-C \"$wt_prefix\"} ${prefix:+--super-prefix \"$prefix\"} submodule--helper set-branch ${GIT_QUIET:+--quiet} ${branch:+--branch $branch} ${default:+--default} -- \"$@\"\n\nI think we need to quote `$branch`, no?\n\n\t${branch:+--branch \"$branch\"}\n\n-- \nDanh\n"},{"id":"398435","messageId":"20200522193907.GA4780@konoha","threadId":"53523","inReplyTo":"xmqqftbtrrt6.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3] submodule: port subcommand 'set-branch' from shell to C","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-05-22T19:39:07Z","receivedAt":"2020-05-22T19:39:17Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"On 21/05 12:50, Junio C Hamano wrote:\n> OK, so \"we accept -q for uniformity across subcommands, but there is\n> nothing to make less verbose in this subcommand\" is the answer to my\n> question.\n> \n> That cannot be read from \"... is currently not used\"; especially\n> with \"currently\", I expect that most readers would expect we would\n> start using it in the (near) future, and some other readers would\n> guess that something used to be talkative and we squelched it using\n> the option but there no longer is such need because that something\n> is now quiet by default and there is no option to make it talkative.\n\nWhat do you think should be the most apt comment here? Also, the rest of\nthe code is fine right?\n"},{"id":"398446","messageId":"nycvar.QRO.7.76.6.2005230012090.56@tvgsbejvaqbjf.bet","threadId":"53523","inReplyTo":"20200521163819.12544-1-shouryashukla.oo@gmail.com","subject":"Re: [PATCH v3] submodule: port subcommand 'set-branch' from shell to C","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-05-22T22:21:01Z","receivedAt":"2020-05-22T22:21:13Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Shourya,\n\nOn Thu, 21 May 2020, Shourya Shukla wrote:\n\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index f50745a03f..d14b9856a3 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -2284,6 +2284,50 @@ 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> +\t/*\n> +\t * The `quiet` option is present for backward compatibility\n> +\t * but is currently not used.\n> +\t */\n> +\tint quiet = 0, opt_default = 0;\n> +\tconst char *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\")),\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\")),\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(_(\"--branch or --default required\"));\n> +\n> +\tif (opt_branch && opt_default)\n> +\t\tdie(_(\"--branch and --default are mutually exclusive\"));\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\nWhat happens if this fails? E.g. when the permission is denied or disk is\nfull? This C code would then still `return 0`, pretending that it\nsucceeded. But the original shell script calls `git submodule--helper\nconfig [...]` which calls `module_config()`, which in turn passes through\nthe return value of the `config_set_in_gitmodules_file_gently()` call.\n\nIn other words, you need something like this:\n\n\tint ret;\n\n\t[...]\n\n\tret = config_set_in_gitmodules_file_gently(config_name, opt_branch);\n\n\tfree(config_name);\n\treturn ret;\n\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 +2359,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\nBTW I just noticed that the return value of these helpers is returned by\nthe `cmd_submodule__helper()` function. That is not correct, as the\nconvention is for Git's functions to return negative values in case of\nerrors _except_ for `cmd_*()` functions, which need to return an exit code\n(valid values are between 0 and 127).\n\nSo I think we'll also need this (it's unrelated to your patch, at least\nunrelated enough that it merits its own, separate patch):\n\n-                       return commands[i].fn(argc - 1, argv + 1, prefix);\n+                       return !!commands[i].fn(argc - 1, argv + 1, prefix);\n\nCiao,\nDscho\n\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..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> --\n> 2.26.2\n>\n>\n"},{"id":"398457","messageId":"20200523163929.7040-1-shouryashukla.oo@gmail.com","threadId":"53523","inReplyTo":"20200521163819.12544-1-shouryashukla.oo@gmail.com","subject":"[PATCH v4] submodule: port subcommand 'set-branch' from shell to C","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-05-23T16:39:29Z","receivedAt":"2020-05-23T16:39:41Z","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 and call it via\n'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>\nHelped-by: Eric Sunshine <sunshine@sunshineco.com>\nSigned-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>\n---\nThank you for the review Junio and Johannes. I have made the requested\nchanges.\n\n builtin/submodule--helper.c | 45 +++++++++++++++++++++++++++++++++++++\n git-submodule.sh            | 32 +++-----------------------\n 2 files changed, 48 insertions(+), 29 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex f50745a03f..7e844e8971 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2284,6 +2284,50 @@ 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+\t/*\n+\t * We accept the `quiet` option for uniformity across subcommands,\n+\t * though there is nothing to make less verbose in this subcommand.\n+\t */\n+\tint quiet = 0, opt_default = 0, ret;\n+\tconst char *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\")),\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\")),\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(_(\"--branch or --default required\"));\n+\n+\tif (opt_branch && opt_default)\n+\t\tdie(_(\"--branch and --default are mutually exclusive\"));\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+\tret = config_set_in_gitmodules_file_gently(config_name, opt_branch);\n+\n+\tfree(config_name);\n+\treturn ret;\n+}\n+\n #define SUPPORT_SUPER_PREFIX (1<<0)\n \n struct cmd_struct {\n@@ -2315,6 +2359,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":"398458","messageId":"33127873-fb19-2bd5-3028-bcd1757e92e5@gmail.com","threadId":"53523","inReplyTo":"20200523163929.7040-1-shouryashukla.oo@gmail.com","subject":"Re: [PATCH v4] submodule: port subcommand 'set-branch' from shell to C","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2020-05-23T18:49:38Z","receivedAt":"2020-05-23T18:49:48Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"Hi Shourya,\n\nI believe you missed Danh's v3 comments[1]. I'm mentioning them inline \nwith some additional comments.\n\nOn 23-05-2020 22:09, Shourya Shukla wrote:\n> \n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index f50745a03f..7e844e8971 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -2284,6 +2284,50 @@ 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> +\t/*\n> +\t * We accept the `quiet` option for uniformity across subcommands,\n> +\t * though there is nothing to make less verbose in this subcommand.\n> +\t */\n> +\tint quiet = 0, opt_default = 0, ret;\n> +\tconst char *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\")),\n\nAs '--quiet' in 'set-branch' is a no-op and is being accepted only for \nuniformity, I think it makes sense to use OPT_NOOP_NOARG instead of \nOPT__QUIET for specifying it, as suggested by Danh.\n\nAlso, the description \"suppress output for setting default tracking \nbranch\" doesn't seem to be valid anymore as we don't print anything when \nset-branch succeeds.\n\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\")),\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\nI also agree with the Danh here that '--quiet' could be removed from \nusage. There's no point in mentioning '--quiet' in the usage when it has \nno effect.\n\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index 39ebdf25b5..8c56191f77 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\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\nDanh questioned whether '$branch' needs to be quoted here. I too think \nit needs to be quoted unless I'm missing something.\n\n\n---\nFootnotes:\n[1]: \nhttps://lore.kernel.org/git/nycvar.QRO.7.76.6.2005230012090.56@tvgsbejvaqbjf.bet/T/#maf26182b084087ed08a2a72d3da2ee2026b1618e\n\nThanks,\nSivaraam\n"},{"id":"398463","messageId":"20200523231838.GB1981@danh.dev","threadId":"53523","inReplyTo":"33127873-fb19-2bd5-3028-bcd1757e92e5@gmail.com","subject":"Re: [PATCH v4] submodule: port subcommand 'set-branch' from shell to C","fromName":"Đoàn Trần Công Danh","fromEmail":"congdanhqx@gmail.com","sentAt":"2020-05-23T23:18:38Z","receivedAt":"2020-05-23T23:18:43Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"Hi Kaartic,\n\nOn 2020-05-24 00:19:38+0530, Kaartic Sivaraam <kaartic.sivaraam@gmail.com> wrote:\n> I believe you missed Danh's v3 comments[1]. I'm mentioning them inline with\n> some additional comments.\n\nThanks for checking this.\n\n> On 23-05-2020 22:09, Shourya Shukla wrote:\n> > \n> > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> > index f50745a03f..7e844e8971 100644\n> > --- a/builtin/submodule--helper.c\n> > +++ b/builtin/submodule--helper.c\n> > @@ -2284,6 +2284,50 @@ static int module_set_url(int argc, const char **argv, const char *prefix)\n> >   \treturn 0;\n> >   }\n> > +static int module_set_branch(int argc, const char **argv, const char *prefix)\n> > +{\n> > +\t/*\n> > +\t * We accept the `quiet` option for uniformity across subcommands,\n> > +\t * though there is nothing to make less verbose in this subcommand.\n> > +\t */\n> > +\tint quiet = 0, opt_default = 0, ret;\n> > +\tconst char *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\")),\n> \n> As '--quiet' in 'set-branch' is a no-op and is being accepted only for\n> uniformity, I think it makes sense to use OPT_NOOP_NOARG instead of\n> OPT__QUIET for specifying it, as suggested by Danh.\n\nYay, I still think it's better to use OPT_NOOP_NOARG, (and with shortopt q,\nwhich I forgot in previous reply.)\n\n\tOPT_NOOP_NOARG('q', \"quiet\")\n\n> Also, the description \"suppress output for setting default tracking branch\"\n> doesn't seem to be valid anymore as we don't print anything when set-branch\n> succeeds.\n\nOPT_NOOP_NOARG will take care of description itself. Even if we choose\nto not use OPT_NOOP_NOARG, a better description should be provided.\n\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\")),\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> \n> I also agree with the Danh here that '--quiet' could be removed from usage.\n> There's no point in mentioning '--quiet' in the usage when it has no effect.\n> \n> > diff --git a/git-submodule.sh b/git-submodule.sh\n> > index 39ebdf25b5..8c56191f77 100755\n> > --- a/git-submodule.sh\n> > +++ b/git-submodule.sh\n> > @@ -750,33 +750,7 @@ cmd_set_branch() {\n> >   \t\tshift\n> >   \tdone\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> Danh questioned whether '$branch' needs to be quoted here. I too think it\n> needs to be quoted unless I'm missing something.\n> \n> \n> ---\n> Footnotes:\n> [1]: https://lore.kernel.org/git/nycvar.QRO.7.76.6.2005230012090.56@tvgsbejvaqbjf.bet/T/#maf26182b084087ed08a2a72d3da2ee2026b1618e\n\nFor the better record, I think it's better to use a permenent link,\njust in case lore.kernel.org go into the dust someday,\npeople can still have a reference if they have an archive.\n\nhttps://lore.kernel.org/git/20200521230453.GB2042@danh.dev/\n\n-- \nDanh\n"},{"id":"398469","messageId":"xmqqmu5xqpsz.fsf@gitster.c.googlers.com","threadId":"53523","inReplyTo":"20200522193907.GA4780@konoha","subject":"Re: [PATCH v3] submodule: port subcommand 'set-branch' from shell to C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-24T16:07:56Z","receivedAt":"2020-05-24T16:08:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shourya Shukla <shouryashukla.oo@gmail.com> writes:\n\n> On 21/05 12:50, Junio C Hamano wrote:\n>> OK, so \"we accept -q for uniformity across subcommands, but there is\n>> nothing to make less verbose in this subcommand\" is the answer to my\n>> question.\n>> \n>> That cannot be read from \"... is currently not used\"; especially\n>> with \"currently\", I expect that most readers would expect we would\n>> start using it in the (near) future, and some other readers would\n>> guess that something used to be talkative and we squelched it using\n>> the option but there no longer is such need because that something\n>> is now quiet by default and there is no option to make it talkative.\n>\n> What do you think should be the most apt comment here?\n\n\"we accept -q for uniformity across subcommands, but there is nothing\nto make less verbose in this subcommand\", perhaps?\n\n> Also, the rest of the code is fine right?\n\nI didn't spot anything bad worth pointing out when I sent the review\nmessage, but that does not necessarily mean the code is \"fine\" ;-) \n\nI see you have v4 sent out already, which probably has more\nimprovements based on others' input.  Thanks for working on this\ntopic.\n\n"},{"id":"398478","messageId":"xmqqtv05orgq.fsf@gitster.c.googlers.com","threadId":"53523","inReplyTo":"nycvar.QRO.7.76.6.2005230012090.56@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v3] submodule: port subcommand 'set-branch' from shell to C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-24T23:15:01Z","receivedAt":"2020-05-24T23:15:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n>> +\tconfig_set_in_gitmodules_file_gently(config_name, opt_branch);\n>\n> What happens if this fails? E.g. when the permission is denied or disk is\n> full? This C code would then still `return 0`, pretending that it\n> succeeded. But the original shell script calls `git submodule--helper\n> config [...]` which calls `module_config()`, which in turn passes through\n> the return value of the `config_set_in_gitmodules_file_gently()` call.\n>\n> In other words, you need something like this:\n>\n> \tint ret;\n>\n> \t[...]\n>\n> \tret = config_set_in_gitmodules_file_gently(config_name, opt_branch);\n>\n> \tfree(config_name);\n> \treturn ret;\n\nMaking sure we check the return value of helper functions we call is\na good discipline, but this is not quite enough.\n\n> So I think we'll also need this (it's unrelated to your patch, at least\n> unrelated enough that it merits its own, separate patch):\n>\n> -                       return commands[i].fn(argc - 1, argv + 1, prefix);\n> +                       return !!commands[i].fn(argc - 1, argv + 1, prefix);\n\nI checked (not all but most of the) functions in that commands[]\ntable and they all seem to return 0 for success and positive\nnon-zero for failure.\n\nconfig_set_in_gitmodules_file_gently() takes the return value of a\nhelper function in its 'ret', gives an warning if it is negative,\nand returns that 'ret' literally to the caller.  You suggestion\nallows module_set_branch() return a negative value as-is.  You'd\nneed to return !!ret from there.\n\nThe \"unrelated\" change becomes only necessary if you do not do the\n!!ret in module_set_branch(); otherwise it is unneeded, I think.\n\nThanks.\n\n"},{"id":"398479","messageId":"xmqqpnasq5v7.fsf@gitster.c.googlers.com","threadId":"53523","inReplyTo":"xmqqtv05orgq.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3] submodule: port subcommand 'set-branch' from shell to C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-24T23:18:36Z","receivedAt":"2020-05-24T23:18:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n>\n>>> +\tconfig_set_in_gitmodules_file_gently(config_name, opt_branch);\n>>\n>> What happens if this fails? E.g. when the permission is denied or disk is\n>> full? This C code would then still `return 0`, pretending that it\n>> succeeded. But the original shell script calls `git submodule--helper\n>> config [...]` which calls `module_config()`, which in turn passes through\n>> the return value of the `config_set_in_gitmodules_file_gently()` call.\n>>\n>> In other words, you need something like this:\n>>\n>> \tint ret;\n>>\n>> \t[...]\n>>\n>> \tret = config_set_in_gitmodules_file_gently(config_name, opt_branch);\n>>\n>> \tfree(config_name);\n>> \treturn ret;\n>\n> Making sure we check the return value of helper functions we call is\n> a good discipline,...\n\nBy the way, another topic by you for set-url has exactly the same\nissue.  Its call to config_set_in_gitmodules_file_gently() can fail.\n\nSo can the call to sync_submodule(), but when it fails it won't come\nback, so we do not have to worry about not capturing its return\nvalue ;-)\n\n"},{"id":"398662","messageId":"20200527171358.GA22073@konoha","threadId":"53523","inReplyTo":"33127873-fb19-2bd5-3028-bcd1757e92e5@gmail.com","subject":"Re: [PATCH v4] submodule: port subcommand 'set-branch' from shell to C","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-05-27T17:13:58Z","receivedAt":"2020-05-27T17:14:07Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"On 24/05 12:19, Kaartic Sivaraam wrote:\n> As '--quiet' in 'set-branch' is a no-op and is being accepted only for\n> uniformity, I think it makes sense to use OPT_NOOP_NOARG instead of\n> OPT__QUIET for specifying it, as suggested by Danh.\n> \n> Also, the description \"suppress output for setting default tracking branch\"\n> doesn't seem to be valid anymore as we don't print anything when set-branch\n> succeeds.\n\nI think it will all boil down to the consistency of all the subcommands.\nChanging this would require making changes in various places: the C code\n(obviously), the shell script (not only the cmd_set_branch() function\nbut the part for accepting user input as well) and the Documentation (I\nmight have maybe missed a couple of other changes to list here too). Its\nnot that I don't want to do this, but it would add unnecessary changes\ndon't you think? I would love it if others could weigh in their opinions\ntoo about this.\n\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> Danh questioned whether '$branch' needs to be quoted here. I too think it\n> needs to be quoted unless I'm missing something.\n\nWe want to do this because $branch is an argument right?\n"},{"id":"398727","messageId":"20200528122147.GA1983@danh.dev","threadId":"53523","inReplyTo":"20200527171358.GA22073@konoha","subject":"Re: [PATCH v4] submodule: port subcommand 'set-branch' from shell to C","fromName":"Đoàn Trần Công Danh","fromEmail":"congdanhqx@gmail.com","sentAt":"2020-05-28T12:21:47Z","receivedAt":"2020-05-28T12:21:57Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"On 2020-05-27 22:43:58+0530, Shourya Shukla <shouryashukla.oo@gmail.com> wrote:\n> On 24/05 12:19, Kaartic Sivaraam wrote:\n> > As '--quiet' in 'set-branch' is a no-op and is being accepted only for\n> > uniformity, I think it makes sense to use OPT_NOOP_NOARG instead of\n> > OPT__QUIET for specifying it, as suggested by Danh.\n> > \n> > Also, the description \"suppress output for setting default tracking branch\"\n> > doesn't seem to be valid anymore as we don't print anything when set-branch\n> > succeeds.\n> \n> I think it will all boil down to the consistency of all the subcommands.\n> Changing this would require making changes in various places: the C code\n> (obviously), the shell script (not only the cmd_set_branch() function\n> but the part for accepting user input as well) and the Documentation (I\n> might have maybe missed a couple of other changes to list here too). Its\n\nI don't think this is a valid argument.\n\nUsing OPT_NOOP_NOARG doesn't require any change in shell script since\nthe binary still accepts -q|--quiet.\n\nThe documentation of --quiet is still valid (since it doesn't print\nanything regardless)\n\nThe only necessary change in in that C code.\n\n> not that I don't want to do this, but it would add unnecessary changes\n> don't you think? I would love it if others could weigh in their opinions\n> too about this.\n> \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> > Danh questioned whether '$branch' needs to be quoted here. I too think it\n> > needs to be quoted unless I'm missing something.\n> \n> We want to do this because $branch is an argument right?\n\nWe want to do this because we don't want to whitespace-split \"$branch\"\n\nLet's say, for some reason, this command was run:\n\n\tgit submodule set-branch --branch \"a-branch --branch another\" a-submodule\n\nThis version will run:\n\n\tgit submodule--helper --branch a-branch --branch another a-submodule\n\nWhich will success if there's a branch \"another\" in the \"a-submodule\".\nWhile that command should fail because we don't accept refname with\nspace.\n\n-- \nDanh\n"},{"id":"398735","messageId":"20200528140142.GA1951@danh.dev","threadId":"53523","inReplyTo":"20200528122147.GA1983@danh.dev","subject":"Re: [PATCH v4] submodule: port subcommand 'set-branch' from shell to C","fromName":"Đoàn Trần Công Danh","fromEmail":"congdanhqx@gmail.com","sentAt":"2020-05-28T14:01:42Z","receivedAt":"2020-05-28T14:01:49Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"On 2020-05-28 19:21:47+0700, Đoàn Trần Công Danh <congdanhqx@gmail.com> wrote:\n> On 2020-05-27 22:43:58+0530, Shourya Shukla <shouryashukla.oo@gmail.com> wrote:\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> > > Danh questioned whether '$branch' needs to be quoted here. I too think it\n> > > needs to be quoted unless I'm missing something.\n> > \n> > We want to do this because $branch is an argument right?\n> \n> We want to do this because we don't want to whitespace-split \"$branch\"\n> \n> Let's say, for some reason, this command was run:\n> \n> \tgit submodule set-branch --branch \"a-branch --branch another\" a-submodule\n\nAnyway, after typing this.\nI'm thinking a bit, then re-read gitcli(7),\nI think git-submodule is quite broken regarding to Git's guidelines:\n\n-----------8<----------\n\nHere are the rules regarding the \"flags\" that you should follow when you are\nscripting Git:\n\n * it's preferred to use the non-dashed form of Git commands, which means that\n   you should prefer `git foo` to `git-foo`.\n\n * splitting short options to separate words (prefer `git foo -a -b`\n   to `git foo -ab`, the latter may not even work).\n\n * when a command-line option takes an argument, use the 'stuck' form.  In\n   other words, write `git foo -oArg` instead of `git foo -o Arg` for short\n   options, and `git foo --long-opt=Arg` instead of `git foo --long-opt Arg`\n   for long options.  An option that takes optional option-argument must be\n   written in the 'stuck' form.\n------------>8--------------\n\nCurrent Git, with and without this change, this command will fail:\n\n\tgit submodule set-branch --branch=a-branch a-submodule\n\nThus, a script conformed with gitcli(7) will fail.\n(And our git-submodule(1) doesn't conform with gitcli(7), FWIW).\n\nAfter this change, those commands will success:\n\n\tgit submodule--helper set-branch --branch a-branch a-submodule\n\tgit submodule set-branch --branch \"a-branch --branch=another\" a-submodule\n\n(The second one was written for demonstration purpose only,\nI don't expect it will success)\n\nThis isn't related to this change, and git-submodule(1) will be\nrewritten in C in the very near future.\nJust want to make sure it's awared.\n\n> \n> This version will run:\n> \n> \tgit submodule--helper --branch a-branch --branch another a-submodule\n> \n> Which will success if there's a branch \"another\" in the \"a-submodule\".\n> While that command should fail because we don't accept refname with\n> space.\n\n-- \nDanh\n"},{"id":"398749","messageId":"20200528155522.GA16787@danh.dev","threadId":"53523","inReplyTo":"20200528140142.GA1951@danh.dev","subject":"Re: [PATCH v4] submodule: port subcommand 'set-branch' from shell to C","fromName":"Đoàn Trần Công Danh","fromEmail":"congdanhqx@gmail.com","sentAt":"2020-05-28T15:55:22Z","receivedAt":"2020-05-28T15:55:34Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"On 2020-05-28 21:01:42+0700, Đoàn Trần Công Danh <congdanhqx@gmail.com> wrote:\n> On 2020-05-28 19:21:47+0700, Đoàn Trần Công Danh <congdanhqx@gmail.com> wrote:\n> > On 2020-05-27 22:43:58+0530, Shourya Shukla <shouryashukla.oo@gmail.com> wrote:\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> > > > Danh questioned whether '$branch' needs to be quoted here. I too think it\n> > > > needs to be quoted unless I'm missing something.\n> > > \n> > > We want to do this because $branch is an argument right?\n> > \n> > We want to do this because we don't want to whitespace-split \"$branch\"\n> > \n> > Let's say, for some reason, this command was run:\n> > \n> > \tgit submodule set-branch --branch \"a-branch --branch another\" a-submodule\n> \n> Anyway, after typing this.\n> I'm thinking a bit, then re-read gitcli(7),\n> I think git-submodule is quite broken regarding to Git's guidelines:\n> \n> -----------8<----------\n> \n> Here are the rules regarding the \"flags\" that you should follow when you are\n> scripting Git:\n> \n>  * it's preferred to use the non-dashed form of Git commands, which means that\n>    you should prefer `git foo` to `git-foo`.\n> \n>  * splitting short options to separate words (prefer `git foo -a -b`\n>    to `git foo -ab`, the latter may not even work).\n> \n>  * when a command-line option takes an argument, use the 'stuck' form.  In\n>    other words, write `git foo -oArg` instead of `git foo -o Arg` for short\n>    options, and `git foo --long-opt=Arg` instead of `git foo --long-opt Arg`\n>    for long options.  An option that takes optional option-argument must be\n>    written in the 'stuck' form.\n> ------------>8--------------\n> \n> Current Git, with and without this change, this command will fail:\n> \n> \tgit submodule set-branch --branch=a-branch a-submodule\n> \n> Thus, a script conformed with gitcli(7) will fail.\n> (And our git-submodule(1) doesn't conform with gitcli(7), FWIW).\n> \n> After this change, those commands will success:\n> \n> \tgit submodule--helper set-branch --branch a-branch a-submodule\n\nThis should be read:\n\n \tgit submodule--helper set-branch --branch=a-branch a-submodule\n\n> \tgit submodule set-branch --branch \"a-branch --branch=another\" a-submodule\n> \n> (The second one was written for demonstration purpose only,\n> I don't expect it will success)\n> \n> This isn't related to this change, and git-submodule(1) will be\n> rewritten in C in the very near future.\n> Just want to make sure it's awared.\n> \n> > \n> > This version will run:\n> > \n> > \tgit submodule--helper --branch a-branch --branch another a-submodule\n> > \n> > Which will success if there's a branch \"another\" in the \"a-submodule\".\n> > While that command should fail because we don't accept refname with\n> > space.\n\nIt's me being noisy again.\nI'm still puzzled by this idea (and I drank too much coffee, today).\n\nI think the day of conversion of submodule from shell to C finish,\nwe can use current git-submodule--helper as the new git-submodule.\n\nWith that idea, I think why don't we passed all arguments from\n\n\tgit submodule set-branch\n\ninto git-submodule--helper.\n(Yes, the idea is wrong because the usage output will have\ngit submodule--helper as $0)\n\nI tried that idea and run the test.\nTo my surprise the test failed :(.\n\nTurn out git-submodule--helper set-branch doesn't do its advertised job,\ngit-submodule--helper set-branch doesn't understand short options -d and -b\n\nWe'll need this fixup regardless of the agreement on my other concerns.\n----------8<---------\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 305c9abb3b..64636161a7 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2291,9 +2291,9 @@ static int module_set_branch(int argc, const char **argv, const char *prefix)\n \tstruct option options[] = {\n \t\tOPT__QUIET(&quiet,\n \t\t\tN_(\"suppress output for setting default tracking branch\")),\n-\t\tOPT_BOOL(0, \"default\", &opt_default,\n+\t\tOPT_BOOL('d', \"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\tOPT_STRING('b', \"branch\", &opt_branch, N_(\"branch\"),\n \t\t\tN_(\"set the default tracking branch\")),\n \t\tOPT_END()\n \t};\n------8<-----------\n\n\n\n-- \nDanh\n"},{"id":"399032","messageId":"20200602163523.7131-1-shouryashukla.oo@gmail.com","threadId":"53523","inReplyTo":"20200523163929.7040-1-shouryashukla.oo@gmail.com","subject":"[GSoC][PATCH v5] submodule: port subcommand 'set-branch' from shell to C","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-06-02T16:35:23Z","receivedAt":"2020-06-02T16:35:38Z","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 and call it via\n'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>\nHelped-by: Eric Sunshine <sunshine@sunshineco.com>\nHelped-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>\nSigned-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>\n---\nHere is the v5 of the subcommand. Thank you Danh for the feedback! I\napologise for not replying on time. I have taken into account Danh's\nsuggestions on the `quiet` option as well as done the fixup Dscho\nsuggested (fixed by Junio here:\nhttps://github.com/gitster/git/commit/77ba62f66ff8e3de54d81c240542edb42a2711c7)\n\n builtin/submodule--helper.c | 44 +++++++++++++++++++++++++++++++++++++\n git-submodule.sh            | 32 +++------------------------\n 2 files changed, 47 insertions(+), 29 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex f50745a03f..a974e17571 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2284,6 +2284,49 @@ 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 opt_default = 0, ret;\n+\tconst char *opt_branch = NULL;\n+\tconst char *path;\n+\tchar *config_name;\n+\n+\t/*\n+\t * We accept the `quiet` option for uniformity across subcommands,\n+\t * though there is nothing to make less verbose in this subcommand.\n+\t */\n+\tstruct option options[] = {\n+\t\tOPT_NOOP_NOARG('q', \"quiet\"),\n+\t\tOPT_BOOL('d', \"default\", &opt_default,\n+\t\t\tN_(\"set the default tracking branch to master\")),\n+\t\tOPT_STRING('b', \"branch\", &opt_branch, N_(\"branch\"),\n+\t\t\tN_(\"set the default tracking branch\")),\n+\t\tOPT_END()\n+\t};\n+\tconst char *const usage[] = {\n+\t\tN_(\"git submodule--helper set-branch [-q|--quiet] (-d|--default) <path>\"),\n+\t\tN_(\"git submodule--helper set-branch [-q|--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(_(\"--branch or --default required\"));\n+\n+\tif (opt_branch && opt_default)\n+\t\tdie(_(\"--branch and --default are mutually exclusive\"));\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+\tret = config_set_in_gitmodules_file_gently(config_name, opt_branch);\n+\n+\tfree(config_name);\n+\treturn !!ret;\n+}\n+\n #define SUPPORT_SUPER_PREFIX (1<<0)\n \n struct cmd_struct {\n@@ -2315,6 +2358,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..43eb6051d2 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":"399039","messageId":"xmqqzh9ls622.fsf@gitster.c.googlers.com","threadId":"53523","inReplyTo":"20200602163523.7131-1-shouryashukla.oo@gmail.com","subject":"Re: [GSoC][PATCH v5] submodule: port subcommand 'set-branch' from shell to C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-02T17:58:45Z","receivedAt":"2020-06-02T17:58:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shourya Shukla <shouryashukla.oo@gmail.com> writes:\n\n> Convert submodule subcommand 'set-branch' to a builtin and call it via\n> 'git-submodule.sh'.\n>\n> Mentored-by: Christian Couder <chriscool@tuxfamily.org>\n> Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>\n> Helped-by: Denton Liu <liu.denton@gmail.com>\n> Helped-by: Eric Sunshine <sunshine@sunshineco.com>\n> Helped-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>\n> Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>\n> ---\n> Here is the v5 of the subcommand. Thank you Danh for the feedback! I\n> apologise for not replying on time. I have taken into account Danh's\n> suggestions on the `quiet` option as well as done the fixup Dscho\n> suggested (fixed by Junio here:\n> https://github.com/gitster/git/commit/77ba62f66ff8e3de54d81c240542edb42a2711c7)\n>\n>  builtin/submodule--helper.c | 44 +++++++++++++++++++++++++++++++++++++\n>  git-submodule.sh            | 32 +++------------------------\n>  2 files changed, 47 insertions(+), 29 deletions(-)\n>\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index f50745a03f..a974e17571 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -2284,6 +2284,49 @@ 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 opt_default = 0, ret;\n> +\tconst char *opt_branch = NULL;\n> +\tconst char *path;\n> +\tchar *config_name;\n> +\n> +\t/*\n> +\t * We accept the `quiet` option for uniformity across subcommands,\n> +\t * though there is nothing to make less verbose in this subcommand.\n> +\t */\n> +\tstruct option options[] = {\n> +\t\tOPT_NOOP_NOARG('q', \"quiet\"),\n> +\t\tOPT_BOOL('d', \"default\", &opt_default,\n> +\t\t\tN_(\"set the default tracking branch to master\")),\n> +\t\tOPT_STRING('b', \"branch\", &opt_branch, N_(\"branch\"),\n> +\t\t\tN_(\"set the default tracking branch\")),\n> ...\n> +\t\tOPT_END()\n> +\t};\n> +\tconst char *const usage[] = {\n> +\t\tN_(\"git submodule--helper set-branch [-q|--quiet] (-d|--default) <path>\"),\n> +\t\tN_(\"git submodule--helper set-branch [-q|--quiet] (-b|--branch) <branch> <path>\"),\n\n\nI notice that we gained back -d and -b shorthands that was\nadvertised but not implemented the previous rounds.  It is a bit\ncurious that we are adding these short-hands that nobody uses,\nthough.  \n\nWill queue.  Thanks.\n"},{"id":"399043","messageId":"1b851e49-3bb1-3b59-7f24-b903c5514391@gmail.com","threadId":"53523","inReplyTo":"20200602163523.7131-1-shouryashukla.oo@gmail.com","subject":"Re: [GSoC][PATCH v5] submodule: port subcommand 'set-branch' from shell to C","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2020-06-02T19:01:46Z","receivedAt":"2020-06-02T19:01:54Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"On 02-06-2020 22:05, Shourya Shukla wrote:\n> Convert submodule subcommand 'set-branch' to a builtin and call it via\n> 'git-submodule.sh'.\n> \n> Mentored-by: Christian Couder <chriscool@tuxfamily.org>\n> Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>\n> Helped-by: Denton Liu <liu.denton@gmail.com>\n> Helped-by: Eric Sunshine <sunshine@sunshineco.com>\n> Helped-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>\n> Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>\n> ---\n> Here is the v5 of the subcommand. Thank you Danh for the feedback! I\n> apologise for not replying on time. I have taken into account Danh's\n> suggestions on the `quiet` option as well as done the fixup Dscho\n> suggested (fixed by Junio here:\n> https://github.com/gitster/git/commit/77ba62f66ff8e3de54d81c240542edb42a2711c7)\n> \n>   builtin/submodule--helper.c | 44 +++++++++++++++++++++++++++++++++++++\n>   git-submodule.sh            | 32 +++------------------------\n>   2 files changed, 47 insertions(+), 29 deletions(-)\n> \n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index f50745a03f..a974e17571 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -2284,6 +2284,49 @@ 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 opt_default = 0, ret;\n> +\tconst char *opt_branch = NULL;\n> +\tconst char *path;\n> +\tchar *config_name;\n> +\n> +\t/*\n> +\t * We accept the `quiet` option for uniformity across subcommands,\n> +\t * though there is nothing to make less verbose in this subcommand.\n> +\t */\n> +\tstruct option options[] = {\n> +\t\tOPT_NOOP_NOARG('q', \"quiet\"),\n> +\t\tOPT_BOOL('d', \"default\", &opt_default,\n> +\t\t\tN_(\"set the default tracking branch to master\")),\n> +\t\tOPT_STRING('b', \"branch\", &opt_branch, N_(\"branch\"),\n> +\t\t\tN_(\"set the default tracking branch\")),\n> +\t\tOPT_END()\n> +\t};\n> +\tconst char *const usage[] = {\n> +\t\tN_(\"git submodule--helper set-branch [-q|--quiet] (-d|--default) <path>\"),\n> +\t\tN_(\"git submodule--helper set-branch [-q|--quiet] (-b|--branch) <branch> <path>\"),\n> +\t\tNULL\n> +\t};\n\nI'm having second thoughts about my suggestion[1] to include\nthe short option for '--quiet' in the usage. This is the only\nusage in submodule--helper that mentions that '-q' is a short\nhand for '--quiet'. That seems inconsistent. I see two ways but\nI'm not sure which one of these would be better:\n\nA. Dropping the mention of '-q' in this usage thus making it consistent\n    with the other usages printed by submodule--helper.\n\nB. Fixing other usages of submodule--helper to mention that '-q' is\n    shorthand for quiet. This has the benefit of properly advertising\n    the shorthand.\n\nC. Just ignore this?\n\nI also noticed one other thing. A quote from\nDocumentation/CodingGuidelines regarding the usage for reference:\n\n>  Optional parts are enclosed in square brackets:\n>    [<extra>]\n>    (Zero or one <extra>.)\n> \n>    --exec-path[=<path>]\n>    (Option with an optional argument.  Note that the \"=\" is inside the\n>    brackets.)\n> \n>    [<patch>...]\n>    (Zero or more of <patch>.  Note that the dots are inside, not\n>    outside the brackets.)\n> \n>  Multiple alternatives are indicated with vertical bars:\n>    [-q | --quiet]\n>    [--utf8 | --no-utf8]\n> \n>  Parentheses are used for grouping:\n>    [(<rev> | <range>)...]\n>    (Any number of either <rev> or <range>.  Parens are needed to make\n>    it clear that \"...\" pertains to both <rev> and <range>.)\n> \n>    [(-p <parent>)...]\n>    (Any number of option -p, each with one <parent> argument.)\n> \n>    git remote set-head <name> (-a | -d | <branch>)\n>    (One and only one of \"-a\", \"-d\" or \"<branch>\" _must_ (no square\n>    brackets) be provided.)\n\nSo, according to this, I think the usage should be ...\n\n     git submodule--helper set-branch [-q | --quiet] [-d | --default] <path>\n\n... and ...\n\n     git submodule--helper set-branch [-q|--quiet] [-b | \n--branch]<branch> <path>\n\n... respectively.\n\n> +\t\tNULL\n> +\t};\n\n---\nFootnotes:\n\n[1]: \nhttps://github.com/periperidip/git/commit/9a8918bf0688c583740b3dddafdba82f47972442#r39606384\n\n-- \nSivaraam\n"},{"id":"399045","messageId":"CA+ARAtoDzoU=eu0mJom7LwVF60k4CtiuBha-RC7zkx9o7O=H3A@mail.gmail.com","threadId":"53523","inReplyTo":"1b851e49-3bb1-3b59-7f24-b903c5514391@gmail.com","subject":"Re: [GSoC][PATCH v5] submodule: port subcommand 'set-branch' from shell to C","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2020-06-02T19:10:58Z","receivedAt":"2020-06-02T19:11:14Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"On Wed, Jun 3, 2020 at 12:31 AM Kaartic Sivaraam\n<kaartic.sivaraam@gmail.com> wrote:\n>\n> I also noticed one other thing. A quote from\n> Documentation/CodingGuidelines regarding the usage for reference:\n>\n> >  Optional parts are enclosed in square brackets:\n> >    [<extra>]\n> >    (Zero or one <extra>.)\n> >\n> >    --exec-path[=<path>]\n> >    (Option with an optional argument.  Note that the \"=\" is inside the\n> >    brackets.)\n> >\n> >    [<patch>...]\n> >    (Zero or more of <patch>.  Note that the dots are inside, not\n> >    outside the brackets.)\n> >\n> >  Multiple alternatives are indicated with vertical bars:\n> >    [-q | --quiet]\n> >    [--utf8 | --no-utf8]\n> >\n> >  Parentheses are used for grouping:\n> >    [(<rev> | <range>)...]\n> >    (Any number of either <rev> or <range>.  Parens are needed to make\n> >    it clear that \"...\" pertains to both <rev> and <range>.)\n> >\n> >    [(-p <parent>)...]\n> >    (Any number of option -p, each with one <parent> argument.)\n> >\n> >    git remote set-head <name> (-a | -d | <branch>)\n> >    (One and only one of \"-a\", \"-d\" or \"<branch>\" _must_ (no square\n> >    brackets) be provided.)\n>\n> So, according to this, I think the usage should be ...\n>\n>      git submodule--helper set-branch [-q | --quiet] [-d | --default] <path>\n>\n> ... and ...\n>\n>      git submodule--helper set-branch [-q|--quiet] [-b |\n> --branch]<branch> <path>\n>\n\nApologies, my mail client messed a little with the formatting.\nThis should actually be:\n\n    git submodule--helper set-branch [-q | --quiet] [-b | --branch]\n<branch> <path>\n\n> ... respectively.\n>\n> > +             NULL\n> > +     };\n>\n> ---\n> Footnotes:\n>\n> [1]:\n> https://github.com/periperidip/git/commit/9a8918bf0688c583740b3dddafdba82f47972442#r39606384\n>\n\n-- \nSivaraam\n"},{"id":"399049","messageId":"CAP8UFD3Qe3iDe+ymKsqv9HarFLYDohXmUGbkNwZ4MdVQ=XP7yQ@mail.gmail.com","threadId":"53523","inReplyTo":"1b851e49-3bb1-3b59-7f24-b903c5514391@gmail.com","subject":"Re: [GSoC][PATCH v5] submodule: port subcommand 'set-branch' from shell to C","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-06-02T19:45:39Z","receivedAt":"2020-06-02T19:45:53Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Tue, Jun 2, 2020 at 9:01 PM Kaartic Sivaraam\n<kaartic.sivaraam@gmail.com> wrote:\n>\n> On 02-06-2020 22:05, Shourya Shukla wrote:\n> > Convert submodule subcommand 'set-branch' to a builtin and call it via\n> > 'git-submodule.sh'.\n> >\n> > Mentored-by: Christian Couder <chriscool@tuxfamily.org>\n> > Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>\n> > Helped-by: Denton Liu <liu.denton@gmail.com>\n> > Helped-by: Eric Sunshine <sunshine@sunshineco.com>\n> > Helped-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>\n> > Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>\n> > ---\n> > Here is the v5 of the subcommand. Thank you Danh for the feedback! I\n> > apologise for not replying on time. I have taken into account Danh's\n> > suggestions on the `quiet` option as well as done the fixup Dscho\n> > suggested (fixed by Junio here:\n> > https://github.com/gitster/git/commit/77ba62f66ff8e3de54d81c240542edb42a2711c7)\n> >\n> >   builtin/submodule--helper.c | 44 +++++++++++++++++++++++++++++++++++++\n> >   git-submodule.sh            | 32 +++------------------------\n> >   2 files changed, 47 insertions(+), 29 deletions(-)\n> >\n> > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> > index f50745a03f..a974e17571 100644\n> > --- a/builtin/submodule--helper.c\n> > +++ b/builtin/submodule--helper.c\n> > @@ -2284,6 +2284,49 @@ static int module_set_url(int argc, const char **argv, const char *prefix)\n> >       return 0;\n> >   }\n> >\n> > +static int module_set_branch(int argc, const char **argv, const char *prefix)\n> > +{\n> > +     int opt_default = 0, ret;\n> > +     const char *opt_branch = NULL;\n> > +     const char *path;\n> > +     char *config_name;\n> > +\n> > +     /*\n> > +      * We accept the `quiet` option for uniformity across subcommands,\n> > +      * though there is nothing to make less verbose in this subcommand.\n> > +      */\n> > +     struct option options[] = {\n> > +             OPT_NOOP_NOARG('q', \"quiet\"),\n> > +             OPT_BOOL('d', \"default\", &opt_default,\n> > +                     N_(\"set the default tracking branch to master\")),\n> > +             OPT_STRING('b', \"branch\", &opt_branch, N_(\"branch\"),\n> > +                     N_(\"set the default tracking branch\")),\n> > +             OPT_END()\n> > +     };\n> > +     const char *const usage[] = {\n> > +             N_(\"git submodule--helper set-branch [-q|--quiet] (-d|--default) <path>\"),\n> > +             N_(\"git submodule--helper set-branch [-q|--quiet] (-b|--branch) <branch> <path>\"),\n> > +             NULL\n> > +     };\n>\n> I'm having second thoughts about my suggestion[1] to include\n> the short option for '--quiet' in the usage. This is the only\n> usage in submodule--helper that mentions that '-q' is a short\n> hand for '--quiet'. That seems inconsistent. I see two ways but\n> I'm not sure which one of these would be better:\n>\n> A. Dropping the mention of '-q' in this usage thus making it consistent\n>     with the other usages printed by submodule--helper.\n>\n> B. Fixing other usages of submodule--helper to mention that '-q' is\n>     shorthand for quiet. This has the benefit of properly advertising\n>     the shorthand.\n>\n> C. Just ignore this?\n\nThe `git submodule` documentation has:\n\n-q::\n--quiet::\n        Only print error messages.\n\neven though the Synopsis is:\n\n'git submodule' [--quiet] [--cached]\n'git submodule' [--quiet] add [<options>] [--] <repository> [<path>]\n'git submodule' [--quiet] status [--cached] [--recursive] [--] [<path>...]\n...\n\nSo I prefer B, and maybe updating the synopsis, as I think most Git\ncommands have '-q' meaning '--quiet'.\n\n[...]\n\n> So, according to this, I think the usage should be ...\n>\n>      git submodule--helper set-branch [-q | --quiet] [-d | --default] <path>\n>\n> ... and ...\n>\n>      git submodule--helper set-branch [-q|--quiet] [-b | --branch]<branch> <path>\n>\n> ... respectively.\n\nI don't agree. I think `git submodule--helper set-branch ...` requires\neither \"-d | --default\" or \"-b | --branch\", while for example:\n\ngit submodule--helper set-branch [-q | --quiet] [-d | --default] <path>\n\nwould mean that \"git submodule--helper set-branch my/path\" is valid.\n"},{"id":"399056","messageId":"20200603001225.GB2222@danh.dev","threadId":"53523","inReplyTo":"xmqqzh9ls622.fsf@gitster.c.googlers.com","subject":"Re: [GSoC][PATCH v5] submodule: port subcommand 'set-branch' from shell to C","fromName":"Đoàn Trần Công Danh","fromEmail":"congdanhqx@gmail.com","sentAt":"2020-06-03T00:12:25Z","receivedAt":"2020-06-03T00:12:29Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"On 2020-06-02 10:58:45-0700, Junio C Hamano <gitster@pobox.com> wrote:\n> Shourya Shukla <shouryashukla.oo@gmail.com> writes:\n> \n> > +\t * though there is nothing to make less verbose in this subcommand.\n> > +\t */\n> > +\tstruct option options[] = {\n> > +\t\tOPT_NOOP_NOARG('q', \"quiet\"),\n> > +\t\tOPT_BOOL('d', \"default\", &opt_default,\n> > +\t\t\tN_(\"set the default tracking branch to master\")),\n> > +\t\tOPT_STRING('b', \"branch\", &opt_branch, N_(\"branch\"),\n> > +\t\t\tN_(\"set the default tracking branch\")),\n> > ...\n> > +\t\tOPT_END()\n> > +\t};\n> > +\tconst char *const usage[] = {\n> > +\t\tN_(\"git submodule--helper set-branch [-q|--quiet] (-d|--default) <path>\"),\n> > +\t\tN_(\"git submodule--helper set-branch [-q|--quiet] (-b|--branch) <branch> <path>\"),\n> \n> \n> I notice that we gained back -d and -b shorthands that was\n> advertised but not implemented the previous rounds.  It is a bit\n> curious that we are adding these short-hands that nobody uses,\n> though.  \n\nI think a day will come, when all git-submodule functionalities will\nrun by calling git-submodule--helper.\n\nIn that day, we will use current git-submodule--helper as the new\ngit-submodule.\n\nTo me, it'll be less noise to just gs/--helper// from this file and use\nit as the new git-submodule, instead of changing the OPT_* all over\nplaces.\n\nOr is that a complain for missing some tests?\n\n-- \nDanh\n"},{"id":"399088","messageId":"xmqqtuzrrk8r.fsf@gitster.c.googlers.com","threadId":"53523","inReplyTo":"20200603001225.GB2222@danh.dev","subject":"Re: [GSoC][PATCH v5] submodule: port subcommand 'set-branch' from shell to C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-03T20:02:12Z","receivedAt":"2020-06-03T20:02:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Đoàn Trần Công Danh  <congdanhqx@gmail.com> writes:\n\n> On 2020-06-02 10:58:45-0700, Junio C Hamano <gitster@pobox.com> wrote:\n>> Shourya Shukla <shouryashukla.oo@gmail.com> writes:\n>> \n>> > +\t * though there is nothing to make less verbose in this subcommand.\n>> > +\t */\n>> > +\tstruct option options[] = {\n>> > +\t\tOPT_NOOP_NOARG('q', \"quiet\"),\n>> > +\t\tOPT_BOOL('d', \"default\", &opt_default,\n>> > +\t\t\tN_(\"set the default tracking branch to master\")),\n>> > +\t\tOPT_STRING('b', \"branch\", &opt_branch, N_(\"branch\"),\n>> > +\t\t\tN_(\"set the default tracking branch\")),\n>> > ...\n>> > +\t\tOPT_END()\n>> > +\t};\n>> > +\tconst char *const usage[] = {\n>> > +\t\tN_(\"git submodule--helper set-branch [-q|--quiet] (-d|--default) <path>\"),\n>> > +\t\tN_(\"git submodule--helper set-branch [-q|--quiet] (-b|--branch) <branch> <path>\"),\n>> \n>> \n>> I notice that we gained back -d and -b shorthands that was\n>> advertised but not implemented the previous rounds.  It is a bit\n>> curious that we are adding these short-hands that nobody uses,\n>> though.  \n>\n> I think a day will come, when all git-submodule functionalities will\n> run by calling git-submodule--helper.\n\nI'd expect that when that day with no scripted parts of \"git\nsubmodule\" remains comes, the main entry point functions in\nbuiltin/submodule--helper.c (like module_list(), update_clone(),\nmodule_set_branch(), etc.) will become helper functions that live in\nsubmodule-lib.c and would be called from builtin/submodule.c.  And\nthe conversion would rip out calls to parse_options() in each of\nthese functions that would migrate to submodule-lib.c\n\n    Side note: instead of adding submodule-lib.c, you could add them\n    directly to submodule.c if they are small enough.  I am however\n    modeling after how the \"diff\" family was converted to C; the\n    diff-lib.c layer is \"library-ish helpers that get pre-parsed\n    command line arguments and performs a single unit of work\" that\n    utilizes service routines at the lower layer that are in diff.c\n    and submodule-lib.c and submodule.c will be in a similar kind of\n    relationship.\n\n> In that day, we will use current git-submodule--helper as the new\n> git-submodule.\n\nNo, I do not think so.  Most of the option parsers would be redone\nin builtin/submodule.c; only some that can be used as-is may migrate\nas a whole to builtin/submodule.c and its parse_options() stuff\nreused, but most of what is in submodule--helper would have to lose\ntheir parse_options() calls, as nobody would be using module_list()\nwhen there is no scripted \"git submodule\" exists, for example.\n\n> Or is that a complain for missing some tests?\n\nNo, it was \"do the minimum necessary for an implementation detail,\nas we'll discard that part later anyway\".\n"},{"id":"399097","messageId":"20200604070930.GB8686@konoha","threadId":"53523","inReplyTo":"CAP8UFD3Qe3iDe+ymKsqv9HarFLYDohXmUGbkNwZ4MdVQ=XP7yQ@mail.gmail.com","subject":"Re: [GSoC][PATCH v5] submodule: port subcommand 'set-branch' from shell to C","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-06-04T07:09:30Z","receivedAt":"2020-06-04T07:09:41Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"On 02/06 09:45, Christian Couder wrote:\n> >\n> > I'm having second thoughts about my suggestion[1] to include\n> > the short option for '--quiet' in the usage. This is the only\n> > usage in submodule--helper that mentions that '-q' is a short\n> > hand for '--quiet'. That seems inconsistent. I see two ways but\n> > I'm not sure which one of these would be better:\n> >\n> > A. Dropping the mention of '-q' in this usage thus making it consistent\n> >     with the other usages printed by submodule--helper.\n> >\n> > B. Fixing other usages of submodule--helper to mention that '-q' is\n> >     shorthand for quiet. This has the benefit of properly advertising\n> >     the shorthand.\n> >\n> > C. Just ignore this?\n> \n> The `git submodule` documentation has:\n> \n> -q::\n> --quiet::\n>         Only print error messages.\n> \n> even though the Synopsis is:\n> \n> 'git submodule' [--quiet] [--cached]\n> 'git submodule' [--quiet] add [<options>] [--] <repository> [<path>]\n> 'git submodule' [--quiet] status [--cached] [--recursive] [--] [<path>...]\n> ...\n> \n> So I prefer B, and maybe updating the synopsis, as I think most Git\n> commands have '-q' meaning '--quiet'.\n\nYep, (B) sounds good! Junio in one of the previous mails stated that\nthis thing will need to be done for all subcommands when the shell\nscript is to be demolished i.e., `git submodule` becomes a builtin\ncompletely. Actually, vrious usages might need to be fixed, for many\nsubcommands because almost none of them have any info about the\nshorthand usages of options in their `options` array.\n\n"},{"id":"399098","messageId":"20200604071719.GC8686@konoha","threadId":"53523","inReplyTo":"xmqqtuzrrk8r.fsf@gitster.c.googlers.com","subject":"Re: [GSoC][PATCH v5] submodule: port subcommand 'set-branch' from shell to C","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-06-04T07:17:19Z","receivedAt":"2020-06-04T07:17:28Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"On 03/06 01:02, Junio C Hamano wrote:\n> I'd expect that when that day with no scripted parts of \"git\n> submodule\" remains comes, the main entry point functions in\n> builtin/submodule--helper.c (like module_list(), update_clone(),\n> module_set_branch(), etc.) will become helper functions that live in\n> submodule-lib.c and would be called from builtin/submodule.c.  And\n> the conversion would rip out calls to parse_options() in each of\n> these functions that would migrate to submodule-lib.c\n> \n>     Side note: instead of adding submodule-lib.c, you could add them\n>     directly to submodule.c if they are small enough.  I am however\n>     modeling after how the \"diff\" family was converted to C; the\n>     diff-lib.c layer is \"library-ish helpers that get pre-parsed\n>     command line arguments and performs a single unit of work\" that\n>     utilizes service routines at the lower layer that are in diff.c\n>     and submodule-lib.c and submodule.c will be in a similar kind of\n>     relationship.\n\nThere does exist a `submodule.c` outside of `builtin/` which has various\nhelper functions. Will that require renaming to `submodule-lib.c`? BTW\n`set-branch` is a subcommand of `git submodule` so do we have to put it\ninto `submodule-lib.c` if there were to be one?\nWhat is the motivation behind modelling it on the diff-family?\n\n"},{"id":"399105","messageId":"CAP8UFD17VRnmtf8LUcXGZ9bqOt72Fww8B4Jp0yf6t0PAT3Q=bw@mail.gmail.com","threadId":"53523","inReplyTo":"20200604071719.GC8686@konoha","subject":"Re: [GSoC][PATCH v5] submodule: port subcommand 'set-branch' from shell to C","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-06-04T07:49:59Z","receivedAt":"2020-06-04T07:50:13Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thu, Jun 4, 2020 at 9:17 AM Shourya Shukla\n<shouryashukla.oo@gmail.com> wrote:\n>\n> On 03/06 01:02, Junio C Hamano wrote:\n> > I'd expect that when that day with no scripted parts of \"git\n> > submodule\" remains comes, the main entry point functions in\n> > builtin/submodule--helper.c (like module_list(), update_clone(),\n> > module_set_branch(), etc.) will become helper functions that live in\n> > submodule-lib.c and would be called from builtin/submodule.c.  And\n> > the conversion would rip out calls to parse_options() in each of\n> > these functions that would migrate to submodule-lib.c\n> >\n> >     Side note: instead of adding submodule-lib.c, you could add them\n> >     directly to submodule.c if they are small enough.  I am however\n> >     modeling after how the \"diff\" family was converted to C; the\n> >     diff-lib.c layer is \"library-ish helpers that get pre-parsed\n> >     command line arguments and performs a single unit of work\" that\n> >     utilizes service routines at the lower layer that are in diff.c\n> >     and submodule-lib.c and submodule.c will be in a similar kind of\n> >     relationship.\n>\n> There does exist a `submodule.c` outside of `builtin/` which has various\n> helper functions. Will that require renaming to `submodule-lib.c`?\n\nNo, as Junio says that \"submodule-lib.c and submodule.c will be in a\nsimilar kind of relationship\" as diff.c and diff-lib.c.\n\n> BTW\n> `set-branch` is a subcommand of `git submodule` so do we have to put it\n> into `submodule-lib.c` if there were to be one?\n> What is the motivation behind modelling it on the diff-family?\n\nMaybe to separate helper functions for submodules from other submodule\nfunctions.\n"},{"id":"399118","messageId":"xmqqd06erhyy.fsf@gitster.c.googlers.com","threadId":"53523","inReplyTo":"20200604071719.GC8686@konoha","subject":"Re: [GSoC][PATCH v5] submodule: port subcommand 'set-branch' from shell to C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-04T15:03:33Z","receivedAt":"2020-06-04T15:03:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shourya Shukla <shouryashukla.oo@gmail.com> writes:\n\n> On 03/06 01:02, Junio C Hamano wrote:\n>> I'd expect that when that day with no scripted parts of \"git\n>> submodule\" remains comes, the main entry point functions in\n>> builtin/submodule--helper.c (like module_list(), update_clone(),\n>> module_set_branch(), etc.) will become helper functions that live in\n>> submodule-lib.c and would be called from builtin/submodule.c.  And\n>> the conversion would rip out calls to parse_options() in each of\n>> these functions that would migrate to submodule-lib.c\n>> \n>>     Side note: instead of adding submodule-lib.c, you could add them\n>>     directly to submodule.c if they are small enough.  I am however\n>>     modeling after how the \"diff\" family was converted to C; the\n>>     diff-lib.c layer is \"library-ish helpers that get pre-parsed\n>>     command line arguments and performs a single unit of work\" that\n>>     utilizes service routines at the lower layer that are in diff.c\n>>     and submodule-lib.c and submodule.c will be in a similar kind of\n>>     relationship.\n>\n> There does exist a `submodule.c` outside of `builtin/` which has various\n> helper functions. Will that require renaming to `submodule-lib.c`?\n\nNo, that is different from what I wrote above.  Just like there is\nthe middle-layer diff-lib.c between the top-layer builtin/diff.c and\nthe low-level helper sets in diff.c, I envision that between the\ntop-layer builtin/submodule.c and the low-level helper sets in\nsubmodule.c, there would be the middle layer submodule-lib.c.\n\nIf a single cmd_submodule_set_url() function implements the whole of\n\"git submoduel set-url\" (by calling helper routines in submodule.c\nand those currently in builtin/submodule--helper.c), I would expect\nit to reside in builtin/submodule.c.\n\n\n"},{"id":"399147","messageId":"92bad281-dd38-aef2-9910-659b41cdd830@gmail.com","threadId":"53523","inReplyTo":"CAP8UFD3Qe3iDe+ymKsqv9HarFLYDohXmUGbkNwZ4MdVQ=XP7yQ@mail.gmail.com","subject":"Re: [GSoC][PATCH v5] submodule: port subcommand 'set-branch' from shell to C","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2020-06-04T19:26:20Z","receivedAt":"2020-06-04T19:26:35Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"On 03-06-2020 01:15, Christian Couder wrote:\n> On Tue, Jun 2, 2020 at 9:01 PM Kaartic Sivaraam\n> <kaartic.sivaraam@gmail.com> wrote:\n>>\n>> I'm having second thoughts about my suggestion[1] to include\n>> the short option for '--quiet' in the usage. This is the only\n>> usage in submodule--helper that mentions that '-q' is a short\n>> hand for '--quiet'. That seems inconsistent. I see two ways but\n>> I'm not sure which one of these would be better:\n>>\n>> A. Dropping the mention of '-q' in this usage thus making it consistent\n>>     with the other usages printed by submodule--helper.\n>>\n>> B. Fixing other usages of submodule--helper to mention that '-q' is\n>>     shorthand for quiet. This has the benefit of properly advertising\n>>     the shorthand.\n>>\n>> C. Just ignore this?\n> \n> The `git submodule` documentation has:\n> \n> -q::\n> --quiet::\n>         Only print error messages.\n> \n> even though the Synopsis is:\n> \n> 'git submodule' [--quiet] [--cached]\n> 'git submodule' [--quiet] add [<options>] [--] <repository> [<path>]\n> 'git submodule' [--quiet] status [--cached] [--recursive] [--] [<path>...]\n> ...\n> \n> So I prefer B, and maybe updating the synopsis, as I think most Git\n> commands have '-q' meaning '--quiet'.\n> \n\nMakes sense.\n\n> [...]\n> \n>> So, according to this, I think the usage should be ...\n>>\n>>      git submodule--helper set-branch [-q | --quiet] [-d | --default] <path>\n>>\n>> ... and ...\n>>\n>>      git submodule--helper set-branch [-q|--quiet] [-b | --branch]<branch> <path>\n>>\n>> ... respectively.\n> \n> I don't agree. I think `git submodule--helper set-branch ...` requires\n> either \"-d | --default\" or \"-b | --branch\", while for example:\n> \n> git submodule--helper set-branch [-q | --quiet] [-d | --default] <path>\n> \n> would mean that \"git submodule--helper set-branch my/path\" is valid.\n> \n\nYou're right. Even I thought about the same thing when I came up with\nthat suggestion after quoting that portion of the CodingGuidelines. But\nit was also curious for me to observe that the original used parenthesis\nto mention the short and long options of an argument:\n\n> +\tconst char *const usage[] = {\n> +\t\tN_(\"git submodule--helper set-branch [-q|--quiet] (-d|--default) <path>\"),\n> +\t\tN_(\"git submodule--helper set-branch [-q|--quiet] (-b|--branch) <branch> <path>\"),\n> +\t\tNULL\n> +\t};\n\nI've not seen such a usage before. That's what actually made me take a\nlook at the CodingGuidelines for this. As the CodingGuidelined doesn't\nseem to be mentioning anything about this explicitly, let's see if I\ncould find something in the usage printed by other commands.\n\n---\n> git am -h\nusage: git am [<options>] [(<mbox> | <Maildir>)...]\n   or: git am [<options>] (--continue | --skip | --abort)\n\n   <options snipped>\n\n> git branch -h\nusage: git branch [<options>] [-r | -a] [--merged | --no-merged]\n   or: git branch [<options>] [-l] [-f] <branch-name> [<start-point>]\n   or: git branch [<options>] [-r] (-d | -D) <branch-name>...\n   or: git branch [<options>] (-m | -M) [<old-branch>] <new-branch>\n   or: git branch [<options>] (-c | -C) [<old-branch>] <new-branch>\n   or: git branch [<options>] [-r | -a] [--points-at]\n   or: git branch [<options>] [-r | -a] [--format]\n\n   <options snipped>\n\n> git checkout -h\nusage: git checkout [<options>] <branch>\n   or: git checkout [<options>] [<branch>] -- <file>...\n\n   <options snipped>\n\n> git switch -h\nusage: git switch [<options>] [<branch>]\n\n   <options snipped>\n\n---\nHmm. Looks like it's not common for us to mention both the short\nand long options in the usage itself. This might be to avoid the\nredundancy as the usage is usually followed by the list of options.\n\nWith this info, I think we could've just gone with the following as the\nusage strings for the `set-branch` subcommand:\n\n    git submodule--helper set-branch [<options>] -d <path>\n    git submodule--helper set-branch [<options>] -b <branch> <path>\n\nThis also solves the problem with `--quiet` I mentioned earlier while\nmaking it concise and inline with the usages printed by other commands.\n\nAll this said, I don't think it's worth a re-roll now for several\nreasons.\n\n-- \nSivaraam\n"}]}