Committing inside a git submodule fails

cjlarose commented 5 years ago

Prerequisites

[X] Checked that your issue isn't already filed
[X] Tried upgrading to the latest git-mob version (npm i -g git-mob)
[X] Tried upgrading to the latest git version (brew upgrade git if installed with Homebrew/Linuxbrew)

Description

Steps to Reproduce

Checkout git-mob at revision cd74de02196f860cf50915a34275f274e43865a6. npm install; npm link.

cat <<-EOF > ~/.git-coauthors
{
  "coauthors": {
    "ad": {
      "name": "Amy Doe",
      "email": "amy@findmypast.com"
    },
    "bd": {
      "name": "Bob Doe",
      "email": "bob@findmypast.com"
    }
  }
}
EOF

cd ~/dev
mkdir parent-repo
git init
git submodule add git@github.com:findmypast-oss/git-mob.git child-repo
cd child-repo
git mob ad bd
git commit

Expected behavior: git opens up my $EDITOR with the Co-Authored-By trailers already in my commit message

Actual behavior:

fatal: could not read '.git/.gitmessage': Not a directory

Reproduces how often: Every time

Versions

operating system and version: macOS Mojave 10.14.2
git-mob version (git-mob --version): 0.4.0
git version (git --version): git version 2.20.1

Additional Information

git mob ad bd correctly updates the git config for the submodule at ~/dev/parent-repo/.git/modules/child-repo/config to include

[commit]
        template = .git/.gitmessage
[git-mob]
        co-author = Amy Doe <amy@findmypast.com>
        co-author = Bob Doe <bob@findmypast.com>

Here, .git/.gitmessage is a path I think is meant to be relative to the child repository's root (~/dev/parent-repo/child-repo), and git does appear to be trying to read the file at ~/dev/parent-repo/child-repo/.git/.gitmessage.

However, because child-repo is a submodule, child-repo/.git is not a directory. The .gitmessage file is actually at ~/dev/parent-repo/.git/modules/child-repo/.gitmessage.

cjlarose commented 5 years ago

I wanted to open up a PR to solve this issue, but I couldn't think of a straightforward and uncontroversial way to make this work.

If we make it so that git config commit.template references an absolute path, that path will break whenever the repo gets moved around or even if you rename your submodule. There doesn't seem to be a way to make it so that git config commit.template is interpreted relative to the .git directory (for example $GIT_DIR doesn't expand when used as part of this config value).

Using a path relative to the .git directory to store the .gitmessage file is certainly desirable because it lets us use project-specific authors; changing git config commit.template to be a global path like ~/.git-mob-message-template would break that use case.

The best solution I can think of right now is to set git config commit.template to something like ~/.git-mob-templates/ABD123DEF using a randomly-generated filename for each project. This approach is resilient to submodule renames and also continues to support project-specific templates, but that seemed like a pretty major change. If there's something simpler we can do, that'd be awesome.

Edit: Ok so I thought of a much simpler solution. I don't know why, but earlier I was thinking that the value stored for commit.template was static. In reality, it can be updated every time we git mob or git solo. I think an elegant solution to this problem is to do something like

git config set commit.template $(git rev-parse --git-path '.gitmessage')

every time we git mob or git solo. That way the path stored in commit.template is always correct.

I'll see if I can work on a PR.

rkotze commented 5 years ago

Thanks for the detailed issue. Interesting bug. Look forward to the PR.

rkotze commented 5 years ago

I've taken (@cjlarose ) commit to contribute to solving this issue and merged it in. Also made a tweak to it.

Now released in v0.5.1

cjlarose commented 5 years ago

Thanks for merging that change! I meant to open a PR earlier, but wanted to write some tests. Didn't get around to it, but happy to see the changes landed in the most recent version!

rkotze commented 5 years ago

I imagine there would be a lot of setup it which might make it slow. But It will be good to have a test to cover it. :+1:

cjlarose commented 5 years ago

Would you be opposed to having some "integration level" tests that actually created new git repositories via git init? I found that the project didn't have any so the tests were essentially bypassing a bunch of code that's actually used in practice.

rkotze commented 5 years ago

I agree it should be an integration test as it will be the most valuable test for this case.

rkotze / git-mob