{"thread":{"id":"53246","subject":"[RFC][PATCH 0/2] submodule: port 'set-url' from shell to C","startedAt":"2020-04-16T21:05:28Z","lastAt":"2020-04-27T10:17:26Z","messageCount":8,"participants":["Shourya Shukla","Christian Couder","Denton Liu"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"395621","messageId":"20200416210456.19122-1-shouryashukla.oo@gmail.com","threadId":"53246","inReplyTo":null,"subject":"[RFC][PATCH 0/2] submodule: port 'set-url' from shell to C","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-04-16T21:04:54Z","receivedAt":"2020-04-16T21:05:28Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"Hello all,\n\nThis is my very first attempt at the conversion of subcommand 'set-url' from shell\nto C, thus making the subcommand a builtin.\n\nI have based my conversion by looking at the way other subcommands have been implemented\nin 'submodule--helper.c' as well as the subcommand 'set-url' in 'remote.c'. The approach\nI have taken is as follows:\n\n1. Create a helper function 'update_url_in_gitmodules()' in 'submodule.c'\n to update the URL of an entry in '.gitmodules'.\n\n2. Port the function 'cmd_set_url()' in 'git-submodule.sh' to 'module_set_url()'\n in 'submodule--helper.c'.\n\nThe issues I am facing are:\n\n1. The patch fails test #2 in t7420, i.e., the test to verify the working of 'set-url'\n subcommand.\n\n2. Though not an issue affecting the patch, but the 'usage' prompt of 'git submodule'\n does not show the subcommands 'set-url' and 'set-branch'.\n\nAlso, I am aware that the patch does not support the '--quiet' option. This is something\nI intended to add after ensuring that the patch works in the first place.\n\nI understand that the patch is in a weak condition right now. How can this patch be improved?\nAny guidance from the community would be appreciated! :)\n\nThanks,\nShourya Shukla\n\nShourya Shukla (2):\n submodule.c: update URL in .gitmodules using update_url_in_gitmodules\n submodule: port subcommand 'set-url' from shell to C\n\n builtin/submodule--helper.c | 38 +++++++++++++++++++++++++++++++++++++\n git-submodule.sh | 23 ++--------------------\n submodule.c | 33 ++++++++++++++++++++++++++++++++\n submodule.h | 2 ++\n 4 files changed, 75 insertions(+), 21 deletions(-)\n\n-- \n2.20.1\n\n"},{"id":"395622","messageId":"20200416210456.19122-2-shouryashukla.oo@gmail.com","threadId":"53246","inReplyTo":"20200416210456.19122-1-shouryashukla.oo@gmail.com","subject":"[PATCH 1/2] submodule.c: update URL in .gitmodules using update_url_in_gitmodules","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-04-16T21:04:55Z","receivedAt":"2020-04-16T21:05:32Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"Create a helper function update_url_in_gitmodules() to update URL\nof an entry in .gitmodules. Later on, we want to use this function\nto aid in the conversion of 'set-url' from shell to C.\n\nSigned-off-by: Shourya Shukla \n---\n submodule.c | 33 +++++++++++++++++++++++++++++++++\n submodule.h | 2 ++\n 2 files changed, 35 insertions(+)\n\ndiff --git a/submodule.c b/submodule.c\nindex c3aadf3fff..0b599dc4e1 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -126,6 +126,39 @@ int update_path_in_gitmodules(const char *oldpath, const char *newpath)\n \treturn ret;\n }\n \n+/*\n+ * Try to update the \"url\" entry in the \"submodule.\" section of the\n+ * .gitmodules file. Return 0 only if a .gitmodules file was found, a section\n+ * with the correct url= setting was found and we could update it.\n+ */\n+int update_url_in_gitmodules(const char *path, const char *newurl)\n+{\n+\tstruct strbuf entry = STRBUF_INIT;\n+\tconst struct submodule *submodule;\n+\tint ret;\n+\n+\t/* Do nothing without .gitmodules */\n+\tif (!file_exists(GITMODULES_FILE))\n+\t\treturn -1;\n+\n+\tif (is_gitmodules_unmerged(the_repository->index))\n+\t\tdie(_(\"Cannot change unmerged .gitmodules, resolve merge conflicts first\"));\n+\n+\tsubmodule = submodule_from_path(the_repository, &null_oid, path);\n+\tif (!submodule || !submodule->name) {\n+\t\twarning(_(\"Could not find section in .gitmodules where path=%s\"), path);\n+\t\treturn -1;\n+\t}\n+\t\n+\tstrbuf_addstr(&entry, \"submodule.\");\n+\tstrbuf_addstr(&entry, submodule->name);\n+\tstrbuf_addstr(&entry, \".url\");\n+\tret = config_set_in_gitmodules_file_gently(entry.buf, newurl);\n+\tstrbuf_release(&entry);\n+\t\n+\treturn ret;\n+}\n+\n /*\n * Try to remove the \"submodule.\" section from .gitmodules where the given\n * path is configured. Return 0 only if a .gitmodules file was found, a section\ndiff --git a/submodule.h b/submodule.h\nindex 4dad649f94..ea09480433 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -49,6 +49,8 @@ void set_diffopt_flags_from_submodule_config(struct diff_options *,\n \t\t\t\t\t const char *path);\n int git_default_submodule_config(const char *var, const char *value, void *cb);\n \n+int update_url_in_gitmodules(const char* path, const char *newurl);\n+\n struct option;\n int option_parse_recurse_submodules_worktree_updater(const struct option *opt,\n \t\t\t\t\t\t const char *arg, int unset);\n-- \n2.20.1\n\n"},{"id":"395623","messageId":"20200416210456.19122-3-shouryashukla.oo@gmail.com","threadId":"53246","inReplyTo":"20200416210456.19122-1-shouryashukla.oo@gmail.com","subject":"[PATCH 2/2] submodule: port subcommand 'set-url' from shell to C","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-04-16T21:04:56Z","receivedAt":"2020-04-16T21:05:35Z","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\nSigned-off-by: Shourya Shukla \n---\n builtin/submodule--helper.c | 38 +++++++++++++++++++++++++++++++++++++\n git-submodule.sh | 23 ++--------------------\n 2 files changed, 40 insertions(+), 21 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 1a4b391c88..10e774a5c9 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2246,6 +2246,43 @@ 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+\tconst char *newurl = NULL;\n+\tconst char *path = NULL;\n+\n+\tstruct pathspec pathspec;\n+\tstruct module_list list;\n+\n+\tint quiet = 0;\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 [--quiet] set-url [--] \"),\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+\tpath = argv[1];\n+\tnewurl = argv[2];\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+\tif (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)\n+\t\treturn 1;\n+\n+\treturn update_url_in_gitmodules(path, newurl);\n+}\n+\n #define SUPPORT_SUPER_PREFIX (1<<0)\n \n struct cmd_struct {\n@@ -2276,6 +2313,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..f0304c3e19 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -783,6 +783,7 @@ 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@@ -803,27 +804,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.20.1\n\n"},{"id":"395673","messageId":"CAP8UFD1y7D_gSharBE9+CVyQKg3qhXMvtzBjkdYA1Aj3hocBMg@mail.gmail.com","threadId":"53246","inReplyTo":"20200416210456.19122-1-shouryashukla.oo@gmail.com","subject":"Re: [RFC][PATCH 0/2] submodule: port 'set-url' from shell to C","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-04-17T22:09:21Z","receivedAt":"2020-04-17T22:09:36Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi,\n\nOn Thu, Apr 16, 2020 at 11:05 PM Shourya Shukla\n wrote:\n\n> This is my very first attempt at the conversion of subcommand 'set-url' from shell\n> to C, thus making the subcommand a builtin.\n>\n> I have based my conversion by looking at the way other subcommands have been implemented\n> in 'submodule--helper.c' as well as the subcommand 'set-url' in 'remote.c'. The approach\n> I have taken is as follows:\n>\n> 1. Create a helper function 'update_url_in_gitmodules()' in 'submodule.c'\n> to update the URL of an entry in '.gitmodules'.\n\nWe usually don't add a function that is unused in the commit that adds it.\n\n> 2. Port the function 'cmd_set_url()' in 'git-submodule.sh' to 'module_set_url()'\n> in 'submodule--helper.c'.\n\nSo it seems to me that you could squash the two commits into one.\n\nThanks,\nChristian.\n"},{"id":"395674","messageId":"CAP8UFD2c_QtS=KWY8rwqMOu1A-KLVE_3dfiCUopWT-W60gg8eA@mail.gmail.com","threadId":"53246","inReplyTo":"20200416210456.19122-3-shouryashukla.oo@gmail.com","subject":"Re: [PATCH 2/2] submodule: port subcommand 'set-url' from shell to C","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-04-17T22:13:32Z","receivedAt":"2020-04-17T22:13:46Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thu, Apr 16, 2020 at 11:05 PM Shourya Shukla\n wrote:\n\n> - git submodule--helper config submodule.\"$name\".url \"$url\"\n> - git submodule--helper sync ${GIT_QUIET:+--quiet} \"$name\"\n> + git ${wt_prefix:+-C \"$wt_prefix\"} ${prefix:+--super-prefix \"$prefix\"} submodule--helper set-url ${GIT_QUIET:+--quiet} -- \"$@\"\n\nMaybe I missed something but I see nothing in the C code that does\nwhat the `git submodule--helper sync ...` call does. Maybe for now it\nwould work better if that call was kept after the the call to `git ...\nsubmodule--helper set-url ...`\n"},{"id":"395732","messageId":"20200420081902.GA202848@generichostname","threadId":"53246","inReplyTo":"20200416210456.19122-1-shouryashukla.oo@gmail.com","subject":"Re: [RFC][PATCH 0/2] submodule: port 'set-url' from shell to C","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-04-20T08:19:02Z","receivedAt":"2020-04-20T08:19:12Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Shourya,\n\nOn Fri, Apr 17, 2020 at 02:34:54AM +0530, Shourya Shukla wrote:\n> The issues I am facing are:\n> \n> 1. The patch fails test #2 in t7420, i.e., the test to verify the working of 'set-url'\n> subcommand.\n\nThe 'set-url' command implicitly runs sync once it is changed. I would\ngo further than what Christian suggests and just call sync_submodule()\n(in C) at the end of module_set_url().\n\n> 2. Though not an issue affecting the patch, but the 'usage' prompt of 'git submodule'\n> does not show the subcommands 'set-url' and 'set-branch'.\n\nHmmm, the first of the pair, 'set-branch', was introduced in v2.22.0. It\nseems like you're running an older version of Git, 2.20.1. Stupid\nquestion but are you sure you're running the correct git? For me this\nruns correctly on the latest 'master':\n\n\t$ ./bin-wrappers/git submodule -h\n\tusage: git submodule [--quiet] [--cached]\n\t or: git submodule [--quiet] add [-b ] [-f|--force] [--name ] [--reference ] [--] []\n\t or: git submodule [--quiet] status [--cached] [--recursive] [--] [...]\n\t or: git submodule [--quiet] init [--] [...]\n\t or: git submodule [--quiet] deinit [-f|--force] (--all| [--] ...)\n\t or: git submodule [--quiet] update [--init] [--remote] [-N|--no-fetch] [-f|--force] [--checkout|--merge|--rebase] [--[no-]recommend-shallow] [--reference ] [--recursive] [--[no-]single-branch] [--] [...]\n\t or: git submodule [--quiet] set-branch (--default|--branch ) [--] \n\t or: git submodule [--quiet] set-url [--] \n\t or: git submodule [--quiet] summary [--cached|--files] [--summary-limit ] [commit] [--] [...]\n\t or: git submodule [--quiet] foreach [--recursive] \n\t or: git submodule [--quiet] sync [--recursive] [--] [...]\n\t or: git submodule [--quiet] absorbgitdirs [--] [...]\n\nThanks,\n\nDenton\n"},{"id":"396318","messageId":"20200427071920.GA23846@konoha","threadId":"53246","inReplyTo":"20200420081902.GA202848@generichostname","subject":"Re: [RFC][PATCH 0/2] submodule: port 'set-url' from shell to C","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-04-27T07:19:20Z","receivedAt":"2020-04-27T07:19:30Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"On 20/04 04:19, Denton Liu wrote:\n> > 1. The patch fails test #2 in t7420, i.e., the test to verify the working of 'set-url'\n> > subcommand.\n> \n> The 'set-url' command implicitly runs sync once it is changed. I would\n> go further than what Christian suggests and just call sync_submodule()\n> (in C) at the end of module_set_url().\n\nI have implemented this, yet running the test still gives an error. The\nfunction 'init_pathspec_item' in 'pathspec.c' tends to fail. Going\nfurther deep, the function 'prefix_path_gently' fails. I think this is\nhappening because of the relative path outside the superproject for\n'../newsubmodule' and hence it throws a problem with prefixing.\n\nThe exact function responsible here is 'normalize_path_copy' in\n'path.c'.\n\nOn doing '-i -v' while running the test. The problem comes down to:\n\nfatal: ../newsubmodule: '../newsubmodule' is outside repository at '/git/t/trash directory.t7420-submodule-set-url/super'\n\nWhat exactly is wrong here and how should this problem\nand similar ones (if encountered) be approached? Do we follow\ncertain procedures when debugging problems at such an intricate level?\n\nRegards,\nShourya Shukla\n"},{"id":"396320","messageId":"20200427101720.GA649724@generichostname","threadId":"53246","inReplyTo":"20200427071920.GA23846@konoha","subject":"Re: [RFC][PATCH 0/2] submodule: port 'set-url' from shell to C","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-04-27T10:17:20Z","receivedAt":"2020-04-27T10:17:26Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Shourya,\n\nOn Mon, Apr 27, 2020 at 12:49:20PM +0530, Shourya Shukla wrote:\n> On 20/04 04:19, Denton Liu wrote:\n> > > 1. The patch fails test #2 in t7420, i.e., the test to verify the working of 'set-url'\n> > > subcommand.\n> > \n> > The 'set-url' command implicitly runs sync once it is changed. I would\n> > go further than what Christian suggests and just call sync_submodule()\n> > (in C) at the end of module_set_url().\n> \n> I have implemented this, yet running the test still gives an error. The\n> function 'init_pathspec_item' in 'pathspec.c' tends to fail. Going\n> further deep, the function 'prefix_path_gently' fails. I think this is\n> happening because of the relative path outside the superproject for\n> '../newsubmodule' and hence it throws a problem with prefixing.\n\nI can't think of a reason why Git should be trying to do anything with\nthe path `../newsubmodule`. The set-url part should only write that\nstring into the `.gitmodules` and the sync part should only write into\n`.git/config`. Neither of these steps should actually access the\nsubmodule repo in any way.\n\n> The exact function responsible here is 'normalize_path_copy' in\n> 'path.c'.\n> \n> On doing '-i -v' while running the test. The problem comes down to:\n> \n> fatal: ../newsubmodule: '../newsubmodule' is outside repository at '/git/t/trash directory.t7420-submodule-set-url/super'\n\nPerhaps it's failing at the `git submodule update --remote` part? It's\nhard to tell without more information. One thing you can do is try\nrunning the test with `-x` which would enable shell tracing. This gives\neven more information on the test that's running, including exactly\nwhich command is failing.\n\n> What exactly is wrong here and how should this problem\n> and similar ones (if encountered) be approached? Do we follow\n> certain procedures when debugging problems at such an intricate level?\n\nI can't tell exactly what's wrong from the information you've provided.\nDo you have link to the code you're working on? Alternatively, it might\nbe a good idea to post a RFC patch onto this list for wider discussion.\n\nSome general debugging techniques that I've found helpful include\n\n\t* debug prints using fprintf(stderr, ...)\n\n\t* `-x` for test scripts (as mentioned above)\n\n\t* `debug` in test scripts (which launches GDB)\n\n\t* using `GIT_DEBUGGER=1 ./bin-wrappers/git ` (which\n\t also launches GDB)\n\nalthough I'm not quite sure how applicable they are to your case. Maybe\nit would be helpful for you to grab a stack trace of where the die() is\nbeing called in GDB to understand the cause of the problem?\n\n-Denton\n\n> Regards,\n> Shourya Shukla\n"}]}