Error when removing submodule

[lencioni]

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.

[jawshooah]

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.

[lencioni]

I'm using v0.23.0.

[jawshooah]

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?

[jawshooah]

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

[sds]

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.

[sds]

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

[jawshooah]

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

[sds]

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?

[jawshooah]

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.

[jawshooah]

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

[sds]

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

[sds]

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)