{"thread":{"id":"53387","subject":"[PATCH v3] submodule: port subcommand 'set-url' from shell to C","startedAt":"2020-05-04T07:27:42Z","lastAt":"2020-05-04T19:14:07Z","messageCount":4,"participants":["Shourya Shukla","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"396889","messageId":"20200504072705.15261-1-shouryashukla.oo@gmail.com","threadId":"53387","inReplyTo":null,"subject":"[PATCH v3] submodule: port subcommand 'set-url' from shell to C","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-05-04T07:27:05Z","receivedAt":"2020-05-04T07:27:42Z","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 \n---\nThank you Junio and Denton for the review :)\nEliminated the whitespace errors and removed the callback structures\nas well. I have made the commit message more abstract for now as pointed\nout by Denton that the message seems to go into too much detail.\n\nAlso, the variable name for `&entry` has been changed to `config_entry` so\nas to avoid confusion with `cache_entry`. The shell file has also been\namended.\n\n builtin/submodule--helper.c | 46 +++++++++++++++++++++++++++++++++++++\n git-submodule.sh | 22 +-----------------\n 2 files changed, 47 insertions(+), 21 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 1a4b391c88..6fd459988e 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2246,6 +2246,51 @@ 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+\n+\tconst char *newurl = NULL;\n+\tconst char *path = NULL;\n+\n+\tstruct strbuf config_entry = 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] \"),\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 (quiet)\n+\t\tquiet |= OPT_QUIET;\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_addstr(&config_entry, \"submodule.\");\n+\tstrbuf_addstr(&config_entry, path);\n+\tstrbuf_addstr(&config_entry, \".url\");\n+\n+\tconfig_set_in_gitmodules_file_gently(config_entry.buf, newurl);\n+\tsync_submodule(path, prefix, quiet);\n+\n+\tstrbuf_release(&config_entry);\n+\n+\treturn 0;\n+}\n+\n #define SUPPORT_SUPER_PREFIX (1<<0)\n \n struct cmd_struct {\n@@ -2276,6 +2321,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":"396915","messageId":"xmqqd07jitjz.fsf@gitster.c.googlers.com","threadId":"53387","inReplyTo":"20200504072705.15261-1-shouryashukla.oo@gmail.com","subject":"Re: [PATCH v3] submodule: port subcommand 'set-url' from shell to C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-04T15:55:12Z","receivedAt":"2020-05-04T15:55:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shourya Shukla 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\nOK.\n\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index 1a4b391c88..6fd459988e 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -2246,6 +2246,51 @@ 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> +\n> +\tconst char *newurl = NULL;\n> +\tconst char *path = NULL;\n> +\n> +\tstruct strbuf config_entry = 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] \"),\n> +\t\tNULL\n> +\t};\n\nI do not see much point in leaving blank lines between the above\nvariable declarations. In fact, it is somewhat irritating to see.\nDrop them.\n\nInitializing \"quiet\" to 0 is good, because parse_options() may not\nsee any \"-q\" or \"--quiet\" on the command line, and you do not want\nto leave the variable uninitialized.\n\nInitializing \"newurl\" and \"path\" on the other hand are totally\nunnecessary. In fact, it will defeat the chance in the future to be\nhelped by compiler warning when somebody accidentally loses the\nassignment to one of these variables. Don't initialize them\nunnecessarily.\n\n> +\targc = parse_options(argc, argv, prefix, set_url_options,\n> +\t\t\t usage, 0);\n> +\n> +\tif (quiet)\n> +\t\tquiet |= OPT_QUIET;\n\nThis is bogus. \"command --quiet --quiet\" would count-up quiet twice\nand would make it 2, and you or OPT_QUIET==1 in to make it 3, but\nyour intention is quite clear that you want to pass 1 to\nsync_submodule() in such a case.\n\nDidn't I give you a review a few days ago and suggested a way to\nmake this whole thing unnecessary?\n\n> +\tif (argc!=2){\n> +\t\tusage_with_options(usage, set_url_options);\n> +\t\treturn 1;\n> +\t}\n\nStyle.\n\n> +\tpath = argv[0];\n> +\tnewurl = argv[1];\n\nThese assign to path and newurl before they are used below, which\nmeans that they can be left uninitialized above without risking use\nof an uninitialized variable.\n\n> +\tstrbuf_addstr(&config_entry, \"submodule.\");\n> +\tstrbuf_addstr(&config_entry, path);\n> +\tstrbuf_addstr(&config_entry, \".url\");\n\nstrbuf_addf()?\n\nUsually an \"entry\" is not just the name of it but also its contents,\nso unless you are handing a struct that holds a tuple\ni.e. (\"submodule..url\", newurl), it is better not to call this\nanything-entry. It is a name of a variable, and on the next line\nyou'll give the variable a vlue. Perhaps config_name or something?\n\n> +\tconfig_set_in_gitmodules_file_gently(config_entry.buf, newurl);\n> +\tsync_submodule(path, prefix, quiet);\n> +\n> +\tstrbuf_release(&config_entry);\n> +\n> +\treturn 0;\n> +}\n> +\n\nI get a feeling that perhaps my review message on the previous round\ndid not reach you? It's here:\n\n https://lore.kernel.org/git/xmqq5zdfmryd.fsf@gitster.c.googlers.com/\n\nThanks.\n"},{"id":"396940","messageId":"20200504173936.GA8919@konoha","threadId":"53387","inReplyTo":"xmqqd07jitjz.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3] submodule: port subcommand 'set-url' from shell to C","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-05-04T17:39:36Z","receivedAt":"2020-05-04T17:39:45Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"On 04/05 08:55, Junio C Hamano wrote: \n> > +\targc = parse_options(argc, argv, prefix, set_url_options,\n> > +\t\t\t usage, 0);\n> > +\n> > +\tif (quiet)\n> > +\t\tquiet |= OPT_QUIET;\n> \n> This is bogus. \"command --quiet --quiet\" would count-up quiet twice\n> and would make it 2, and you or OPT_QUIET==1 in to make it 3, but\n> your intention is quite clear that you want to pass 1 to\n> sync_submodule() in such a case.\n\nThis is a grave mistake from my side. Though I do not understand how\nwill `quiet` be counted twice. Let's say we pass `--quiet` option in the\ncommand, then `quiet` is set to 1. And now if we do `quiet|= OPT_QUIET`,\nthis will make `quiet` 1 again right? Could you please tell me what am I\nmissing out here?\n\nThe fix you suggested (quiet ? OPT_QUIET : 0), we use this because we\nwant to ensure `quiet` goes into sync as either 1/0 right? Not any other\nnon-zero positive integer right?\n"},{"id":"396971","messageId":"xmqq368fh5s6.fsf@gitster.c.googlers.com","threadId":"53387","inReplyTo":"20200504173936.GA8919@konoha","subject":"Re: [PATCH v3] submodule: port subcommand 'set-url' from shell to C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-04T19:14:01Z","receivedAt":"2020-05-04T19:14:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shourya Shukla writes:\n\n> On 04/05 08:55, Junio C Hamano wrote: \n>> > +\targc = parse_options(argc, argv, prefix, set_url_options,\n>> > +\t\t\t usage, 0);\n>> > +\n>> > +\tif (quiet)\n>> > +\t\tquiet |= OPT_QUIET;\n>> \n>> This is bogus. \"command --quiet --quiet\" would count-up quiet twice\n>> and would make it 2, and you or OPT_QUIET==1 in to make it 3, but\n>> your intention is quite clear that you want to pass 1 to\n>> sync_submodule() in such a case.\n>\n> This is a grave mistake from my side. Though I do not understand how\n> will `quiet` be counted twice.\n\nThe way I read the definition of OPT__QUIET() \n\n #define OPT__QUIET(var, h) OPT_COUNTUP('q', \"quiet\", (var), (h))\n\nis that it is OPT_COUNTUP() in disguise, and that is designed to\nyield increased quietness when \"-q\" is given more than once.\n\n> The fix you suggested (quiet ? OPT_QUIET : 0), we use this because we\n> want to ensure `quiet` goes into sync as either 1/0 right? Not any other\n> non-zero positive integer right?\n\nThe \"if (quiet) quiet |= OPT_QUIET\" does not make *any* sense, if\nyou are expecting quiet to be set to 1 or left as 0 as initialized\nby parse_options() API. You are defeating the whole point of using\npreprocessor macro OPT_QUIET, as the correctness of the construct\nheavily rely on OPT_QUIET defined to be 1. If for any reason the\npreprocessor macro gets redefined to 8, writing\n\n\tquiet ? OPT_QUIET : 0\n\nwould need *no* adjustment, while \"if (quiet) quiet |= OPT_QUIET\"\nwould require fixing.\n\n"}]}