Skip to content

Error when removing submodule #168

New issue
New issue
Closed
Closed
Error when removing submodule#168
Labels
bug
Milestone
0.24.0

Description

@lencioni
lencioni
opened on Apr 8, 2015

I just ran into an interesting error when trying to make a commit that only removes a submodule. Here's what I did:

git submodule deinit path/to/submodule
git rm path/to/submodule
rm -rf .git/modules/path/to/submodule
git commit

The first time I tried committing, it ran the hooks but there were errors. The second time I tried committing, I get the following output:

Unable to setup environment for pre-commit hook run:
STDOUT:
STDERR:error: path/to/submodule: is a directory - add files inside instead
fatal: Unable to process path path/to/submodule
Cannot save the current worktree state

If I run git status everything looks as I would expect.

Doing this makes things work again:

git reset
git add .gitmodules
git rm path/to/submodule

I'm not really sure if this is a problem with overcommit or with git, but I figured I would start here.

Activity

  1. jawshooah commented on Apr 8, 2015

    @jawshooah
    jawshooah
    on Apr 8, 2015
    Contributor

    Are you using Overcommit v0.23.0, or the latest master? This may have been fixed in #158, as I can't reproduce the error locally.

  2. lencioni commented on Apr 8, 2015

    @lencioni
    lencioni
    on Apr 8, 2015
    ContributorAuthor

    I'm using v0.23.0.

  3. jawshooah commented on Apr 8, 2015

    @jawshooah
    jawshooah
    on Apr 8, 2015
    Contributor

    Whoops, I wasn't recreating the error conditions properly. After forcing AuthorName to fail and attempting to commit again, I see the same error with both v0.23.0 and the latest master.

    Seems like the following sequence triggers the error:

    $ git rm path/to/submodule
rm 'path/to/submodule'

$ git stash save --keep-index
Saved working directory and index state WIP on test-submodule-delete: bc0bb4a Add submodule
HEAD is now at bc0bb4a Add submodule

$ git reset --hard
HEAD is now at bc0bb4a Add submodule

$ git stash pop --index
Removing path/to/submodule
On branch test-submodule-delete
Changes to be committed:
  (use "git reset HEAD <file>..." to unstage)

    modified:   .gitmodules
    deleted:    path/to/submodule

Dropped refs/stash@{0} (43e3071fc1068177c4dadb96d106a35092145b7e)

$ git stash save --keep-index
error: path/to/submodule: is a directory - add files inside instead
fatal: Unable to process path path/to/submodule
Cannot save the current worktree state

    This is the exact sequence that plays out when a pre-commit hook fails, and then runs again. The problem is that git reset --hard restores the path/to/submodule directory, but not its contents, and git stash save doesn't know what to do with the empty directory.

    To sum up, submodules are terrible and no one should ever use them. But I guess this still needs a fix, eh?

  4. jawshooah commented on Apr 8, 2015

    @jawshooah
    jawshooah
    on Apr 8, 2015
    Contributor

    More generally, this error can be reproduced with the following commands:

    git submodule add $SUBMODULE_URL path/to/submodule
git commit -m "Add submodule"
git rm path/to/submodule
mkdir -p path/to/submodule
git stash save
  5. lencioni
    added this to the 0.24.0 milestone on Apr 8, 2015
  6. lencioni
    added
    bug
    on Apr 8, 2015

  7. sds commented on Apr 9, 2015

    @sds
    sds
    on Apr 9, 2015
    Owner

    I believe I've come up with a workaround for this in 5916e2c and 59fa73b. It's definitely more invasive than I would like, so if there are other ideas please suggest them. I'm leaving this open until some tests are written.

  8. sds commented on Apr 9, 2015

    @sds
    sds
    on Apr 9, 2015
    Owner

    It turns out we're running against some differences in behavior between git 1.8 and 2.3 (at least those are the versions I jump between when developing on CentOS versus Mac OS X).

    I've added a test in 25f9e62 with some explanation of the difference in behavior.

    However, I've realized that the separate tests added in 5916e2c fail on git 1.8, since .gitmodules seems to be treated differently between those two versions (haven't looked into this deeply yet).

  9. jawshooah commented on Apr 9, 2015

    @jawshooah
    jawshooah
    on Apr 9, 2015
    Contributor

    Would it be sufficient to test only against 1.8 and the latest release (currently 2.3.5)? If so, we could add something like this to .travis.yml:

    env:
  - GIT_VERSION=1.8
  - GIT_VERSION=latest

before_install:
  - case "$GIT_VERSION" in
    "1.8")
      export GIT_PPA="ppa:git-core/v1.8" ;;

    "latest")
      export GIT_PPA="ppa:git-core/ppa" ;;

    *)
      echo "invalid Git version requested: $GIT_VERSION" ;
      exit 1 ;;
    esac

install:
  - sudo add-apt-repository "$GIT_PPA" -y
  - sudo apt-get update
  - sudo apt-get install git -y

  10. sds commented on Apr 10, 2015

    @sds
    sds
    on Apr 10, 2015
    Owner

    We'd probably want to explicitly test a number of releases. Much of enterprise Linux such as RHEL 6 is still on 1.7, and the relatively new RHEL 7 is on 1.8.

    I'm guessing the largest audience of Overcommit will be Mac users, which means we'll see a wide-range of versions given the ease with which you can install git via homebrew and the relative infrequency with which most developers update it (it's usually a one-time thought when they're setting up a new machine and is never touched after that).

    I'm not sure how much of a pain this will turn out to be in practice, but it seems like supporting the latest patch level of all minor versions 1.7 or newer is probably something a low-level tool like Overcommit needs to do. What do you think?

  11. jawshooah commented on Apr 10, 2015

    @jawshooah
    jawshooah
    on Apr 10, 2015
    Contributor

    Sounds good to me. We just need to find apt repositories for each minor release since 1.7, or else compile from source, which we probably want to avoid.

    This is going to create a huge build matrix, though. 6 supported Ruby versions and 7 supported Git versions gives us 42 builds overall.

  12. jawshooah commented on Apr 10, 2015

    @jawshooah
    jawshooah
    on Apr 10, 2015
    Contributor

    Strange, all tests seem to be passing with both git 1.8.5 and 2.3.5: https://travis-ci.org/jawshooah/overcommit/builds/57886049

  13. sds commented on Apr 10, 2015

    @sds
    sds
    on Apr 10, 2015
    Owner

    I can confirm git_repo_spec.rb fails on git 1.8.3.1.

    $ bundle exec rspec spec/overcommit/git_repo_spec.rb
[Coveralls] Set up the SimpleCov formatter.
[Coveralls] Using SimpleCov's default settings.
..FF

Failures:

  1) Overcommit::GitRepo.staged_submodule_removals when there is one submodule removal staged returns the submodule that was removed
     Failure/Error: subject.size.should == 1
       expected: 1
            got: 0 (using ==)
     # ./spec/overcommit/git_repo_spec.rb:41:in `block (4 levels) in <top (required)>'
     # ./spec/overcommit/git_repo_spec.rb:15:in `block (4 levels) in <top (required)>'
     # ./spec/support/git_spec_helpers.rb:16:in `call'
     # ./spec/support/git_spec_helpers.rb:16:in `block in repo'
     # ./spec/support/git_spec_helpers.rb:27:in `call'
     # ./spec/support/git_spec_helpers.rb:27:in `block (3 levels) in directory'
     # ./spec/support/git_spec_helpers.rb:26:in `chdir'
     # ./spec/support/git_spec_helpers.rb:26:in `block (2 levels) in directory'
     # ./spec/support/git_spec_helpers.rb:24:in `chdir'
     # ./spec/support/git_spec_helpers.rb:24:in `block in directory'
     # ./spec/support/git_spec_helpers.rb:23:in `tap'
     # ./spec/support/git_spec_helpers.rb:23:in `directory'
     # ./spec/support/git_spec_helpers.rb:8:in `repo'
     # ./spec/overcommit/git_repo_spec.rb:12:in `block (3 levels) in <top (required)>'

  2) Overcommit::GitRepo.staged_submodule_removals when there are multiple submodule removals staged returns all submodules that were removed
     Failure/Error: subject.size.should == 2
       expected: 2
            got: 0 (using ==)
     # ./spec/overcommit/git_repo_spec.rb:62:in `block (4 levels) in <top (required)>'
     # ./spec/overcommit/git_repo_spec.rb:15:in `block (4 levels) in <top (required)>'
     # ./spec/support/git_spec_helpers.rb:16:in `call'
     # ./spec/support/git_spec_helpers.rb:16:in `block in repo'
     # ./spec/support/git_spec_helpers.rb:27:in `call'
     # ./spec/support/git_spec_helpers.rb:27:in `block (3 levels) in directory'
     # ./spec/support/git_spec_helpers.rb:26:in `chdir'
     # ./spec/support/git_spec_helpers.rb:26:in `block (2 levels) in directory'
     # ./spec/support/git_spec_helpers.rb:24:in `chdir'
     # ./spec/support/git_spec_helpers.rb:24:in `block in directory'
     # ./spec/support/git_spec_helpers.rb:23:in `tap'
     # ./spec/support/git_spec_helpers.rb:23:in `directory'
     # ./spec/support/git_spec_helpers.rb:8:in `repo'
     # ./spec/overcommit/git_repo_spec.rb:12:in `block (3 levels) in <top (required)>'

Finished in 1.37 seconds (files took 0.47412 seconds to load)
4 examples, 2 failures

Failed examples:

rspec ./spec/overcommit/git_repo_spec.rb:40 # Overcommit::GitRepo.staged_submodule_removals when there is one submodule removal staged returns the submodule that was removed
rspec ./spec/overcommit/git_repo_spec.rb:61 # Overcommit::GitRepo.staged_submodule_removals when there are multiple submodule removals staged returns all submodules that were removed

  14. sds commented on Apr 10, 2015

    @sds
    sds
    on Apr 10, 2015
    Owner

    So much for testing only minor versions? :P (I think we're fine with the policy as defined—I just need to dig into what's going on here)

  15. jawshooah commented on Apr 10, 2015

    @jawshooah
    jawshooah
    on Apr 10, 2015
    Contributor

    Started testing all patch-level releases of Git from 1.8.3.1 onward, seems this was first fixed in 1.8.5. Possibly related to this entry in the release notes.

    Edit: Specifically, this was fixed in this commit, which is included in all releases since 1.8.5.

  16. sds commented on Apr 10, 2015

    @sds
    sds
    on Apr 10, 2015
    Owner

    I believe I've addressed this issue in 56f7339...6223ce2. I've also introduced a magic constant GIT_VERSION that we can use to hopefully make special cases like this easier to deal with in the future.

    Feel free to comment inline on the commits.

    I believe this wraps up any blockers for the 0.24.0 release. I'll be playing around with master for the next day and hopefully cut a release (barring any other issues discovered).

  17. sds
    closed this as completedon Apr 10, 2015
  18. sds
    added a commit that references this issue on Nov 11, 2026

    Support hook side effects without stashing repository changes (#939)

    a5bdc87
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bug

    Projects

    No projects

      Milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions