[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:

diff --git a/builtin/commit.c b/builtin/commit.c
index 003bd7d..38a7c77 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c

@@ -678,6 +678,9 @@ static int prepare_to_commit(const char
                hook_arg2 = "";
        }

+       char *buf = get_current_dir_name();
+       die("DBG: %s | %s", buf, git_path(commit_editmsg));
+
        s->fp = fopen(git_path(commit_editmsg), "w");
        if (s->fp == NULL)
                die_errno(_("could not open '%s'"), git_path(commit_editmsg));

On master, commit returns:

  fatal: DBG: /home/artagnon/.elisp/flx |
../../.git/modules/.elisp/flx/COMMIT_EDITMSG

When backported to v1.8.3.3, commit returns:

  fatal: DBG: /home/artagnon/.elisp/flx |
/home/artagnon/dotfiles/.git/modules/.elisp/flx/COMMIT_EDITMSG

I tried looking through the logs to see what has changed in
path.c/environment.c, but have come up with nothing so far.  I think
I'll have to resort to using a hammer like bisect now.

*scratches head*

On Sat, Jul 27, 2013 at 6:10 PM, Ramkumar Ramachandra
[off-list ref] wrote:

I tried looking through the logs to see what has changed in
path.c/environment.c, but have come up with nothing so far.  I think
I'll have to resort to using a hammer like bisect now.

*scratches head*

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

Ramkumar Ramachandra wrote:

*scratches head*

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.

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

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:

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.

-- 
Med vänliga hälsningar
Fredrik Gustafsson

tel: 0733-608274
e-post: iveqy@iveqy.com

Ramkumar Ramachandra wrote:

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

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
[off-list ref] wrote:

Ramkumar Ramachandra wrote:

quoted

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

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?

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

Fredrik Gustafsson wrote:

When I hear submodules

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.

Duy Nguyen wrote:

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

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
[off-list ref] wrote:

Duy Nguyen wrote:

quoted

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

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?!

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

Duy Nguyen 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?

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 [off-list ref]:

Duy Nguyen wrote:

quoted

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?

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

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 [off-list ref]
     1 file changed, 27 insertions(+)

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

-- 
Jiang Xin

Jiang Xin wrote:

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

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"

From the point of view of $EDITOR, how is this different?  Yet, when

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

On Sat, Jul 27, 2013 at 11:05 PM, Ramkumar Ramachandra
[off-list ref] 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"

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

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.

-- 
Duy

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

Duy Nguyen wrote:

quoted

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?

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?

How about something like this as a workaround for emacs?

-- 8< --

diff --git a/editor.c b/editor.c
index 27bdecd..fda3e41 100644
--- a/editor.c
+++ b/editor.c

@@ -37,7 +37,8 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en
 		return error("Terminal is dumb, but EDITOR unset");
 
 	if (strcmp(editor, ":")) {
-		const char *args[] = { editor, path, NULL };
+		char *rpath = realpath(path, NULL);
+		const char *args[] = { editor, rpath, NULL };
 		struct child_process p;
 		int ret, sig;
 

@@ -51,6 +52,7 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en
 		sigchain_push(SIGINT, SIG_IGN);
 		sigchain_push(SIGQUIT, SIG_IGN);
 		ret = finish_command(&p);
+		free(rpath);
 		sig = ret - 128;
 		sigchain_pop(SIGINT);
 		sigchain_pop(SIGQUIT);
-- 8< --

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

Duy Nguyen wrote:

I think instead of letting the kernel walk the path, emacs does it by
itself.

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
[off-list ref] wrote:

Duy Nguyen wrote:

quoted

I think instead of letting the kernel walk the path, emacs does it by
itself.

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?

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

Duy Nguyen 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.

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

This bug can be reproduced without git involved:

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

Duy Nguyen wrote:

How about something like this as a workaround for emacs?

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.