git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
From: Stefan Beller <sbeller@google.com>
To: kumbayo84@arcor.de
Cc: git <git@vger.kernel.org>, Prathamesh Chavan <pc44800@gmail.com>
Subject: Re: git submodule deinit resulting in BUG: builtin/submodule--helper.c:1045: module_list_compute should not choke on empty pathspec
Date: Tue, 27 Mar 2018 22:56:42 +0000
Message-ID: <CAGZ79kYGY5bjh0WPQh7xkXQxLkB9EQ-OcJhVuGE8YUnwmvk2Fg@mail.gmail.com> (raw)
In-Reply-To: <9e22b49e-6732-17c7-76fe-0ce241787db9@arcor.de>

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

  reply index

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-03-27 19:48 Peter Oberndorfer
2018-03-27 22:56 ` Stefan Beller [this message]
2018-03-27 23:28   ` [PATCH] submodule deinit: handle non existing pathspecs gracefully Stefan Beller
2018-03-28  4:09     ` Martin Ågren
2018-03-28  5:06     ` Junio C Hamano
2018-03-28 19:37   ` git submodule deinit resulting in BUG: builtin/submodule--helper.c:1045: module_list_compute should not choke on empty pathspec Peter Oberndorfer
2018-03-28 21:19     ` Stefan Beller

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAGZ79kYGY5bjh0WPQh7xkXQxLkB9EQ-OcJhVuGE8YUnwmvk2Fg@mail.gmail.com \
    --to=sbeller@google.com \
    --cc=git@vger.kernel.org \
    --cc=kumbayo84@arcor.de \
    --cc=pc44800@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox