[BUG] git_path() returns relative paths

Hi,

I noticed a regression in the latest master, and I've been trying to debug it for 30 minutes now. I'm still clueless about the root cause, but I'll list whatever I found so far:

I suddenly noticed that I wasn't able to commit to a certain repository with submodules anymore. This was because git commit was opening a COMMIT_EDITMSG in the wrong path. To reproduce the problem, you need a setup like mine:

  ~/dotfiles is a repository containing submodules
  ~/.elisp is a symbolic link to ~/dotfiles/.elisp, a normal directory
  ~/.elisp/flx is the submodule repository to which I'm trying to commit

The buffer-file-name of COMMIT_EDITMSG comes out as /home/artagnon/.git/modules/flx/.git/COMMIT_EDITMSG, which is completely wrong because ~ does not even contain a .git.

So, I started debugging the issue with this patch:

On Sat, Jul 27, 2013 at 6:10 PM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:

Try bisect with GIT_TRACE_SETUP=2 and watch "setup: git_dir: " line. Although $GIT_DIR can be relative and so can git_path(), there may be something else that leads to wrong result path.

Ramkumar Ramachandra wrote:

Just took a much-needed shower and came back.  It was trivial to find.

  $ git log v1.8.3.4.. -- path.c

e02ca72 (path.c: refactor relative_path(), not only strip prefix, 2013-06-25) is the offender.

Ramkumar Ramachandra wrote:

The problem is the callsite in setup.c:setup_work_tree(). When relative_path() is called with "/home/artagnon/dotfiles/.git/modules/.elisp/flx" and "/home/artagnon/dotfiles/.elisp/flx" as the first and second arguments, it sets sb to "../../.git/modules/.elisp/flx".

Makes me wonder why setup_git_dir() doesn't just use git_dir; why does it need a relative path at all?

On Sat, Jul 27, 2013 at 7:31 PM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:

044bbbc (Make git_dir a path relative to work_tree in setup_work_tree() - 2008-06-19)

Duy Nguyen wrote:

Okay, so it does seem to be a significant optimization. Frankly, e02ca72 only improves the relative_path() algorithm, and it's not really doing anything Wrong: it's has just uncovered a previously undiscovered bug.

So far, we know that the line:

  s->fp = fopen(git_path(commit_editmsg), "w");

in commit.c:prepare_to_commit() isn't failing; so, the work_tree and git_dir do seem to be set correctly. The problem seems to be in launch_editor(). What's worse? Everything works just fine when I have a symbolic link to a directory in a normal repository; I still can't figure out what submodules have to do with any of this.

What the hell is going on?!

On Sat, Jul 27, 2013 at 9:33 PM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:

I was involved with this code (the gitdir setup code, not submodule) and am interested to know what's going on too. Could you produce a small script to reproduce it?

Duy Nguyen wrote:

Here's your reduced testcase. Just point mygit to a HEAD build.

  #!/bin/sh

  mygit=~/src/git/git
  cd /tmp
  $mygit clone https://github.com/artagnon/clayoven
  cd clayoven
  $mygit submodule add https://github.com/lewang/flx .elisp/flx
  $mygit commit -a -m "Added submodule"
  cd /tmp
  ln -s clayoven/.elisp
  cd .elisp/flx
  EDITOR="emacs -Q" git commit --amend
  # buffer-file-name = "/tmp/.git/modules/.elisp/flx/COMMIT_EDITMSG"

Note that this is emacs 24.3. I used -Q to make sure that none of my init magic (magit etc.) was responsible for changing directories or doing something equally stupid. However, considering that it's impossible to reproduce the problem with either cat or vim as the EDITOR, you might be inclined to classify this as an Emacs bug. In that case, why can't I reproduce it without submodules?

I'm going off to eat cake before I tear my hair out in frustration.

2013/7/28 Ramkumar Ramachandra <artagnon@gmail.com>:

The keypoint is that you set GIT_EDITOR to "emacs -Q". Last night I can not reproduce the bug, because I use vim as the editor. Vim as a editor may call realpath before open file.

I change the EDITOR(GIT_EDITOR) environment in your test script as follows:

    GIT_EDITOR="echo PWD: $(pwd); echo REALPATH: $(pwd -P); echo" \
    git commit --amend

And the result is :

    PWD: /tmp/.elisp/flx
    REALPATH: /private/tmp/clayoven/.elisp/flx
    ../../.git/modules/.elisp/flx/COMMIT_EDITMSG
    [master 248e49e] add usage information
     Author: Le Wang <le.wang@agworld.com.au>
     1 file changed, 27 insertions(+)

How to fix this bug, I have no idea now.

Jiang Xin wrote:

See, what stumps be about this is the no-submodule case:

  #!/bin/sh

  mygit=~/src/git/git
  cd /tmp
  rm -rf clayoven lib
  $mygit clone https://github.com/artagnon/clayoven
  ln -s clayoven/lib
  cd lib/clayoven
  EDITOR="echo PWD: $(pwd); echo REALPATH: $(pwd -P); echo" \
  git commit --amend
  # buffer-file-name = "/tmp/.git/modules/.elisp/flx/COMMIT_EDITMSG"

you change EDITOR to "emacs -Q", it works fine.

On Sat, Jul 27, 2013 at 11:05 PM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:

Here cwd is /tmp/.elisp/flx but "setup: cwd: " returns /tmp/clayoven/.elisp/flx. The COMMIT_EDITMSG path is ../../.git/modules/.elisp/flx/COMMIT_EDITMSG. If you follow the former cwd (without symlink resolution), you end up at /tmp/.git/modules. If you follow the latter cwd, you end up at /tmp/clayoven/.git/modules... I think instead of letting the kernel walk the path, emacs does it by itself. Still looking for documents about this behavior..

Duy Nguyen wrote:

If this were true, shouldn't we be able to reproduce the behavior with my no-submodules symlink testcase? How can it resolve symlinks in one case, and not in the other case?

On Sun, Jul 28, 2013 at 3:49 PM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:

In the no-submodules symlink test case, the path given to the editor is .git/COMMIT_EDITMSG, no following ".." back in the symlink target. This bug can be reproduced without git involved:

mkdir z z/a z/b echo ha >z/a/file ln -s z/b cd b cat ../a/file emacs ../a/file # fail to open 'file'

Duy Nguyen wrote:

Thanks, and sorry about the stupidity. I just patched builtin/commit.c to check this.

I'm taking this up with Emacs-Devel; you are CC'ed.

On Sat, Jul 27, 2013 at 09:35:40PM +0530, Ramkumar Ramachandra wrote:

How about something like this as a workaround for emacs?

-- 8< --

Duy Nguyen wrote:

Even if we do manage to patch Emacs now, we still need to support older versions: so yeah, this is an urgent candidate for maint. I'm waiting for the word from Emacs-Devel before writing out a commit message.

On Sat, Jul 27, 2013 at 04:40:12PM +0530, Ramkumar Ramachandra wrote:

When I hear submodules, latest master and path the only thing that comes to my mind that have touched these areas is 091a6eb0feed820a43663ca63dc2bc0bb247bbae "submodule: drop the top-level requirement".

A first test could be to see if it's this patch does anything strange. However this patch is in git since 1.8.3.3 so it's almost a month old.

Fredrik Gustafsson wrote:

The only reason this bug has something to do with submodules is because of the relative path in the gitfile (although I'm yet to corner the exact issue). Otherwise, it doesn't exercise any new code in submodule.c/ git-submodule.sh.