Re: git submodule deinit resulting in BUG: builtin/submodule--helper.c:1045: module_list_compute should not choke on empty pathspec
On Wed, Mar 28, 2018 at 12:37 PM, Peter Oberndorferwrote: >>> 2) Should "git submodule deinit" work on submodules that were removed by >> upstream already? >> >> To answer the question "Is this a submodule that upstream removed >> (recently)?" >> we'd have to put in some effort, essentially checking if that was ever a >> submodule >> (and not a directory or file). >> > > Hmm, yeah looks a bit more complicated than I initially imagined > since submodules can have a name that's different from their path. > And after the rebase, the name <-> path mapping via .gitmodules is not > available anymore. > > Naively I think it could work the following way: > * Either iterate over all submodules in .git/modules/ and check their config > has a worktree = "../../path" that resolves to the submodule path we want > to remove. This would work but scales linearly with the number of submodules. > * Or check the "gitlink:" path in submodule/.git if it points to our > .git/modules/ > Then if .git/config contains a [submodule "name"] entry > we should have a pretty good idea if this folder contains a stale submodule. If you move a submodule a directory up or down, the relative path is not exact any more, we'd need to check for the last part to loosely match. >> When using "git pull --recurse-submodules" the submodule ought to be removed >> automatically. >> >> When doing a fetch && merge manually, we may want to teach merge to remove >> a submodule that we have locally upon merge, too. >> > > Yeah that would be nice :-) > In my case I updated the repository via a rebase, so that would also have to > be covered. Oh rebase itself has not yet learned about recursion into submodules. ("git pull --rebase --recurse-submodules" is a thing though) >> I view the git-submodule command as a bare bones plumbing helper, that we'd >> want >> to deprecate eventually as all other higher level commands will know how to >> deal >> with submodules. >> >> So I think we do not want to teach "git submodule deinit" to remove dormant >> repositories, that were submodules removed by upstream already. >> > > My gut feeling makes me expect the following: > * It would be nice if such stale submodules showed up in "git submodule > status" or "git status" > Now "git submodule" shows nothing related to this stale submodule That has currently only two ways "+" or "-" for there/not there. Maybe we'd need to add some characters similar to "git status --porcelain" such as "?" > Now "git status" shows Untracked files: src/rt which is a bit confusing as > the actual submodule is in src/rt/hoedown > Now "Git gui" shows src/rt/hoedown as untracked git repository hm. The current state of affairs doesn't sound intriguing. Though, I think we'd want to step back one more step and rather want to ask how a dormant submodule comes into existence, instead of just improving the reporting. Reportingthem is of course also important, but in the long run I'd rather want to have situations like these happen less often. When upstream deletes a file, they are also not required to be deleted manually, but merge/checkout would take care of them. > * There should be an easy(and safe) way for the user to deinit such a > submodule > if if the automatic submodule updating during a merge/rebase was not > enabled or somehow failed. > (Minus the problem of somebody having to actually do the work...) > >>> ~/src/rust/rust$ git submodule status >> ... >>> b87873eaceb75cf9342d5273f01ba2c020f61ca8 src/tools/lld ((null)) >> >>> -> strangely I get (null) for the current branch/commit in some >> submodules? >> >> This sounds like (3). Looking into that. > > Sorry, what do you mean by (3)? I meant the ((null)) issue is another third thought that we can discuss separately, slightly unrelated to the others (that you marked as (1) and (2)) Thanks, Stefan
Re: git submodule deinit resulting in BUG: builtin/submodule--helper.c:1045: module_list_compute should not choke on empty pathspec
On 2018-03-28 00:56, Stefan Beller wrote: > On Tue, Mar 27, 2018 at 12:55 PM Peter Oberndorfer> wrote: Hi, as expected your patch fixed the BUG output. Thanks! >> 2) Should "git submodule deinit" work on submodules that were removed by > upstream already? > > To answer the question "Is this a submodule that upstream removed > (recently)?" > we'd have to put in some effort, essentially checking if that was ever a > submodule > (and not a directory or file). > Hmm, yeah looks a bit more complicated than I initially imagined since submodules can have a name that's different from their path. And after the rebase, the name <-> path mapping via .gitmodules is not available anymore. Naively I think it could work the following way: * Either iterate over all submodules in .git/modules/ and check their config has a worktree = "../../path" that resolves to the submodule path we want to remove. * Or check the "gitlink:" path in submodule/.git if it points to our .git/modules/ Then if .git/config contains a [submodule "name"] entry we should have a pretty good idea if this folder contains a stale submodule. > When using "git pull --recurse-submodules" the submodule ought to be removed > automatically. > > When doing a fetch && merge manually, we may want to teach merge to remove > a submodule that we have locally upon merge, too. > Yeah that would be nice :-) In my case I updated the repository via a rebase, so that would also have to be covered. > I view the git-submodule command as a bare bones plumbing helper, that we'd > want > to deprecate eventually as all other higher level commands will know how to > deal > with submodules. > > So I think we do not want to teach "git submodule deinit" to remove dormant > repositories, that were submodules removed by upstream already. > My gut feeling makes me expect the following: * It would be nice if such stale submodules showed up in "git submodule status" or "git status" Now "git submodule" shows nothing related to this stale submodule Now "git status" shows Untracked files: src/rt which is a bit confusing as the actual submodule is in src/rt/hoedown Now "Git gui" shows src/rt/hoedown as untracked git repository * There should be an easy(and safe) way for the user to deinit such a submodule if if the automatic submodule updating during a merge/rebase was not enabled or somehow failed. (Minus the problem of somebody having to actually do the work...) >> ~/src/rust/rust$ git submodule status > ... >> b87873eaceb75cf9342d5273f01ba2c020f61ca8 src/tools/lld ((null)) > >> -> strangely I get (null) for the current branch/commit in some > submodules? > > This sounds like (3). Looking into that. Sorry, what do you mean by (3)? Thanks, Greetings Peter
Re: git submodule deinit resulting in BUG: builtin/submodule--helper.c:1045: module_list_compute should not choke on empty pathspec
On Tue, Mar 27, 2018 at 12:55 PM Peter Oberndorferwrote: > Hi, > i tried to run "git submodule deinit xxx" > on a submodule that was recently removed from the Rust project. > But git responded with a BUG/Core dump (and also did not remove the submodule directory from the checkout). > ~/src/rust/rust$ git submodule deinit src/rt/hoedown/ > error: pathspec 'src/rt/hoedown/' did not match any file(s) known to git. > BUG: builtin/submodule--helper.c:1045: module_list_compute should not choke on empty pathspec > Aborted (core dumped) > I had a short look at submodule--helper.c and module_list_compute() is called from multiple places. > Most of them handle failure by return 1; > Only module_deinit() seems to calls BUG() on failure. Thanks for the analysis! > This leaves me with 2 questions: > 1) Should this code path just ignore the error and also return 1 like other code paths? This would be a sensible thing to do. I would think. I just checked out v2.0.0 (an ancient version, way before the efforts to rewrite git-submodule in C were taking off) and there we can do $ git submodule deinit gerrit-gpg-asdf/ ignoring UNTR extension error: pathspec 'gerrit-gpg-asdf/' did not match any file(s) known to git. Did you forget to 'git add'? $ echo $? 1 (The warning about the UNTR extension can be ignored that was introduced later). But the important part is that we get the same error for the missing pathspec. The next line ("Did you forget to git-add?") comes from git-ls-files which at the time was invoked by module_list() implemented in shell. I would think we can live without that line. So to fix the segfault, we can just s/BUG(..)/return 1/ as you suggest. > 2) Should "git submodule deinit" work on submodules that were removed by upstream already? To answer the question "Is this a submodule that upstream removed (recently)?" we'd have to put in some effort, essentially checking if that was ever a submodule (and not a directory or file). When using "git pull --recurse-submodules" the submodule ought to be removed automatically. When doing a fetch && merge manually, we may want to teach merge to remove a submodule that we have locally upon merge, too. I view the git-submodule command as a bare bones plumbing helper, that we'd want to deprecate eventually as all other higher level commands will know how to deal with submodules. So I think we do not want to teach "git submodule deinit" to remove dormant repositories, that were submodules removed by upstream already. > ~/src/rust/rust$ git submodule status ... > b87873eaceb75cf9342d5273f01ba2c020f61ca8 src/tools/lld ((null)) > -> strangely I get (null) for the current branch/commit in some submodules? This sounds like (3). Looking into that. Thanks, Stefan
git submodule deinit resulting in BUG: builtin/submodule--helper.c:1045: module_list_compute should not choke on empty pathspec
Hi, i tried to run "git submodule deinit xxx" on a submodule that was recently removed from the Rust project. But git responded with a BUG/Core dump (and also did not remove the submodule directory from the checkout). ~/src/rust/rust$ git submodule deinit src/rt/hoedown/ error: pathspec 'src/rt/hoedown/' did not match any file(s) known to git. BUG: builtin/submodule--helper.c:1045: module_list_compute should not choke on empty pathspec Aborted (core dumped) I had a short look at submodule--helper.c and module_list_compute() is called from multiple places. Most of them handle failure by return 1; Only module_deinit() seems to calls BUG() on failure. This leaves me with 2 questions: 1) Should this code path just ignore the error and also return 1 like other code paths? 2) Should "git submodule deinit" work on submodules that were removed by upstream already? For more debugging information please see below. Thanks, Greetings Peter ~/src/rust/rust$ git --version git version 2.17.0.rc1.47.g9f57127417.dirty (this should basically be 90bbd502d54fe920356fa9278055dc9c9bfe9a56 + some Makefile adjustments) Git Gui reports src/rt/hoedown Untracked, not staged * Git Repository (subproject) ~/src/rust/rust$ git status On branch fix_literal_attribute_doc Untracked files: (use "git add ..." to include in what will be committed) src/rt/ ~/src/rust/rust$ cat .git/config ... [submodule "src/rt/hoedown"] url = https://github.com/rust-lang/hoedown.git ... -> there is no "active = true" in this hoedown section which is present on some (not all) other submodules ~/src/rust/rust$ cat .gitmodules -> does not contain any references to hoedown anymore as they were remove by upstream ~/src/rust/rust$ cat src/rt/hoedown/.git gitdir: ../../../.git/modules/src/rt/hoedown ~/src/rust/rust/src/rt/hoedown$ git status HEAD detached at da282f1 nothing to commit, working tree clean -> so there is a working git repository at src/rt/hoedown ~/src/rust/rust$ git submodule status 9b2dcac06c3e23235f8997b3c5f2325a6d3382df src/dlmalloc (heads/master) b889e1e30c5e9953834aa9fa6c982bb28df46ac9 src/doc/book (remotes/origin/ch10-edits-137-gb889e1e3) 6a8f0a27e9a58c55c89d07bc43a176fdae5e051c src/doc/nomicon (remotes/origin/HEAD) 76296346e97c3702974d3398fdb94af9e10111a2 src/doc/reference (remotes/origin/HEAD) d5ec87eabe5733cc2348c7dada89fc67c086f391 src/doc/rust-by-example (remotes/origin/HEAD) 1f5a28755e301ac581e2048011e4e0ff3da482ef src/jemalloc (3.6.0-775-g1f5a2875) 263a703b10351d8930e48045b4fd09768991b867 src/libcompiler_builtins (remotes/origin/auto-10-g263a703) ed04152aacf5b4798f78ff13396f3c04c0a77144 src/liblibc (0.2.37-29-ged04152aac) 6ceaaa4b0176a200e4bbd347d6a991ab6c776ede src/llvm (remotes/origin/rust-llvm-release-6-0-0) -2717444753318e461e0c3b30dacd03ffbac96903 src/llvm-emscripten bcb720e55861c38db47f2ebdf26b7198338cb39d src/stdsimd ((null)) 311a5eda6f90d660bb23e97c8ee77090519b9eda src/tools/cargo (0.14.0-2144-g311a5eda) eafd09010815da43302ac947afee45b0f5219e6b src/tools/clippy (v0.0.189-21-geafd0901) b87873eaceb75cf9342d5273f01ba2c020f61ca8 src/tools/lld ((null)) d4712ca37500f26bbcbf97edcb27820717f769f7 src/tools/miri (remotes/origin/hack_branch_for_miri_do_not_delete_until_merged) f5a0c91a39368395b1c1ad322e04be7b6074bc65 src/tools/rls (0.125-131-gf5a0c91) 118e078c5badd520d18b92813fd88789c8d341ab src/tools/rust-installer (remotes/origin/HEAD) 374dba833e22cc8df8e16e19cccbde61c69d9aed src/tools/rustfmt (0.4.1-35-g374dba83) -> strangely I get (null) for the current branch/commit in some submodules?