On Tue, Mar 27, 2018 at 12:55 PM Peter Oberndorfer <kumbayo84@xxxxxxxx>
wrote:
> 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
