{"thread":{"id":"53364","subject":"[PATCH v2] submodule: port subcommand 'set-url' from shell to C","startedAt":"2020-05-01T07:32:47Z","lastAt":"2020-05-01T18:55:01Z","messageCount":4,"participants":["Shourya Shukla","Denton Liu","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"396727","messageId":"20200501073232.18409-1-shouryashukla.oo@gmail.com","threadId":"53364","inReplyTo":null,"subject":"[PATCH v2] submodule: port subcommand 'set-url' from shell to C","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-05-01T07:32:32Z","receivedAt":"2020-05-01T07:32:47Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"This aims to convert submodule subcommand 'set-url' to a builtin.\n'set-url' is ported to 'submodule--helper.c' and the latter is called\nvia 'git-submodule.sh'.\n\n'module_set_url()' accepts the the user input, which is parsed by\n'parse_options()', followed by the setting of the appropriate entry\n(i.e., the submodule URL here). Finally, the URL is synced, via\n'sync_submodule()', with the upstream.\n\nSigned-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>\n---\nI finally fixed the implementation, was making a silly mistake.\nThe test './t7420-submodule-set-url.sh' passes (both tests 1 and 2\nare successful now). I have deleted the function,\n'update_url_in_gitmodules()' because I was able to suffice that\nfunctionality in the 'module_set_url()' function itself.\n\nThank you so much Christian and Denton for helping me :)\n\n builtin/submodule--helper.c | 55 +++++++++++++++++++++++++++++++++++++\n git-submodule.sh            | 25 ++---------------\n 2 files changed, 58 insertions(+), 22 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 1a4b391c88..59334d4286 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2246,6 +2246,60 @@ 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+struct set_url_cb {\n+\tconst char *prefix;\n+\tunsigned int flags;\n+};\n+\n+#define SET_URL_CB_INIT { NULL, 0 }\n+\n+static int module_set_url(int argc, const char **argv, const char *prefix)\n+{\n+\tstruct set_url_cb info = SET_URL_CB_INIT;\n+\tint quiet = 0;\n+\n+\tconst char *newurl = NULL;\n+\tconst char *path = NULL;\n+\t\n+\tstruct strbuf entry = STRBUF_INIT;\n+\n+\tstruct option module_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 git_submodule_helper_usage[] = {\n+\t\tN_(\"git submodule--helper set-url [--quiet] [<path>] [<newurl>]\"),\n+\t\tNULL\n+\t};\n+\n+\targc = parse_options(argc, argv, NULL, module_set_url_options,\n+\t\t\t     git_submodule_helper_usage, 0);\n+\n+\tinfo.prefix = prefix;\n+\tif (quiet)\n+\t\tinfo.flags |= OPT_QUIET;\n+\n+\tpath = argv[0];\n+\tnewurl = argv[1];\n+\n+\tif(!path || !newurl){\n+\t\tusage_with_options(git_submodule_helper_usage,module_set_url_options);\n+\t\treturn 1;\n+\t}\n+\n+\tstrbuf_addstr(&entry, \"submodule.\");\n+\tstrbuf_addstr(&entry, path);\n+\tstrbuf_addstr(&entry, \".url\");\n+\t\n+\t/* Setting the new URL in .gitmodules */\n+\tconfig_set_in_gitmodules_file_gently(entry.buf, newurl);  \n+\t/* Syncing the updated URL */\n+\tsync_submodule(path, prefix, info.flags);\n+\n+\treturn 0;\n+}\n+\n #define SUPPORT_SUPER_PREFIX (1<<0)\n \n struct cmd_struct {\n@@ -2276,6 +2330,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 89f915cae9..ea72d5a5f5 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -783,11 +783,13 @@ cmd_set_branch() {\n # $@ = requested path, requested url\n #\n cmd_set_url() {\n+\n \twhile test $# -ne 0\n \tdo\n \t\tcase \"$1\" in\n \t\t-q|--quiet)\n \t\t\tGIT_QUIET=1\n+\t\t\tshift\n \t\t\t;;\n \t\t--)\n \t\t\tshift\n@@ -800,30 +802,9 @@ cmd_set_url() {\n \t\t\tbreak\n \t\t\t;;\n \t\tesac\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":"396739","messageId":"20200501100109.GA55820@generichostname","threadId":"53364","inReplyTo":"20200501073232.18409-1-shouryashukla.oo@gmail.com","subject":"Re: [PATCH v2] submodule: port subcommand 'set-url' from shell to C","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-05-01T10:01:09Z","receivedAt":"2020-05-01T10:01:16Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Shourya,\n\nI ran into a few whitespace errors when applying this patch. You should\nrun `git diff --check` before committing so that you can fix these.\n\nOn Fri, May 01, 2020 at 01:02:32PM +0530, Shourya Shukla wrote:\n> This aims to convert submodule subcommand 'set-url' to a builtin.\n> 'set-url' is ported to 'submodule--helper.c' and the latter is called\n> via 'git-submodule.sh'.\n\nCommit messages should be written in imperative mood. This could be\nrewritten as\n\n\tConvert submodule subcommand 'set-url' to a builtin. Port\n\t'set-url' to 'submodule--helper.c' and call the latter via\n\t'git-submodule.sh'.\n\n> 'module_set_url()' accepts the the user input, which is parsed by\n\ns/the the/the/\n\n> 'parse_options()', followed by the setting of the appropriate entry\n> (i.e., the submodule URL here). Finally, the URL is synced, via\n> 'sync_submodule()', with the upstream.\n\nOthers might disagree with me but I believe that this paragraph dives\nin too deeply. It might be more helpful to give a higher level overview\nthan repeating what the code.\n\n> Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>\n> ---\n> I finally fixed the implementation, was making a silly mistake.\n> The test './t7420-submodule-set-url.sh' passes (both tests 1 and 2\n> are successful now). I have deleted the function,\n> 'update_url_in_gitmodules()' because I was able to suffice that\n> functionality in the 'module_set_url()' function itself.\n> \n> Thank you so much Christian and Denton for helping me :)\n\nNo problem :)\n\n>  builtin/submodule--helper.c | 55 +++++++++++++++++++++++++++++++++++++\n>  git-submodule.sh            | 25 ++---------------\n>  2 files changed, 58 insertions(+), 22 deletions(-)\n> \n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index 1a4b391c88..59334d4286 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -2246,6 +2246,60 @@ 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> +struct set_url_cb {\n> +\tconst char *prefix;\n> +\tunsigned int flags;\n> +};\n> +\n> +#define SET_URL_CB_INIT { NULL, 0 }\n> +\n> +static int module_set_url(int argc, const char **argv, const char *prefix)\n> +{\n> +\tstruct set_url_cb info = SET_URL_CB_INIT;\n> +\tint quiet = 0;\n> +\n> +\tconst char *newurl = NULL;\n> +\tconst char *path = NULL;\n> +\t\n> +\tstruct strbuf entry = STRBUF_INIT;\n> +\n> +\tstruct option module_set_url_options[] = {\n> +\t\tOPT__QUIET(&quiet, N_(\"Suppress output for setting url of a submodule\")),\n\nI know that many options in the rest of the file don't follow this\nconvention but the help string should begin with a lowercase.\n\n> +\t\tOPT_END()\n> +\t};\n> +\n> +\tconst char *const git_submodule_helper_usage[] = {\n> +\t\tN_(\"git submodule--helper set-url [--quiet] [<path>] [<newurl>]\"),\n\n<path> and <newurl> should be mandatory arguments.\n\n> +\t\tNULL\n> +\t};\n> +\n> +\targc = parse_options(argc, argv, NULL, module_set_url_options,\n\nHow come you aren't passing in the prefix here?\n\n> +\t\t\t     git_submodule_helper_usage, 0);\n> +\n> +\tinfo.prefix = prefix;\n> +\tif (quiet)\n> +\t\tinfo.flags |= OPT_QUIET;\n> +\n\nOver here, you should check argc to ensure that we have exactly two\narguments...\n\n> +\tpath = argv[0];\n> +\tnewurl = argv[1];\n\n...otherwise this may segfault.\n\n> +\n> +\tif(!path || !newurl){\n\nnit: space after `if` and betwen `) {`. Also, this won't be necessary\nonce you check the argc.\n\n> +\t\tusage_with_options(git_submodule_helper_usage,module_set_url_options);\n\nnit: space after the `,`\n\n> +\t\treturn 1;\n> +\t}\n> +\n> +\tstrbuf_addstr(&entry, \"submodule.\");\n> +\tstrbuf_addstr(&entry, path);\n> +\tstrbuf_addstr(&entry, \".url\");\n> +\t\n> +\t/* Setting the new URL in .gitmodules */\n\nI don't think these comments are necessary. It's pretty obvious what's\nhappening in the code.\n\n> +\tconfig_set_in_gitmodules_file_gently(entry.buf, newurl);  \n> +\t/* Syncing the updated URL */\n> +\tsync_submodule(path, prefix, info.flags);\n> +\n\nMake sure you strbuf_release() the strbuf here.\n\n> +\treturn 0;\n> +}\n> +\n>  #define SUPPORT_SUPER_PREFIX (1<<0)\n>  \n>  struct cmd_struct {\n> @@ -2276,6 +2330,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)\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index 89f915cae9..ea72d5a5f5 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -783,11 +783,13 @@ cmd_set_branch() {\n>  # $@ = requested path, requested url\n>  #\n>  cmd_set_url() {\n> +\n>  \twhile test $# -ne 0\n>  \tdo\n>  \t\tcase \"$1\" in\n>  \t\t-q|--quiet)\n>  \t\t\tGIT_QUIET=1\n> +\t\t\tshift\n>  \t\t\t;;\n>  \t\t--)\n>  \t\t\tshift\n> @@ -800,30 +802,9 @@ cmd_set_url() {\n>  \t\t\tbreak\n>  \t\t\t;;\n>  \t\tesac\n> -\t\tshift\n\nHmmm, I don't understand why the above hunk and this line are necessary.\n\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\nThis looks good, though :)\n\nThanks,\n\nDenton\n\n>  }\n>  \n>  #\n> -- \n> 2.26.2\n> \n"},{"id":"396775","messageId":"xmqq5zdfmryd.fsf@gitster.c.googlers.com","threadId":"53364","inReplyTo":"20200501073232.18409-1-shouryashukla.oo@gmail.com","subject":"Re: [PATCH v2] submodule: port subcommand 'set-url' from shell to C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-01T18:27:06Z","receivedAt":"2020-05-01T18:27:18Z","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> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index 1a4b391c88..59334d4286 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -2246,6 +2246,60 @@ 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> +struct set_url_cb {\n> +\tconst char *prefix;\n> +\tunsigned int flags;\n> +};\n> +\n> +#define SET_URL_CB_INIT { NULL, 0 }\n\nIs any of the above even necessary?  I cannot see the place\ninfo.prefix is used (I can see where it is set, but the only other\nappearance of string 'prefix' is a call to sync_submodule() and\ninfo.prefix is not used there at all), and info.flags is assigned\nOPT_QUIET and passed to sync_submodule().  If you must have a\nvariable, a single \"unsigned int\" local variable in the function\nwould be sufficient, but it is unclear if even that is needed.\n\nWorse yet, it claims to have something to do with a callback API by\nhaving _cb suffix, but nobody calls anything with the data here.\n\n> +static int module_set_url(int argc, const char **argv, const char *prefix)\n> +{\n> +\tstruct set_url_cb info = SET_URL_CB_INIT;\n\nAnd this is the only place where SET_URL_CB_INIT is used.\n\nI think all of the above should be discarded.\n\n> +\tint quiet = 0;\n\nThis is a good initialization, as parse_options() may not even see\nany option related to this variable.\n\n> +\tconst char *newurl = NULL;\n> +\tconst char *path = NULL;\n\nThese are unnecessary, as we will _always_ assign to them before\ndoing anything else.  It is sufficient to say:\n\n\tconst char *newurl, *path;\n\n> +\tstruct strbuf entry = STRBUF_INIT;\n> +\n\nWhy is this called entry?  All other occurences of string 'entry' in\nthsi file always refer to a cache_entry.  Read on for a better name.\n\n> +\tstruct option module_set_url_options[] = {\n> +\t\tOPT__QUIET(&quiet, N_(\"Suppress output for setting url of a submodule\")),\n> +\t\tOPT_END()\n> +\t};\n\nI think you are copying what other functions do, but for a local\nvariable to this function, I do not see the need to call this array\nwith such a long name.  Calling it \"set_url_options[]\" or even just\n\"options[]\" is sufficient, because there is no other \"struct option\"\ninvolved in this function.\n\n> +\tconst char *const git_submodule_helper_usage[] = {\n> +\t\tN_(\"git submodule--helper set-url [--quiet] [<path>] [<newurl>]\"),\n> +\t\tNULL\n> +\t};\n\nLikewise.  \"usage[]\" should be plenty clear.\n\n> +\targc = parse_options(argc, argv, NULL, module_set_url_options,\n> +\t\t\t     git_submodule_helper_usage, 0);\n\nAnd this would become\n\n\targc = parse_options(argc, argv, NULL, options,\tusage, 0);\n\nwithout losing any clarity.\n\n> +\tinfo.prefix = prefix;\n> +\tif (quiet)\n> +\t\tinfo.flags |= OPT_QUIET;\n\nDrop these three lines.\n\n> +\tpath = argv[0];\n> +\tnewurl = argv[1];\n\nWait.  If path is NULL, it is likely that argv[1] is an\nout-of-bounds access.\n\n> +\tif(!path || !newurl){\n\nStyle.\n\n\tif (!path || !newurl) {\n\n> +\t\tusage_with_options(git_submodule_helper_usage,module_set_url_options);\n\nThis would become a short-and-sweet\n\n\t\tusage_with_options(usage, options);\n\nNote that we require a SP after that comma.\n\n> +\t\treturn 1;\n\nOK.  This follows a (bit unnatural) convention used throughout this\nfile that module_<action> helpers signal a failure with positive 1,\nand signal a success with 0.\n\n> +\t}\n> +\n\nTaking all these together,\n\n\tif (argc != 3) {\n\t\tusage_with_options(usage, options);\n                return 1;\n\t}\n        path = argv[0];\n        newurl = argv[1];\n\nIf you feel paranoid, you can check these two are not NULL, too,\ni.e.\n\n        if (argc != 3 || !(path = argv[0]) || !(newurl = argv[1])) {\n\t\tusage_with_options(usage, options);\n\t\treturn 1;\n\t}\n\nI have no strong preference either way.  Perhaps the latter is more\nconcise and more careful at the same time, so some people may prefer\nit.\n\n> +\tstrbuf_addstr(&entry, \"submodule.\");\n> +\tstrbuf_addstr(&entry, path);\n> +\tstrbuf_addstr(&entry, \".url\");\n\nThe strbuf is to hold a configuration variable name and has nothing\nto do with a cache entry.  Perhaps\n\n\tstrbuf_addf(&config_name, \"submodule.%s.url\", path);\n\n> +\t/* Setting the new URL in .gitmodules */\n> +\tconfig_set_in_gitmodules_file_gently(entry.buf, newurl);  \n\nIt is dubious the comment above this call (and above the next call\nto sync) adds any value.  The function name and its parameters tells\nus enough, especially if you rename the strbuf variable and in a\nsingle strbuf_addf(), which makes it pretty clear what the variable\ncontains (i.e. \"submodule.<path>.url\" variable in the .gitmodules\nfile).\n\n> +\t/* Syncing the updated URL */\n> +\tsync_submodule(path, prefix, info.flags);\n\nThis will become\n\n\tsync_submodule(path, prefix, quiet ? OPT_QUIET : 0);\n\nSo, there was no need for 'flags' variable, let alone set_url_cb\nstructure, at all.\n\n> +\treturn 0;\n> +}\n> +\n>  #define SUPPORT_SUPER_PREFIX (1<<0)\n>  \n>  struct cmd_struct {\n> @@ -2276,6 +2330,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)\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index 89f915cae9..ea72d5a5f5 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -783,11 +783,13 @@ cmd_set_branch() {\n>  # $@ = requested path, requested url\n>  #\n>  cmd_set_url() {\n> +\n>  \twhile test $# -ne 0\n>  \tdo\n>  \t\tcase \"$1\" in\n>  \t\t-q|--quiet)\n>  \t\t\tGIT_QUIET=1\n> +\t\t\tshift\n>  \t\t\t;;\n>  \t\t--)\n>  \t\t\tshift\n> @@ -800,30 +802,9 @@ cmd_set_url() {\n>  \t\t\tbreak\n>  \t\t\t;;\n>  \t\tesac\n> -\t\tshift\n>  \tdone\n\nWhy any of the above change?  It may not change the behaviour from\nthe current implementation, but why do we even want to touch it?\n\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\nThe loss of these lines is a good change ;-)  Very pleased to see\nfewer lines of shell script.\n\nThanks.\n"},{"id":"396777","messageId":"xmqqwo5vlc3i.fsf@gitster.c.googlers.com","threadId":"53364","inReplyTo":"20200501100109.GA55820@generichostname","subject":"Re: [PATCH v2] submodule: port subcommand 'set-url' from shell to C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-01T18:54:57Z","receivedAt":"2020-05-01T18:55:01Z","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> On Fri, May 01, 2020 at 01:02:32PM +0530, Shourya Shukla wrote:\n>> This aims to convert submodule subcommand 'set-url' to a builtin.\n>> 'set-url' is ported to 'submodule--helper.c' and the latter is called\n>> via 'git-submodule.sh'.\n>\n> Commit messages should be written in imperative mood. This could be\n> rewritten as\n>\n> \tConvert submodule subcommand 'set-url' to a builtin. Port\n> \t'set-url' to 'submodule--helper.c' and call the latter via\n> \t'git-submodule.sh'.\n> ...\n>>  \t\tesac\n>> -\t\tshift\n>\n> Hmmm, I don't understand why the above hunk and this line are necessary.\n>\n> ...\n>\n> This looks good, though :)\n\nThanks for giving a pretty good review.  I think we covered many\nissues pretty much the same way.\n\n"}]}