{"thread":{"id":"53408","subject":"[PATCH v4] submodule: port subcommand 'set-url' from shell to C","startedAt":"2020-05-06T07:37:29Z","lastAt":"2020-05-08T17:51:31Z","messageCount":22,"participants":["Shourya Shukla","Christian Couder","Junio C Hamano","Denton Liu","Eric Sunshine"],"isPatch":true,"patchVersion":4,"patchTotal":null},"messages":[{"id":"397169","messageId":"20200506073717.9789-1-shouryashukla.oo@gmail.com","threadId":"53408","inReplyTo":null,"subject":"[PATCH v4] submodule: port subcommand 'set-url' from shell to C","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-05-06T07:37:17Z","receivedAt":"2020-05-06T07:37:29Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"Convert submodule subcommand 'set-url' to a builtin. Port 'set-url'to\n'submodule--helper.c' and call the latter via 'git-submodule.sh'.\n\nSigned-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>\n---\nThank you Junio for the review! :)\nBTW, how detailed should the commit message be about the\npatch?\n\n builtin/submodule--helper.c | 39 +++++++++++++++++++++++++++++++++++++\n git-submodule.sh            | 22 +--------------------\n 2 files changed, 40 insertions(+), 21 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 1a4b391c88..f50745a03f 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2246,6 +2246,44 @@ static int module_config(int argc, const char **argv, const char *prefix)\n \tusage_with_options(git_submodule_helper_usage, module_config_options);\n }\n \n+static int module_set_url(int argc, const char **argv, const char *prefix)\n+{\n+\tint quiet = 0;\n+\tconst char *newurl;\n+\tconst char *path;\n+\tstruct strbuf config_name = STRBUF_INIT;\n+\n+\tstruct option set_url_options[] = {\n+\t\tOPT__QUIET(&quiet, N_(\"Suppress output for setting url of a submodule\")),\n+\t\tOPT_END()\n+\t};\n+\n+\tconst char *const usage[] = {\n+\t\tN_(\"git submodule--helper set-url [--quiet] <path> <newurl>\"),\n+\t\tNULL\n+\t};\n+\n+\targc = parse_options(argc, argv, prefix, set_url_options,\n+\t\t\t     usage, 0);\n+\n+\tif (argc!=2) {\n+\t\tusage_with_options(usage, set_url_options);\n+\t\treturn 1;\n+\t}\n+\n+\tpath = argv[0];\n+\tnewurl = argv[1];\n+\n+\tstrbuf_addf(&config_name, \"submodule.%s.url\", path);\n+\n+\tconfig_set_in_gitmodules_file_gently(config_name.buf, newurl);\n+\tsync_submodule(path, prefix, quiet ? OPT_QUIET : 0);\n+\n+\tstrbuf_release(&config_name);\n+\n+\treturn 0;\n+}\n+\n #define SUPPORT_SUPER_PREFIX (1<<0)\n \n struct cmd_struct {\n@@ -2276,6 +2314,7 @@ static struct cmd_struct commands[] = {\n \t{\"is-active\", is_active, 0},\n \t{\"check-name\", check_name, 0},\n \t{\"config\", module_config, 0},\n+\t{\"set-url\", module_set_url, 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 08e0439df0..39ebdf25b5 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -805,27 +805,7 @@ cmd_set_url() {\n \t\tshift\n \tdone\n \n-\tif test $# -ne 2\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-\turl=\"$2\"\n-\tif test -z \"$url\"\n-\tthen\n-\t\texit 1\n-\tfi\n-\n-\tgit submodule--helper config submodule.\"$name\".url \"$url\"\n-\tgit submodule--helper sync ${GIT_QUIET:+--quiet} \"$name\"\n+\tgit ${wt_prefix:+-C \"$wt_prefix\"} ${prefix:+--super-prefix \"$prefix\"} submodule--helper set-url ${GIT_QUIET:+--quiet} -- \"$@\"\n }\n \n #\n-- \n2.26.2\n\n"},{"id":"397174","messageId":"CAP8UFD0o7WwibV8+cwYOO949BkBggSphi0zbgPUZsk6nfvYyHQ@mail.gmail.com","threadId":"53408","inReplyTo":"20200506073717.9789-1-shouryashukla.oo@gmail.com","subject":"Re: [PATCH v4] submodule: port subcommand 'set-url' from shell to C","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-05-06T08:09:31Z","receivedAt":"2020-05-06T08:09:46Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, May 6, 2020 at 9:37 AM Shourya Shukla\n<shouryashukla.oo@gmail.com> wrote:\n>\n> Convert submodule subcommand 'set-url' to a builtin. Port 'set-url'to\n\nThere is a space missing between \"'set-url'\" and \"to\".\n\n> 'submodule--helper.c' and call the latter via 'git-submodule.sh'.\n>\n> Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>\n> ---\n> Thank you Junio for the review! :)\n> BTW, how detailed should the commit message be about the\n> patch?\n\nIt looks good to me. Maybe it could more explicitely state that the\nlarger goal is to convert shell code in 'git-submodule.sh' to C code\nin 'submodule--helper.c'. It can be guessed from the subject though.\n\n>  builtin/submodule--helper.c | 39 +++++++++++++++++++++++++++++++++++++\n>  git-submodule.sh            | 22 +--------------------\n>  2 files changed, 40 insertions(+), 21 deletions(-)\n>\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index 1a4b391c88..f50745a03f 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -2246,6 +2246,44 @@ static int module_config(int argc, const char **argv, const char *prefix)\n>         usage_with_options(git_submodule_helper_usage, module_config_options);\n>  }\n>\n> +static int module_set_url(int argc, const char **argv, const char *prefix)\n> +{\n> +       int quiet = 0;\n> +       const char *newurl;\n> +       const char *path;\n> +       struct strbuf config_name = STRBUF_INIT;\n> +\n> +       struct option set_url_options[] = {\n> +               OPT__QUIET(&quiet, N_(\"Suppress output for setting url of a submodule\")),\n> +               OPT_END()\n> +       };\n> +\n> +       const char *const usage[] = {\n> +               N_(\"git submodule--helper set-url [--quiet] <path> <newurl>\"),\n> +               NULL\n> +       };\n> +\n> +       argc = parse_options(argc, argv, prefix, set_url_options,\n> +                            usage, 0);\n> +\n> +       if (argc!=2) {\n\nPlease add space chars around \"!=\" like \"argc != 2\".\n\n> +               usage_with_options(usage, set_url_options);\n> +               return 1;\n> +       }\n> +\n> +       path = argv[0];\n> +       newurl = argv[1];\n> +\n> +       strbuf_addf(&config_name, \"submodule.%s.url\", path);\n> +\n> +       config_set_in_gitmodules_file_gently(config_name.buf, newurl);\n> +       sync_submodule(path, prefix, quiet ? OPT_QUIET : 0);\n> +\n> +       strbuf_release(&config_name);\n\nNit: it might be a bit simpler to define config_name as a \"char *\",\nand then use xstrfmt() and free() instead of strbuf_addf() and\nstrbuf_release().\n\n> +       return 0;\n> +}\n"},{"id":"397202","messageId":"20200506163128.GA14899@konoha","threadId":"53408","inReplyTo":"CAP8UFD0o7WwibV8+cwYOO949BkBggSphi0zbgPUZsk6nfvYyHQ@mail.gmail.com","subject":"Re: [PATCH v4] submodule: port subcommand 'set-url' from shell to C","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-05-06T16:31:28Z","receivedAt":"2020-05-06T16:31:37Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"On 06/05 10:09, Christian Couder wrote:\n> > +       strbuf_addf(&config_name, \"submodule.%s.url\", path);\n> > +\n> > +       config_set_in_gitmodules_file_gently(config_name.buf, newurl);\n> > +       sync_submodule(path, prefix, quiet ? OPT_QUIET : 0);\n> > +\n> > +       strbuf_release(&config_name);\n> \n> Nit: it might be a bit simpler to define config_name as a \"char *\",\n> and then use xstrfmt() and free() instead of strbuf_addf() and\n> strbuf_release().\n\nApart from the simplicity purposes, does doing this aid in performance\nin any way?\n\n> > +       return 0;\n> > +}\n"},{"id":"397210","messageId":"xmqqtv0t6l84.fsf@gitster.c.googlers.com","threadId":"53408","inReplyTo":"20200506073717.9789-1-shouryashukla.oo@gmail.com","subject":"Re: [PATCH v4] submodule: port subcommand 'set-url' from shell to C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-06T17:12:43Z","receivedAt":"2020-05-06T17:12:53Z","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-url' to a builtin. Port 'set-url'to\n> 'submodule--helper.c' and call the latter via 'git-submodule.sh'.\n>\n> Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>\n> ---\n> Thank you Junio for the review! :)\n> BTW, how detailed should the commit message be about the\n> patch?\n>\n>  builtin/submodule--helper.c | 39 +++++++++++++++++++++++++++++++++++++\n>  git-submodule.sh            | 22 +--------------------\n>  2 files changed, 40 insertions(+), 21 deletions(-)\n>\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index 1a4b391c88..f50745a03f 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -2246,6 +2246,44 @@ static int module_config(int argc, const char **argv, const char *prefix)\n>  \tusage_with_options(git_submodule_helper_usage, module_config_options);\n>  }\n>  \n> +static int module_set_url(int argc, const char **argv, const char *prefix)\n> +{\n> +\tint quiet = 0;\n> +\tconst char *newurl;\n> +\tconst char *path;\n> +\tstruct strbuf config_name = STRBUF_INIT;\n> +\n> +\tstruct option set_url_options[] = {\n> +\t\tOPT__QUIET(&quiet, N_(\"Suppress output for setting url of a submodule\")),\n> +\t\tOPT_END()\n> +\t};\n> +\n> +\tconst char *const usage[] = {\n> +\t\tN_(\"git submodule--helper set-url [--quiet] <path> <newurl>\"),\n> +\t\tNULL\n> +\t};\n\nHmph, do we really want all the blank lines in the above?\n\nThere is only one \"struct option\" the code in this function needs to\nbe aware of and worried about.  Isn't naming it set_url_options[]\noverly redundant?  Calling it just options[] would save lines here ;-)\n\n> +\targc = parse_options(argc, argv, prefix, set_url_options,\n> +\t\t\t     usage, 0);\n\n\targc = parse_options(argc, argv, prefix, options, usage, 0);\n\n> +\tif (argc!=2) {\n\nStyle.  SP around all binary operators like !=, i.e.\n\n\tif (argc != 2) {\n\nBy the way, looking at print_default_remote() that takes no\narguments wants argc to be 1, and resolve_relative_url() that takes\nonly one or two arguments checks for 2 or 3, shouldn't this be\nchecking if argc is 3, not 2?\n\nI thought I pointed it out in my very first review of this series.\n\n\t... tries to go back and check, notices that this v4 is not\n        ... a reply to v3 or earlier and feels somewhat irritated.\n\t... then finally finds the following in the v2 review.\n\n> Taking all these together,\n> \n>         if (argc != 3) {\n>                 usage_with_options(usage, options);\n>                 return 1;\n>         }\n>         path = argv[0];\n>         newurl = argv[1];\n> \n> If you feel paranoid, you can check these two are not NULL, too,\n> i.e.\n> \n>         if (argc != 3 || !(path = argv[0]) || !(newurl = argv[1])) {\n>                 usage_with_options(usage, options);\n>                 return 1;\n>         }\n> \n> I have no strong preference either way.  Perhaps the latter is more\n> concise and more careful at the same time, so some people may prefer\n> it.\n\nThanks.\n"},{"id":"397211","messageId":"xmqqpnbh6l10.fsf@gitster.c.googlers.com","threadId":"53408","inReplyTo":"20200506163128.GA14899@konoha","subject":"Re: [PATCH v4] submodule: port subcommand 'set-url' from shell to C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-06T17:16:59Z","receivedAt":"2020-05-06T17:17:07Z","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 06/05 10:09, Christian Couder wrote:\n>> > +       strbuf_addf(&config_name, \"submodule.%s.url\", path);\n>> > +\n>> > +       config_set_in_gitmodules_file_gently(config_name.buf, newurl);\n>> > +       sync_submodule(path, prefix, quiet ? OPT_QUIET : 0);\n>> > +\n>> > +       strbuf_release(&config_name);\n>> \n>> Nit: it might be a bit simpler to define config_name as a \"char *\",\n>> and then use xstrfmt() and free() instead of strbuf_addf() and\n>> strbuf_release().\n>\n> Apart from the simplicity purposes, does doing this aid in performance\n> in any way?\n\nstrbuf.c::xstrfmt() uses strbuf.c::xstrvfmt() that formats into a\ntemporary strbuf and returns the detached buffer as the result.\n\nCompare it with what strbuf.c::strbuf_addf() and you can draw a\nconclusion on your own ;-)\n\n\n"},{"id":"397223","messageId":"20200506181239.GA5683@konoha","threadId":"53408","inReplyTo":"xmqqtv0t6l84.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v4] submodule: port subcommand 'set-url' from shell to C","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-05-06T18:12:39Z","receivedAt":"2020-05-06T18:12:48Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"On 06/05 10:12, Junio C Hamano wrote: \n> > +static int module_set_url(int argc, const char **argv, const char *prefix)\n> > +{\n> > +\tint quiet = 0;\n> > +\tconst char *newurl;\n> > +\tconst char *path;\n> > +\tstruct strbuf config_name = STRBUF_INIT;\n> > +\n> > +\tstruct option set_url_options[] = {\n> > +\t\tOPT__QUIET(&quiet, N_(\"Suppress output for setting url of a submodule\")),\n> > +\t\tOPT_END()\n> > +\t};\n> > +\n> > +\tconst char *const usage[] = {\n> > +\t\tN_(\"git submodule--helper set-url [--quiet] <path> <newurl>\"),\n> > +\t\tNULL\n> > +\t};\n> \n> Hmph, do we really want all the blank lines in the above?\n\nApologies,will amend.\n\n> There is only one \"struct option\" the code in this function needs to\n> be aware of and worried about.  Isn't naming it set_url_options[]\n> overly redundant?  Calling it just options[] would save lines here ;-)\n\nI was actually following the format of the other subcommands, will\nsurely change it.\n\n> > +\targc = parse_options(argc, argv, prefix, set_url_options,\n> > +\t\t\t     usage, 0);\n> \n> \targc = parse_options(argc, argv, prefix, options, usage, 0);\n> \n> > +\tif (argc!=2) {\n> \n> Style.  SP around all binary operators like !=, i.e.\n> \n> \tif (argc != 2) {\n> \n> By the way, looking at print_default_remote() that takes no\n> arguments wants argc to be 1, and resolve_relative_url() that takes\n> only one or two arguments checks for 2 or 3, shouldn't this be\n> checking if argc is 3, not 2?\n\nAren't `path` and `newurl` the only arguments we should worry about\nhere as 'parse_options' will parse out the other arguments ('git\nsubmodule--helper' and the 'quiet' option) leaving us with only the\naforementioned arguments. Am I missing something here?\n\nTo add on, checking for `argc!=3` results in a failure of t7420.\nIf we have anything but 2 arguments (either less or more) we should have\na failure.\n\nI think that we will do a check for 3 if we pass the macro\n`PARSE_OPT_KEEP_ARGV0` in `parse_options()`. So the final code segment\nwould look like:\n\t\n\targc = parse_options(argc, argv, prefix, options,\n\t\t\t     usage, PARSE_OPT_KEEP_ARGV0);\n\n\tif (argc != 3) {\n\t\tusage_with_options(usage, options);\n\t\treturn 1;\n\t}\n\n\tpath = argv[1];\n\tnewurl = argv[2];\n\nwhich does pass t7420. Therefore a stricter check could be:\n\t\n\targc = parse_options(argc, argv, prefix, options,\n\t\t\t     usage, 0);\n\n\tpath = argv[0];\n\tnewurl = argv[1];\n\n\tif (argc != 2 || path == NULL || newurl == NULL) {\n\t\tusage_with_options(usage, options);\n\t\treturn 1;\n\t}\nwhich passes t7420.\n\n> I thought I pointed it out in my very first review of this series.\n> \n> \t... tries to go back and check, notices that this v4 is not\n>         ... a reply to v3 or earlier and feels somewhat irritated.\n> \t... then finally finds the following in the v2 review.\n\nI am very very sorry for this. I undestand how this must feel. Will\nensure this from the next version. :)\n"},{"id":"397226","messageId":"xmqqwo5o6hzp.fsf@gitster.c.googlers.com","threadId":"53408","inReplyTo":"20200506181239.GA5683@konoha","subject":"Re: [PATCH v4] submodule: port subcommand 'set-url' from shell to C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-06T18:22:34Z","receivedAt":"2020-05-06T18:22:42Z","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>> By the way, looking at print_default_remote() that takes no\n>> arguments wants argc to be 1, and resolve_relative_url() that takes\n>> only one or two arguments checks for 2 or 3, shouldn't this be\n>> checking if argc is 3, not 2?\n>\n> Aren't `path` and `newurl` the only arguments we should worry about\n> here as 'parse_options' will parse out the other arguments ('git\n> submodule--helper' and the 'quiet' option) leaving us with only the\n> aforementioned arguments. Am I missing something here?\n\nAh, I misread those examples that suggested that you are supposed to\ncheck for N+1 when you expect N arguments.   They are *not* using\nparse_options() and that is where that funny numbering comes from.\n\nThis one uses \"argc = parse_options(...)\" so we should check for N\nwhen we want N args.  Thanks.\n"},{"id":"397258","messageId":"20200507044028.GA5168@konoha","threadId":"53408","inReplyTo":"xmqqwo5o6hzp.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v4] submodule: port subcommand 'set-url' from shell to C","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-05-07T04:40:28Z","receivedAt":"2020-05-07T04:40:37Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"On 06/05 11:22, Junio C Hamano wrote: \n> Ah, I misread those examples that suggested that you are supposed to\n> check for N+1 when you expect N arguments.   They are *not* using\n> parse_options() and that is where that funny numbering comes from.\n> \n> This one uses \"argc = parse_options(...)\" so we should check for N\n> when we want N args.  Thanks.\n\nNo worries. BTW, should I include the `path == NULL` check in the\nif-statement? I think the `argc` check would suffice but I would still\nlove to hear a final verdict from you and Christian :)\n"},{"id":"397260","messageId":"xmqqv9l849i4.fsf@gitster.c.googlers.com","threadId":"53408","inReplyTo":"20200507044028.GA5168@konoha","subject":"Re: [PATCH v4] submodule: port subcommand 'set-url' from shell to C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-07T05:08:51Z","receivedAt":"2020-05-07T05:08:58Z","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> No worries. BTW, should I include the `path == NULL` check in the\n> if-statement?\n\nIf I were writing this code, I would probably write it like so:\n\n\tif (!path || !newurl)\n\t\toops;\n\nSpecifically, I would write \"!path\", not \"path == NULL\".  I thought\na rule for that is in the CodingGuidelines (I didn't double check,\nthough).\n\nThe comparison on argc is to see if we are even allowed to access\nargv[0] and/or argv[1].  In practice, if what main() got from the\noutside world in argv[] is passed directly to you, argv[n] would\nnever be NULL as long as n < argc, but there are a few levels of\ncallchain between main() and you (i.e. module_set_url()), so not\ncounting on that would be sensible.\n"},{"id":"397390","messageId":"20200508054728.GA8615@konoha","threadId":"53408","inReplyTo":"xmqqv9l849i4.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v4] submodule: port subcommand 'set-url' from shell to C","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-05-08T05:47:28Z","receivedAt":"2020-05-08T05:47:38Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"On 06/05 10:08, Junio C Hamano wrote:\n> Shourya Shukla <shouryashukla.oo@gmail.com> writes:\n> \n> > No worries. BTW, should I include the `path == NULL` check in the\n> > if-statement?\n> \n> If I were writing this code, I would probably write it like so:\n> \n> \tif (!path || !newurl)\n> \t\toops;\n> \n> Specifically, I would write \"!path\", not \"path == NULL\".  I thought\n> a rule for that is in the CodingGuidelines (I didn't double check,\n> though).\n\nI could not find a rule like that in the CodingGuidelines.\nShould I add it?\nhttps://github.com/git/git/blob/master/Documentation/CodingGuidelines\n\n> The comparison on argc is to see if we are even allowed to access\n> argv[0] and/or argv[1].  In practice, if what main() got from the\n> outside world in argv[] is passed directly to you, argv[n] would\n> never be NULL as long as n < argc, but there are a few levels of\n> callchain between main() and you (i.e. module_set_url()), so not\n> counting on that would be sensible.\n\nUnderstood. I will add the NULL check as well.\n"},{"id":"397391","messageId":"CAP8UFD0=_8D8hkT5VVPV_F++dr131bkjby357fA+QfhQxktcMg@mail.gmail.com","threadId":"53408","inReplyTo":"20200508054728.GA8615@konoha","subject":"Re: [PATCH v4] submodule: port subcommand 'set-url' from shell to C","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-05-08T06:18:57Z","receivedAt":"2020-05-08T06:19:21Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, May 8, 2020 at 7:51 AM Shourya Shukla\n<shouryashukla.oo@gmail.com> wrote:\n>\n> On 06/05 10:08, Junio C Hamano wrote:\n\n> > Specifically, I would write \"!path\", not \"path == NULL\".  I thought\n> > a rule for that is in the CodingGuidelines (I didn't double check,\n> > though).\n>\n> I could not find a rule like that in the CodingGuidelines.\n> Should I add it?\n> https://github.com/git/git/blob/master/Documentation/CodingGuidelines\n\nSure.\n\n> > The comparison on argc is to see if we are even allowed to access\n> > argv[0] and/or argv[1].  In practice, if what main() got from the\n> > outside world in argv[] is passed directly to you, argv[n] would\n> > never be NULL as long as n < argc, but there are a few levels of\n> > callchain between main() and you (i.e. module_set_url()), so not\n> > counting on that would be sensible.\n>\n> Understood. I will add the NULL check as well.\n\nThanks,\nChristian.\n"},{"id":"397392","messageId":"20200508062136.15257-1-shouryashukla.oo@gmail.com","threadId":"53408","inReplyTo":"20200506073717.9789-1-shouryashukla.oo@gmail.com","subject":"[PATCH v5] submodule: port subcommand 'set-url' from shell to C","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-05-08T06:21:36Z","receivedAt":"2020-05-08T06:21:51Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"Convert submodule subcommand 'set-url' to a builtin. Port 'set-url' to\n'submodule--helper.c' and call the latter via 'git-submodule.sh'.\n\nSigned-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>\n---\n builtin/submodule--helper.c | 37 +++++++++++++++++++++++++++++++++++++\n git-submodule.sh            | 22 +---------------------\n 2 files changed, 38 insertions(+), 21 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 1a4b391c88..8bc7b4cfa6 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2246,6 +2246,42 @@ static int module_config(int argc, const char **argv, const char *prefix)\n \tusage_with_options(git_submodule_helper_usage, module_config_options);\n }\n \n+static int module_set_url(int argc, const char **argv, const char *prefix)\n+{\n+\tint quiet = 0;\n+\tconst char *newurl;\n+\tconst char *path;\n+\tchar* config_name;\n+\n+\tstruct option options[] = {\n+\t\tOPT__QUIET(&quiet, N_(\"Suppress output for setting url of a submodule\")),\n+\t\tOPT_END()\n+\t};\n+\tconst char *const usage[] = {\n+\t\tN_(\"git submodule--helper set-url [--quiet] <path> <newurl>\"),\n+\t\tNULL\n+\t};\n+\n+\targc = parse_options(argc, argv, prefix, options, usage, 0);\n+\n+\tpath = argv[0];\n+\tnewurl = argv[1];\n+\n+\tif (argc != 2 || !path || !newurl) {\n+\t\tusage_with_options(usage, options);\n+\t\treturn 1;\n+\t}\n+\n+\tconfig_name = xstrfmt(\"submodule.%s.url\", path);\n+\n+\tconfig_set_in_gitmodules_file_gently(config_name, newurl);\n+\tsync_submodule(path, prefix, quiet ? OPT_QUIET : 0);\n+\n+\tfree(config_name);\n+\n+\treturn 0;\n+}\n+\n #define SUPPORT_SUPER_PREFIX (1<<0)\n \n struct cmd_struct {\n@@ -2276,6 +2312,7 @@ static struct cmd_struct commands[] = {\n \t{\"is-active\", is_active, 0},\n \t{\"check-name\", check_name, 0},\n \t{\"config\", module_config, 0},\n+\t{\"set-url\", module_set_url, 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 08e0439df0..39ebdf25b5 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -805,27 +805,7 @@ cmd_set_url() {\n \t\tshift\n \tdone\n \n-\tif test $# -ne 2\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-\turl=\"$2\"\n-\tif test -z \"$url\"\n-\tthen\n-\t\texit 1\n-\tfi\n-\n-\tgit submodule--helper config submodule.\"$name\".url \"$url\"\n-\tgit submodule--helper sync ${GIT_QUIET:+--quiet} \"$name\"\n+\tgit ${wt_prefix:+-C \"$wt_prefix\"} ${prefix:+--super-prefix \"$prefix\"} submodule--helper set-url ${GIT_QUIET:+--quiet} -- \"$@\"\n }\n \n #\n-- \n2.26.2\n\n"},{"id":"397393","messageId":"20200508063022.GA18557@generichostname","threadId":"53408","inReplyTo":"20200508062136.15257-1-shouryashukla.oo@gmail.com","subject":"Re: [PATCH v5] submodule: port subcommand 'set-url' from shell to C","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-05-08T06:30:22Z","receivedAt":"2020-05-08T06:30:27Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Shourya,\n\nIt looks good to me except for one tiny nit:\n\nOn Fri, May 08, 2020 at 11:51:36AM +0530, Shourya Shukla wrote:\n> Convert submodule subcommand 'set-url' to a builtin. Port 'set-url' to\n> 'submodule--helper.c' and call the latter via 'git-submodule.sh'.\n> \n> Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>\n> ---\n>  builtin/submodule--helper.c | 37 +++++++++++++++++++++++++++++++++++++\n>  git-submodule.sh            | 22 +---------------------\n>  2 files changed, 38 insertions(+), 21 deletions(-)\n> \n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index 1a4b391c88..8bc7b4cfa6 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -2246,6 +2246,42 @@ static int module_config(int argc, const char **argv, const char *prefix)\n>  \tusage_with_options(git_submodule_helper_usage, module_config_options);\n>  }\n>  \n> +static int module_set_url(int argc, const char **argv, const char *prefix)\n> +{\n> +\tint quiet = 0;\n> +\tconst char *newurl;\n> +\tconst char *path;\n> +\tchar* config_name;\n\nThe asterisk should be stuck with the name, not the type, similar to how\nyou wrote it above.\n\n> +\n> +\tstruct option options[] = {\n> +\t\tOPT__QUIET(&quiet, N_(\"Suppress output for setting url of a submodule\")),\n> +\t\tOPT_END()\n> +\t};\n> +\tconst char *const usage[] = {\n> +\t\tN_(\"git submodule--helper set-url [--quiet] <path> <newurl>\"),\n> +\t\tNULL\n> +\t};\n> +\n> +\targc = parse_options(argc, argv, prefix, options, usage, 0);\n> +\n> +\tpath = argv[0];\n> +\tnewurl = argv[1];\n> +\n> +\tif (argc != 2 || !path || !newurl) {\n> +\t\tusage_with_options(usage, options);\n> +\t\treturn 1;\n> +\t}\n> +\n> +\tconfig_name = xstrfmt(\"submodule.%s.url\", path);\n> +\n> +\tconfig_set_in_gitmodules_file_gently(config_name, newurl);\n> +\tsync_submodule(path, prefix, quiet ? OPT_QUIET : 0);\n> +\n> +\tfree(config_name);\n> +\n> +\treturn 0;\n> +}\n> +\n>  #define SUPPORT_SUPER_PREFIX (1<<0)\n>  \n>  struct cmd_struct {\n"},{"id":"397409","messageId":"xmqq8si21mlz.fsf_-_@gitster.c.googlers.com","threadId":"53408","inReplyTo":"CAP8UFD0=_8D8hkT5VVPV_F++dr131bkjby357fA+QfhQxktcMg@mail.gmail.com","subject":"Re* [PATCH v4] submodule: port subcommand 'set-url' from shell to C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-08T15:18:32Z","receivedAt":"2020-05-08T15:18:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <christian.couder@gmail.com> writes:\n\n> On Fri, May 8, 2020 at 7:51 AM Shourya Shukla\n> <shouryashukla.oo@gmail.com> wrote:\n>>\n>> On 06/05 10:08, Junio C Hamano wrote:\n>\n>> > Specifically, I would write \"!path\", not \"path == NULL\".  I thought\n>> > a rule for that is in the CodingGuidelines (I didn't double check,\n>> > though).\n>>\n>> I could not find a rule like that in the CodingGuidelines.\n>> Should I add it?\n>> https://github.com/git/git/blob/master/Documentation/CodingGuidelines\n>\n> Sure.\n\nI'd rather not see too many unrelated things piled up on Shourya's\nplate.  Without guidance, the new entry we'll see would be only\nabout comparing with NULL, and we'd need to spend review cycles\ncorrecting that, too.\n\nHow about something like this, perhaps?\n\n-- >8 --\nCodingGuidelines: do not ==/!= compare with 0/NULL\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n Documentation/CodingGuidelines | 13 +++++++++++++\n 1 file changed, 13 insertions(+)\n\ndiff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\nindex 390ceece52..41a89dd845 100644\n--- a/Documentation/CodingGuidelines\n+++ b/Documentation/CodingGuidelines\n@@ -236,6 +236,19 @@ For C programs:\n         while( condition )\n \t\tfunc (bar+1);\n \n+ - Do not explicitly compare an integral value with constant 0 or a\n+   pointer value with constant NULL for equality; just say !value\n+   instead.  To validate a counted array at ptr that has cnt elements\n+   in it, write:\n+\n+\tif (!ptr || !cnt)\n+\t\tBUG(\"array should not be empty at this point\");\n+\n+   and not:\n+\n+\tif (ptr == NULL || cnt == 0);\n+\t\tBUG(\"array should not be empty at this point\");\n+\n  - We avoid using braces unnecessarily.  I.e.\n \n \tif (bla) {\n"},{"id":"397411","messageId":"CAPig+cQP_9onrq-z5db1GhXSSHaeKJ+UhNewWP25wLCsMRzSrA@mail.gmail.com","threadId":"53408","inReplyTo":"xmqq8si21mlz.fsf_-_@gitster.c.googlers.com","subject":"Re: Re* [PATCH v4] submodule: port subcommand 'set-url' from shell to C","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-05-08T15:38:34Z","receivedAt":"2020-05-08T15:38:48Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, May 8, 2020 at 11:18 AM Junio C Hamano <gitster@pobox.com> wrote:\n> + - Do not explicitly compare an integral value with constant 0 or a\n> +   pointer value with constant NULL for equality; just say !value\n> +   instead.  To validate a counted array at ptr that has cnt elements\n> +   in it, write:\n> +\n> +       if (!ptr || !cnt)\n> +               BUG(\"array should not be empty at this point\");\n> +\n> +   and not:\n> +\n> +       if (ptr == NULL || cnt == 0);\n> +               BUG(\"array should not be empty at this point\");\n\nThis talks only about '=='. People might still use 0 or NULL with\n'!='. I wonder if the example can include '!=', as well. Perhaps:\n\n    if (!ptr)\n        BUG(\"...\");\n    if (cnt)\n        foo(ptr, cnt);\n\ninstead of:\n\n    if (ptr == NULL)\n        BUG(\"...\");\n    if (cnt != 0)\n        foo(ptr, cnt);\n\nor something.\n\nAlso, would you want to talk about not comparing against NUL character?\n\n    if (*s)\n        foo(s);\n\ninstead of:\n\n    if (*s != '\\0')\n        foo(s);\n\nMaybe that's overkill since NUL is an integral value which is already\ncovered by your earlier statement (but perhaps some people would\noverlook that).\n"},{"id":"397414","messageId":"xmqqpnbezaga.fsf@gitster.c.googlers.com","threadId":"53408","inReplyTo":"CAPig+cQP_9onrq-z5db1GhXSSHaeKJ+UhNewWP25wLCsMRzSrA@mail.gmail.com","subject":"Re: Re* [PATCH v4] submodule: port subcommand 'set-url' from shell to C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-08T15:57:09Z","receivedAt":"2020-05-08T15:57:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> On Fri, May 8, 2020 at 11:18 AM Junio C Hamano <gitster@pobox.com> wrote:\n>> + - Do not explicitly compare an integral value with constant 0 or a\n>> +   pointer value with constant NULL for equality; just say !value\n>> +   instead.  To validate a counted array at ptr that has cnt elements\n>> +   in it, write:\n>> +\n>> +       if (!ptr || !cnt)\n>> +               BUG(\"array should not be empty at this point\");\n>> +\n>> +   and not:\n>> +\n>> +       if (ptr == NULL || cnt == 0);\n>> +               BUG(\"array should not be empty at this point\");\n>\n> This talks only about '=='.\n\nYup.  The text would need a matching change, though.\n\n> People might still use 0 or NULL with\n> '!='. I wonder if the example can include '!=', as well. Perhaps:\n>\n>     if (!ptr)\n>         BUG(\"...\");\n>     if (cnt)\n>         foo(ptr, cnt);\n>\n> instead of:\n>\n>     if (ptr == NULL)\n>         BUG(\"...\");\n>     if (cnt != 0)\n>         foo(ptr, cnt);\n>\n> or something.\n\nOr more succinctly:\n\n\tif (!ptr || cnt)\n\t\tBUG(\"we must have an empty array at this point\");\n\nperhaps?\n\n> Also, would you want to talk about not comparing against NUL character?\n>\n>     if (*s)\n>         foo(s);\n>\n> instead of:\n>\n>     if (*s != '\\0')\n>         foo(s);\n>\n> Maybe that's overkill since NUL is an integral value which is already\n> covered by your earlier statement (but perhaps some people would\n> overlook that).\n\nYeah, it might be worth saying it explicitly.  I dunno.\n\n\n"},{"id":"397415","messageId":"20200508161315.GA3504@flurp.local","threadId":"53408","inReplyTo":"xmqqpnbezaga.fsf@gitster.c.googlers.com","subject":"Re: Re* [PATCH v4] submodule: port subcommand 'set-url' from shell to C","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-05-08T16:13:15Z","receivedAt":"2020-05-08T16:13:25Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, May 08, 2020 at 08:57:09AM -0700, Junio C Hamano wrote:\n> Eric Sunshine <sunshine@sunshineco.com> writes:\n> > This talks only about '=='.\n> \n> Yup.  The text would need a matching change, though.\n\nHere's a re-roll with the necessary changes.\n\n> >     if (!ptr)\n> >         BUG(\"...\");\n> >     if (cnt)\n> >         foo(ptr, cnt);\n> \n> Or more succinctly:\n> \n> \tif (!ptr || cnt)\n> \t\tBUG(\"we must have an empty array at this point\");\n\nI considered that but thought it might be too \"cute\", however, seeing\nit written out, it looks fine, so I used it in the re-roll.\n\n> > Also, would you want to talk about not comparing against NUL character?\n> \n> Yeah, it might be worth saying it explicitly.  I dunno.\n\nRather than giving this a separate example in the re-roll, I just\nmentioned '\\0' in the text.\n\n--- >8 ---\n\nFrom: Junio C Hamano <gitster@pobox.com>\nSubject: [PATCH] CodingGuidelines: do not ==/!= compare with 0 or '\\0' or NULL\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Eric Sunshine <sunshine@sunshineco.com>\n---\n Documentation/CodingGuidelines | 12 ++++++++++++\n 1 file changed, 12 insertions(+)\n\ndiff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\nindex 390ceece52..6dfc47ed7d 100644\n--- a/Documentation/CodingGuidelines\n+++ b/Documentation/CodingGuidelines\n@@ -236,6 +236,18 @@ For C programs:\n         while( condition )\n \t\tfunc (bar+1);\n \n+ - Do not explicitly compare an integral value with constant 0 or '\\0',\n+   or a pointer value with constant NULL.  For instance, to validate a\n+   counted array ptr that has cnt elements, write:\n+\n+\tif (!ptr || cnt)\n+\t\tBUG(\"empty array expected\");\n+\n+   and not:\n+\n+\tif (ptr == NULL || cnt != 0);\n+\t\tBUG(\"empty array expected\");\n+\n  - We avoid using braces unnecessarily.  I.e.\n \n \tif (bla) {\n-- \n2.26.2.717.g5cccb0e1a8\n"},{"id":"397416","messageId":"xmqqlfm2z9oh.fsf@gitster.c.googlers.com","threadId":"53408","inReplyTo":"20200508063022.GA18557@generichostname","subject":"Re: [PATCH v5] submodule: port subcommand 'set-url' from shell to C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-08T16:13:50Z","receivedAt":"2020-05-08T16:13:56Z","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> Hi Shourya,\n>\n> It looks good to me except for one tiny nit:\n>\n> On Fri, May 08, 2020 at 11:51:36AM +0530, Shourya Shukla wrote:\n>> Convert submodule subcommand 'set-url' to a builtin. Port 'set-url' to\n>> 'submodule--helper.c' and call the latter via 'git-submodule.sh'.\n>> \n>> Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>\n>> ---\n>>  builtin/submodule--helper.c | 37 +++++++++++++++++++++++++++++++++++++\n>>  git-submodule.sh            | 22 +---------------------\n>>  2 files changed, 38 insertions(+), 21 deletions(-)\n>> \n>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n>> index 1a4b391c88..8bc7b4cfa6 100644\n>> --- a/builtin/submodule--helper.c\n>> +++ b/builtin/submodule--helper.c\n>> @@ -2246,6 +2246,42 @@ static int module_config(int argc, const char **argv, const char *prefix)\n>>  \tusage_with_options(git_submodule_helper_usage, module_config_options);\n>>  }\n>>  \n>> +static int module_set_url(int argc, const char **argv, const char *prefix)\n>> +{\n>> +\tint quiet = 0;\n>> +\tconst char *newurl;\n>> +\tconst char *path;\n>> +\tchar* config_name;\n>\n> The asterisk should be stuck with the name, not the type, similar to how\n> you wrote it above.\n\nRight.\n\n>> +\n>> +\tstruct option options[] = {\n>> +\t\tOPT__QUIET(&quiet, N_(\"Suppress output for setting url of a submodule\")),\n>> +\t\tOPT_END()\n>> +\t};\n>> +\tconst char *const usage[] = {\n>> +\t\tN_(\"git submodule--helper set-url [--quiet] <path> <newurl>\"),\n>> +\t\tNULL\n>> +\t};\n>> +\n>> +\targc = parse_options(argc, argv, prefix, options, usage, 0);\n>> +\n>> +\tpath = argv[0];\n>> +\tnewurl = argv[1];\n>> +\n>> +\tif (argc != 2 || !path || !newurl) {\n\nChecking argc at this point is too late to protect against the\npotential out-of-bounds access we have already made to argv[0]\nand argv[1].\n"},{"id":"397417","messageId":"xmqqh7wqz9il.fsf@gitster.c.googlers.com","threadId":"53408","inReplyTo":"20200508063022.GA18557@generichostname","subject":"Re: [PATCH v5] submodule: port subcommand 'set-url' from shell to C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-08T16:17:22Z","receivedAt":"2020-05-08T16:17:27Z","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>> +\tif (argc != 2 || !path || !newurl) {\n>> +\t\tusage_with_options(usage, options);\n>> +\t\treturn 1;\n\nIt is embarrassing that nobody noticed that usage_with_options() is\nNORETURN; return 1 has no effect here.\n\n>> +\t}\n>> +\n>> +\tconfig_name = xstrfmt(\"submodule.%s.url\", path);\n>> +\n>> +\tconfig_set_in_gitmodules_file_gently(config_name, newurl);\n>> +\tsync_submodule(path, prefix, quiet ? OPT_QUIET : 0);\n>> +\n>> +\tfree(config_name);\n>> +\n>> +\treturn 0;\n>> +}\n>> +\n>>  #define SUPPORT_SUPER_PREFIX (1<<0)\n>>  \n>>  struct cmd_struct {\n"},{"id":"397418","messageId":"xmqqd07ez9g7.fsf_-_@gitster.c.googlers.com","threadId":"53408","inReplyTo":"20200508062136.15257-1-shouryashukla.oo@gmail.com","subject":"[PATCH v6] submodule: port subcommand 'set-url' from shell to C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-08T16:18:48Z","receivedAt":"2020-05-08T16:18:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"From: Shourya Shukla <shouryashukla.oo@gmail.com>\n\nConvert submodule subcommand 'set-url' to a builtin. Port 'set-url' to\n'submodule--helper.c' and call the latter via 'git-submodule.sh'.\n\nSigned-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * Here is what I'll queue with fixups for now.\n\n builtin/submodule--helper.c | 32 ++++++++++++++++++++++++++++++++\n git-submodule.sh            | 22 +---------------------\n 2 files changed, 33 insertions(+), 21 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 1a4b391c88..46c03d2a12 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2246,6 +2246,37 @@ static int module_config(int argc, const char **argv, const char *prefix)\n \tusage_with_options(git_submodule_helper_usage, module_config_options);\n }\n \n+static int module_set_url(int argc, const char **argv, const char *prefix)\n+{\n+\tint quiet = 0;\n+\tconst char *newurl;\n+\tconst char *path;\n+\tchar *config_name;\n+\n+\tstruct option options[] = {\n+\t\tOPT__QUIET(&quiet, N_(\"Suppress output for setting url of a submodule\")),\n+\t\tOPT_END()\n+\t};\n+\tconst char *const usage[] = {\n+\t\tN_(\"git submodule--helper set-url [--quiet] <path> <newurl>\"),\n+\t\tNULL\n+\t};\n+\n+\targc = parse_options(argc, argv, prefix, options, usage, 0);\n+\n+\tif (argc != 2 || !(path = argv[0]) || !(newurl = argv[1]))\n+\t\tusage_with_options(usage, options);\n+\n+\tconfig_name = xstrfmt(\"submodule.%s.url\", path);\n+\n+\tconfig_set_in_gitmodules_file_gently(config_name, newurl);\n+\tsync_submodule(path, prefix, quiet ? OPT_QUIET : 0);\n+\n+\tfree(config_name);\n+\n+\treturn 0;\n+}\n+\n #define SUPPORT_SUPER_PREFIX (1<<0)\n \n struct cmd_struct {\n@@ -2276,6 +2307,7 @@ static struct cmd_struct commands[] = {\n \t{\"is-active\", is_active, 0},\n \t{\"check-name\", check_name, 0},\n \t{\"config\", module_config, 0},\n+\t{\"set-url\", module_set_url, 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 08e0439df0..39ebdf25b5 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -805,27 +805,7 @@ cmd_set_url() {\n \t\tshift\n \tdone\n \n-\tif test $# -ne 2\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-\turl=\"$2\"\n-\tif test -z \"$url\"\n-\tthen\n-\t\texit 1\n-\tfi\n-\n-\tgit submodule--helper config submodule.\"$name\".url \"$url\"\n-\tgit submodule--helper sync ${GIT_QUIET:+--quiet} \"$name\"\n+\tgit ${wt_prefix:+-C \"$wt_prefix\"} ${prefix:+--super-prefix \"$prefix\"} submodule--helper set-url ${GIT_QUIET:+--quiet} -- \"$@\"\n }\n \n #\n-- \n2.26.2-561-g07d8ea56f2\n\n"},{"id":"397420","messageId":"xmqq4ksqz8jq.fsf@gitster.c.googlers.com","threadId":"53408","inReplyTo":"20200508161315.GA3504@flurp.local","subject":"Re: Re* [PATCH v4] submodule: port subcommand 'set-url' from shell to C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-08T16:38:17Z","receivedAt":"2020-05-08T16:38:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> From: Junio C Hamano <gitster@pobox.com>\n> Subject: [PATCH] CodingGuidelines: do not ==/!= compare with 0 or '\\0' or NULL\n>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>\n> ---\n>  Documentation/CodingGuidelines | 12 ++++++++++++\n>  1 file changed, 12 insertions(+)\n>\n> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\n> index 390ceece52..6dfc47ed7d 100644\n> --- a/Documentation/CodingGuidelines\n> +++ b/Documentation/CodingGuidelines\n> @@ -236,6 +236,18 @@ For C programs:\n>          while( condition )\n>  \t\tfunc (bar+1);\n>  \n> + - Do not explicitly compare an integral value with constant 0 or '\\0',\n> +   or a pointer value with constant NULL.  For instance, to validate a\n> +   counted array ptr that has cnt elements, write:\n\nI think this should be\n\n      counted array <ptr, cnt> is initialized but has no elements, write:\n\n> +\n> +\tif (!ptr || cnt)\n> +\t\tBUG(\"empty array expected\");\n> +\n> +   and not:\n> +\n> +\tif (ptr == NULL || cnt != 0);\n> +\t\tBUG(\"empty array expected\");\n> +\n>   - We avoid using braces unnecessarily.  I.e.\n>  \n>  \tif (bla) {\n"},{"id":"397428","messageId":"20200508175121.GA20180@flurp.local","threadId":"53408","inReplyTo":"xmqq4ksqz8jq.fsf@gitster.c.googlers.com","subject":"Re: Re* [PATCH v4] submodule: port subcommand 'set-url' from shell to C","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-05-08T17:51:21Z","receivedAt":"2020-05-08T17:51:31Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, May 08, 2020 at 09:38:17AM -0700, Junio C Hamano wrote:\n> Eric Sunshine <sunshine@sunshineco.com> writes:\n> > + - Do not explicitly compare an integral value with constant 0 or '\\0',\n> > +   or a pointer value with constant NULL.  For instance, to validate a\n> > +   counted array ptr that has cnt elements, write:\n> \n> I think this should be\n> \n>       counted array <ptr, cnt> is initialized but has no elements, write:\n\nYou're right. Here's a corrected version. I also applied s/a/that/ in\nthe second line to improve the grammar a bit.\n\n--- >8 ---\n\nFrom: Junio C Hamano <gitster@pobox.com>\nSubject: [PATCH v3] CodingGuidelines: do not ==/!= compare with 0 or '\\0' or\n NULL\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Eric Sunshine <sunshine@sunshineco.com>\n---\n Documentation/CodingGuidelines | 12 ++++++++++++\n 1 file changed, 12 insertions(+)\n\ndiff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\nindex 390ceece52..803a3b9bde 100644\n--- a/Documentation/CodingGuidelines\n+++ b/Documentation/CodingGuidelines\n@@ -236,6 +236,18 @@ For C programs:\n         while( condition )\n \t\tfunc (bar+1);\n \n+ - Do not explicitly compare an integral value with constant 0 or '\\0',\n+   or a pointer value with constant NULL.  For instance, to validate that\n+   counted array <ptr, cnt> is initialized but has no elements, write:\n+\n+\tif (!ptr || cnt)\n+\t\tBUG(\"empty array expected\");\n+\n+   and not:\n+\n+\tif (ptr == NULL || cnt != 0);\n+\t\tBUG(\"empty array expected\");\n+\n  - We avoid using braces unnecessarily.  I.e.\n \n \tif (bla) {\n-- \n2.26.2.737.gf3227dd3d3\n"}]}