git submodule deinit resulting in BUG: builtin/submodule--helper.c:1045: module_list_compute should not choke on empty pathspec

git submodule deinit resulting in BUG: builtin/submodule--helper.c:1045: module_list_compute should not choke on empty pathspec

[Peter Oberndorfer]

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 <file>..." 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?

Re: git submodule deinit resulting in BUG: builtin/submodule--helper.c:1045: module_list_compute should not choke on empty pathspec

[Stefan Beller]

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

[PATCH] submodule deinit: handle non existing pathspecs gracefully

[Stefan Beller]

This fixes a regression introduced in 22e612731b5 (submodule: port
submodule subcommand 'deinit' from shell to C, 2018-01-15), when handling
pathspecs that do not exist gracefully. This restores the historic behavior
of reporting the pathspec as unknown and returning instead of reporting a
bug.

Reported-by: Peter Oberndorfer <kumbayo84@arcor.de>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/submodule--helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index ee020d4749..6ba8587b6d 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1042,7 +1042,7 @@ static int module_deinit(int argc, const char **argv, const char *prefix)
 		die(_("Use '--all' if you really want to deinitialize all submodules"));
 
 	if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)
-		BUG("module_list_compute should not choke on empty pathspec");
+		return 1;
 
 	info.prefix = prefix;
 	if (quiet)
-- 
2.17.0.rc1.321.gba9d0f2565-goog

Re: [PATCH] submodule deinit: handle non existing pathspecs gracefully

[Martin Ågren]

On 28 March 2018 at 01:28, Stefan Beller <sbeller@google.com> wrote:
> This fixes a regression introduced in 22e612731b5 (submodule: port

s/22/2/

> submodule subcommand 'deinit' from shell to C, 2018-01-15), when handling
> pathspecs that do not exist gracefully. This restores the historic behavior
> of reporting the pathspec as unknown and returning instead of reporting a
> bug.

Re: [PATCH] submodule deinit: handle non existing pathspecs gracefully

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> This fixes a regression introduced in 22e612731b5 (submodule: port

s/22e/2e/, I think.

> submodule subcommand 'deinit' from shell to C, 2018-01-15), when handling
> pathspecs that do not exist gracefully. This restores the historic behavior
> of reporting the pathspec as unknown and returning instead of reporting a
> bug.
>
> Reported-by: Peter Oberndorfer <kumbayo84@arcor.de>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  builtin/submodule--helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

It seems that all the other callersof module-list expect that a
negative return from the function is a normal "nothing to do"
condition and returns 1, and this patch makes the oddball "deinit"
do the same.

Sounds good.  Will queue.

Thanks.


>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index ee020d4749..6ba8587b6d 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -1042,7 +1042,7 @@ static int module_deinit(int argc, const char **argv, const char *prefix)
>  		die(_("Use '--all' if you really want to deinitialize all submodules"));
>  
>  	if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)
> -		BUG("module_list_compute should not choke on empty pathspec");
> +		return 1;
>  
>  	info.prefix = prefix;
>  	if (quiet)

Re: git submodule deinit resulting in BUG: builtin/submodule--helper.c:1045: module_list_compute should not choke on empty pathspec

[Peter Oberndorfer]

On 2018-03-28 00:56, Stefan Beller wrote:
> On Tue, Mar 27, 2018 at 12:55 PM Peter Oberndorfer <kumbayo84@arcor.de>
> 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

[Stefan Beller]

On Wed, Mar 28, 2018 at 12:37 PM, Peter Oberndorfer <kumbayo84@arcor.de> wrote:

>>> 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