{"thread":{"id":"48166","subject":"git submodule deinit resulting in BUG: builtin/submodule--helper.c:1045: module_list_compute should not choke on empty pathspec","startedAt":"2018-03-27T19:55:47Z","lastAt":"2018-03-28T21:20:01Z","messageCount":7,"participants":["Peter Oberndorfer","Stefan Beller","Martin Ågren","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"343167","messageId":"9e22b49e-6732-17c7-76fe-0ce241787db9@arcor.de","threadId":"48166","inReplyTo":null,"subject":"git submodule deinit resulting in BUG: builtin/submodule--helper.c:1045: module_list_compute should not choke on empty pathspec","fromName":"Peter Oberndorfer","fromEmail":"kumbayo84@arcor.de","sentAt":"2018-03-27T19:48:43Z","receivedAt":"2018-03-27T19:55:47Z","isPatch":false,"sender":{"key":"kumbayo84@arcor.de","avatar":"https://avatars.githubusercontent.com/u/1041267?v=4"},"body":"Hi,\n\ni tried to run \"git submodule deinit xxx\"\non a submodule that was recently removed from the Rust project.\nBut git responded with a BUG/Core dump (and also did not remove the submodule directory from the checkout).\n\n~/src/rust/rust$ git submodule deinit src/rt/hoedown/\nerror: pathspec 'src/rt/hoedown/' did not match any file(s) known to git.\nBUG: builtin/submodule--helper.c:1045: module_list_compute should not choke on empty pathspec\nAborted (core dumped)\n\nI had a short look at submodule--helper.c and module_list_compute() is called from multiple places.\nMost of them handle failure by return 1;\nOnly module_deinit() seems to calls BUG() on failure.\n\nThis leaves me with 2 questions:\n1) Should this code path just ignore the error and also return 1 like other code paths?\n2) Should \"git submodule deinit\" work on submodules that were removed by upstream already?\n\nFor more debugging information please see below.\n\nThanks,\nGreetings Peter\n\n\n\n~/src/rust/rust$ git --version\ngit version 2.17.0.rc1.47.g9f57127417.dirty\n(this should basically be 90bbd502d54fe920356fa9278055dc9c9bfe9a56 + some Makefile adjustments)\n\nGit Gui reports\nsrc/rt/hoedown\nUntracked, not staged\n* Git Repository (subproject)\n\n\n~/src/rust/rust$ git status\nOn branch fix_literal_attribute_doc\nUntracked files:\n  (use \"git add <file>...\" to include in what will be committed)\n\n        src/rt/\n\n\n~/src/rust/rust$ cat .git/config\n...\n[submodule \"src/rt/hoedown\"]\n        url = https://github.com/rust-lang/hoedown.git\n...\n-> there is no \"active = true\" in this hoedown section\nwhich is present on some (not all) other submodules\n\n\n~/src/rust/rust$ cat .gitmodules\n-> does not contain any references to hoedown anymore as they were remove by upstream\n\n\n~/src/rust/rust$ cat src/rt/hoedown/.git\ngitdir: ../../../.git/modules/src/rt/hoedown\n\n\n~/src/rust/rust/src/rt/hoedown$ git status\nHEAD detached at da282f1\nnothing to commit, working tree clean\n\n-> so there is a working git repository at src/rt/hoedown\n\n\n~/src/rust/rust$ git submodule status\n 9b2dcac06c3e23235f8997b3c5f2325a6d3382df src/dlmalloc (heads/master)\n b889e1e30c5e9953834aa9fa6c982bb28df46ac9 src/doc/book (remotes/origin/ch10-edits-137-gb889e1e3)\n 6a8f0a27e9a58c55c89d07bc43a176fdae5e051c src/doc/nomicon (remotes/origin/HEAD)\n 76296346e97c3702974d3398fdb94af9e10111a2 src/doc/reference (remotes/origin/HEAD)\n d5ec87eabe5733cc2348c7dada89fc67c086f391 src/doc/rust-by-example (remotes/origin/HEAD)\n 1f5a28755e301ac581e2048011e4e0ff3da482ef src/jemalloc (3.6.0-775-g1f5a2875)\n 263a703b10351d8930e48045b4fd09768991b867 src/libcompiler_builtins (remotes/origin/auto-10-g263a703)\n ed04152aacf5b4798f78ff13396f3c04c0a77144 src/liblibc (0.2.37-29-ged04152aac)\n 6ceaaa4b0176a200e4bbd347d6a991ab6c776ede src/llvm (remotes/origin/rust-llvm-release-6-0-0)\n-2717444753318e461e0c3b30dacd03ffbac96903 src/llvm-emscripten\n bcb720e55861c38db47f2ebdf26b7198338cb39d src/stdsimd ((null))\n 311a5eda6f90d660bb23e97c8ee77090519b9eda src/tools/cargo (0.14.0-2144-g311a5eda)\n eafd09010815da43302ac947afee45b0f5219e6b src/tools/clippy (v0.0.189-21-geafd0901)\n b87873eaceb75cf9342d5273f01ba2c020f61ca8 src/tools/lld ((null))\n d4712ca37500f26bbcbf97edcb27820717f769f7 src/tools/miri (remotes/origin/hack_branch_for_miri_do_not_delete_until_merged)\n f5a0c91a39368395b1c1ad322e04be7b6074bc65 src/tools/rls (0.125-131-gf5a0c91)\n 118e078c5badd520d18b92813fd88789c8d341ab src/tools/rust-installer (remotes/origin/HEAD)\n 374dba833e22cc8df8e16e19cccbde61c69d9aed src/tools/rustfmt (0.4.1-35-g374dba83)\n\n-> strangely I get (null) for the current branch/commit in some submodules?\n"},{"id":"343181","messageId":"CAGZ79kYGY5bjh0WPQh7xkXQxLkB9EQ-OcJhVuGE8YUnwmvk2Fg@mail.gmail.com","threadId":"48166","inReplyTo":"9e22b49e-6732-17c7-76fe-0ce241787db9@arcor.de","subject":"Re: git submodule deinit resulting in BUG: builtin/submodule--helper.c:1045: module_list_compute should not choke on empty pathspec","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-03-27T22:56:42Z","receivedAt":"2018-03-27T22:57:00Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Mar 27, 2018 at 12:55 PM Peter Oberndorfer <kumbayo84@arcor.de>\nwrote:\n\n> Hi,\n\n> i tried to run \"git submodule deinit xxx\"\n> on a submodule that was recently removed from the Rust project.\n> But git responded with a BUG/Core dump (and also did not remove the\nsubmodule directory from the checkout).\n\n> ~/src/rust/rust$ git submodule deinit src/rt/hoedown/\n> error: pathspec 'src/rt/hoedown/' did not match any file(s) known to git.\n> BUG: builtin/submodule--helper.c:1045: module_list_compute should not\nchoke on empty pathspec\n> Aborted (core dumped)\n\n> I had a short look at submodule--helper.c and module_list_compute() is\ncalled from multiple places.\n> Most of them handle failure by return 1;\n> Only module_deinit() seems to calls BUG() on failure.\n\nThanks for the analysis!\n\n> This leaves me with 2 questions:\n> 1) Should this code path just ignore the error and also return 1 like\nother code paths?\n\nThis would be a sensible thing to do. I would think.\nI just checked out v2.0.0 (an ancient version, way before the efforts to\nrewrite\ngit-submodule in C were taking off) and there we can do\n\n     $ git submodule deinit gerrit-gpg-asdf/\n     ignoring UNTR extension\n     error: pathspec 'gerrit-gpg-asdf/' did not match any file(s) known to\ngit.\n     Did you forget to 'git add'?\n     $ echo $?\n     1\n\n(The warning about the UNTR extension can be ignored that was introduced\nlater).\nBut the important part is that we get the same error for the missing\npathspec.\nThe next line (\"Did you forget to git-add?\") comes from git-ls-files which\nat the time\nwas invoked by module_list() implemented in shell. I would think we can\nlive without\nthat line. So to fix the segfault, we can just s/BUG(..)/return 1/ as you\nsuggest.\n\n> 2) Should \"git submodule deinit\" work on submodules that were removed by\nupstream already?\n\nTo answer the question \"Is this a submodule that upstream removed\n(recently)?\"\nwe'd have to put in some effort, essentially checking if that was ever a\nsubmodule\n(and not a directory or file).\n\nWhen using \"git pull --recurse-submodules\" the submodule ought to be removed\nautomatically.\n\nWhen doing a fetch && merge manually, we may want to teach merge to remove\na submodule that we have locally upon merge, too.\n\nI view the git-submodule command as a bare bones plumbing helper, that we'd\nwant\nto deprecate eventually as all other higher level commands will know how to\ndeal\nwith submodules.\n\nSo I think we do not want to teach \"git submodule deinit\" to remove dormant\nrepositories, that were submodules removed by upstream already.\n\n> ~/src/rust/rust$ git submodule status\n...\n>   b87873eaceb75cf9342d5273f01ba2c020f61ca8 src/tools/lld ((null))\n\n> -> strangely I get (null) for the current branch/commit in some\nsubmodules?\n\nThis sounds like (3). Looking into that.\n\nThanks,\nStefan\n"},{"id":"343189","messageId":"20180327232824.112539-1-sbeller@google.com","threadId":"48166","inReplyTo":"CAGZ79kYGY5bjh0WPQh7xkXQxLkB9EQ-OcJhVuGE8YUnwmvk2Fg@mail.gmail.com","subject":"[PATCH] submodule deinit: handle non existing pathspecs gracefully","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-03-27T23:28:24Z","receivedAt":"2018-03-27T23:28:33Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This fixes a regression introduced in 22e612731b5 (submodule: port\nsubmodule subcommand 'deinit' from shell to C, 2018-01-15), when handling\npathspecs that do not exist gracefully. This restores the historic behavior\nof reporting the pathspec as unknown and returning instead of reporting a\nbug.\n\nReported-by: Peter Oberndorfer <kumbayo84@arcor.de>\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n builtin/submodule--helper.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex ee020d4749..6ba8587b6d 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1042,7 +1042,7 @@ static int module_deinit(int argc, const char **argv, const char *prefix)\n \t\tdie(_(\"Use '--all' if you really want to deinitialize all submodules\"));\n \n \tif (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)\n-\t\tBUG(\"module_list_compute should not choke on empty pathspec\");\n+\t\treturn 1;\n \n \tinfo.prefix = prefix;\n \tif (quiet)\n-- \n2.17.0.rc1.321.gba9d0f2565-goog\n\n"},{"id":"343203","messageId":"CAN0heSpG9zq1bFawid7MoaVQ--WXPv=cXduhjjV=Q3kcK2TMeQ@mail.gmail.com","threadId":"48166","inReplyTo":"20180327232824.112539-1-sbeller@google.com","subject":"Re: [PATCH] submodule deinit: handle non existing pathspecs gracefully","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-03-28T04:09:00Z","receivedAt":"2018-03-28T04:09:06Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 28 March 2018 at 01:28, Stefan Beller <sbeller@google.com> wrote:\n> This fixes a regression introduced in 22e612731b5 (submodule: port\n\ns/22/2/\n\n> submodule subcommand 'deinit' from shell to C, 2018-01-15), when handling\n> pathspecs that do not exist gracefully. This restores the historic behavior\n> of reporting the pathspec as unknown and returning instead of reporting a\n> bug.\n"},{"id":"343205","messageId":"xmqq4ll0aiw6.fsf@gitster-ct.c.googlers.com","threadId":"48166","inReplyTo":"20180327232824.112539-1-sbeller@google.com","subject":"Re: [PATCH] submodule deinit: handle non existing pathspecs gracefully","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-28T05:06:33Z","receivedAt":"2018-03-28T05:06:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> This fixes a regression introduced in 22e612731b5 (submodule: port\n\ns/22e/2e/, I think.\n\n> submodule subcommand 'deinit' from shell to C, 2018-01-15), when handling\n> pathspecs that do not exist gracefully. This restores the historic behavior\n> of reporting the pathspec as unknown and returning instead of reporting a\n> bug.\n>\n> Reported-by: Peter Oberndorfer <kumbayo84@arcor.de>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>  builtin/submodule--helper.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n\nIt seems that all the other callersof module-list expect that a\nnegative return from the function is a normal \"nothing to do\"\ncondition and returns 1, and this patch makes the oddball \"deinit\"\ndo the same.\n\nSounds good.  Will queue.\n\nThanks.\n\n\n>\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index ee020d4749..6ba8587b6d 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -1042,7 +1042,7 @@ static int module_deinit(int argc, const char **argv, const char *prefix)\n>  \t\tdie(_(\"Use '--all' if you really want to deinitialize all submodules\"));\n>  \n>  \tif (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)\n> -\t\tBUG(\"module_list_compute should not choke on empty pathspec\");\n> +\t\treturn 1;\n>  \n>  \tinfo.prefix = prefix;\n>  \tif (quiet)\n"},{"id":"343266","messageId":"9ead5ee1-9d4a-38f6-0fa3-ca4c982b33f5@arcor.de","threadId":"48166","inReplyTo":"CAGZ79kYGY5bjh0WPQh7xkXQxLkB9EQ-OcJhVuGE8YUnwmvk2Fg@mail.gmail.com","subject":"Re: git submodule deinit resulting in BUG: builtin/submodule--helper.c:1045: module_list_compute should not choke on empty pathspec","fromName":"Peter Oberndorfer","fromEmail":"kumbayo84@arcor.de","sentAt":"2018-03-28T19:37:19Z","receivedAt":"2018-03-28T19:37:37Z","isPatch":false,"sender":{"key":"kumbayo84@arcor.de","avatar":"https://avatars.githubusercontent.com/u/1041267?v=4"},"body":"On 2018-03-28 00:56, Stefan Beller wrote:\n> On Tue, Mar 27, 2018 at 12:55 PM Peter Oberndorfer <kumbayo84@arcor.de>\n> wrote:\n\nHi,\n\nas expected your patch fixed the BUG output.\nThanks!\n\n>> 2) Should \"git submodule deinit\" work on submodules that were removed by\n> upstream already?\n> \n> To answer the question \"Is this a submodule that upstream removed\n> (recently)?\"\n> we'd have to put in some effort, essentially checking if that was ever a\n> submodule\n> (and not a directory or file).\n> \n\nHmm, yeah looks a bit more complicated than I initially imagined\nsince submodules can have a name that's different from their path.\nAnd after the rebase, the name <-> path mapping via .gitmodules is not available anymore.\n\nNaively I think it could work the following way:\n* Either iterate over all submodules in .git/modules/ and check their config\n  has a worktree = \"../../path\" that resolves to the submodule path we want to remove.\n* Or check the \"gitlink:\" path in submodule/.git if it points to our .git/modules/\nThen if .git/config contains a [submodule \"name\"] entry\nwe should have a pretty good idea if this folder contains a stale submodule.\n\n> When using \"git pull --recurse-submodules\" the submodule ought to be removed\n> automatically.\n> \n> When doing a fetch && merge manually, we may want to teach merge to remove\n> a submodule that we have locally upon merge, too.\n> \n\nYeah that would be nice :-)\nIn my case I updated the repository via a rebase, so that would also have to be covered.\n\n> I view the git-submodule command as a bare bones plumbing helper, that we'd\n> want\n> to deprecate eventually as all other higher level commands will know how to\n> deal\n> with submodules.\n> \n> So I think we do not want to teach \"git submodule deinit\" to remove dormant\n> repositories, that were submodules removed by upstream already.\n> \n\nMy gut feeling makes me expect the following:\n* It would be nice if such stale submodules showed up in \"git submodule status\" or \"git status\"\n  Now \"git submodule\" shows nothing related to this stale submodule\n  Now \"git status\" shows  Untracked files: src/rt which is a bit confusing as the actual submodule is in src/rt/hoedown\n  Now \"Git gui\" shows src/rt/hoedown as untracked git repository\n* There should be an easy(and safe) way for the user to deinit such a submodule\n  if if the automatic submodule updating during a merge/rebase was not enabled or somehow failed.\n(Minus the problem of somebody having to actually do the work...)\n\n>> ~/src/rust/rust$ git submodule status\n> ...\n>>   b87873eaceb75cf9342d5273f01ba2c020f61ca8 src/tools/lld ((null))\n> \n>> -> strangely I get (null) for the current branch/commit in some\n> submodules?\n> \n> This sounds like (3). Looking into that.\n\nSorry, what do you mean by (3)?\n\nThanks,\nGreetings Peter\n"},{"id":"343277","messageId":"CAGZ79kZ326BGMuNNDTepN=_9j35Tu+zUACHKK67m+dhz7MFpMQ@mail.gmail.com","threadId":"48166","inReplyTo":"9ead5ee1-9d4a-38f6-0fa3-ca4c982b33f5@arcor.de","subject":"Re: git submodule deinit resulting in BUG: builtin/submodule--helper.c:1045: module_list_compute should not choke on empty pathspec","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-03-28T21:19:54Z","receivedAt":"2018-03-28T21:20:01Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Mar 28, 2018 at 12:37 PM, Peter Oberndorfer <kumbayo84@arcor.de> wrote:\n\n>>> 2) Should \"git submodule deinit\" work on submodules that were removed by\n>> upstream already?\n>>\n>> To answer the question \"Is this a submodule that upstream removed\n>> (recently)?\"\n>> we'd have to put in some effort, essentially checking if that was ever a\n>> submodule\n>> (and not a directory or file).\n>>\n>\n> Hmm, yeah looks a bit more complicated than I initially imagined\n> since submodules can have a name that's different from their path.\n> And after the rebase, the name <-> path mapping via .gitmodules is not available anymore.\n>\n> Naively I think it could work the following way:\n> * Either iterate over all submodules in .git/modules/ and check their config\n>   has a worktree = \"../../path\" that resolves to the submodule path we want to remove.\n\nThis would work but scales linearly with the number of submodules.\n\n\n> * Or check the \"gitlink:\" path in submodule/.git if it points to our .git/modules/\n> Then if .git/config contains a [submodule \"name\"] entry\n> we should have a pretty good idea if this folder contains a stale submodule.\n\nIf you move a submodule a directory up or down, the relative path is not exact\nany more, we'd need to check for the last part to loosely match.\n\n\n>> When using \"git pull --recurse-submodules\" the submodule ought to be removed\n>> automatically.\n>>\n>> When doing a fetch && merge manually, we may want to teach merge to remove\n>> a submodule that we have locally upon merge, too.\n>>\n>\n> Yeah that would be nice :-)\n> In my case I updated the repository via a rebase, so that would also have to be covered.\n\nOh rebase itself has not yet learned about recursion into submodules.\n(\"git pull --rebase --recurse-submodules\" is a thing though)\n\n>> I view the git-submodule command as a bare bones plumbing helper, that we'd\n>> want\n>> to deprecate eventually as all other higher level commands will know how to\n>> deal\n>> with submodules.\n>>\n>> So I think we do not want to teach \"git submodule deinit\" to remove dormant\n>> repositories, that were submodules removed by upstream already.\n>>\n>\n> My gut feeling makes me expect the following:\n> * It would be nice if such stale submodules showed up in \"git submodule status\" or \"git status\"\n>   Now \"git submodule\" shows nothing related to this stale submodule\n\nThat has currently only two ways \"+\" or \"-\" for there/not there.\nMaybe we'd need to add some characters similar to \"git status --porcelain\"\nsuch as \"?\"\n\n>   Now \"git status\" shows  Untracked files: src/rt which is a bit confusing as the actual submodule is in src/rt/hoedown\n>   Now \"Git gui\" shows src/rt/hoedown as untracked git repository\n\nhm. The current state of affairs doesn't sound intriguing.\nThough, I think we'd want to step back one more step and rather want\nto ask how a dormant submodule comes into existence, instead of\njust improving the reporting. Reportingthem is of course also important,\nbut in the long run I'd rather want to have situations like these happen\nless often. When upstream deletes a file, they are also not required to be\ndeleted manually, but merge/checkout would take care of them.\n\n> * There should be an easy(and safe) way for the user to deinit such a submodule\n>   if if the automatic submodule updating during a merge/rebase was not enabled or somehow failed.\n> (Minus the problem of somebody having to actually do the work...)\n>\n>>> ~/src/rust/rust$ git submodule status\n>> ...\n>>>   b87873eaceb75cf9342d5273f01ba2c020f61ca8 src/tools/lld ((null))\n>>\n>>> -> strangely I get (null) for the current branch/commit in some\n>> submodules?\n>>\n>> This sounds like (3). Looking into that.\n>\n> Sorry, what do you mean by (3)?\n\nI meant the ((null)) issue is another third thought that we can\ndiscuss separately,\nslightly unrelated to the others (that you marked as (1) and (2))\n\nThanks,\nStefan\n"}]}