[BUG] git_path() returns relative paths

[BUG] git_path() returns relative paths

[Ramkumar Ramachandra]

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*

Re: [BUG] git_path() returns relative paths

[Duy Nguyen]

On Sat, Jul 27, 2013 at 6:10 PM, Ramkumar Ramachandra
<artagnon@gmail.com> 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

Re: [BUG] git_path() returns relative paths

[Ramkumar Ramachandra]

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.

Re: [BUG] git_path() returns relative paths

[Fredrik Gustafsson]

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

Re: [BUG] git_path() returns relative paths

[Ramkumar Ramachandra]

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?

Re: [BUG] git_path() returns relative paths

[Duy Nguyen]

On Sat, Jul 27, 2013 at 7:31 PM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> 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?

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

Re: [BUG] git_path() returns relative paths

[Ramkumar Ramachandra]

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.

Re: [BUG] git_path() returns relative paths

[Ramkumar Ramachandra]

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

Re: [BUG] git_path() returns relative paths

[Duy Nguyen]

On Sat, Jul 27, 2013 at 9:33 PM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> 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?!

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

Re: [BUG] git_path() returns relative paths

[Ramkumar Ramachandra]

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.

Re: [BUG] git_path() returns relative paths

[Jiang Xin]

2013/7/28 Ramkumar Ramachandra <artagnon@gmail.com>:
> 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

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

Re: [BUG] git_path() returns relative paths

[Ramkumar Ramachandra]

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.

Re: [BUG] git_path() returns relative paths

[Duy Nguyen]

On Sat, Jul 27, 2013 at 11:05 PM, Ramkumar Ramachandra
<artagnon@gmail.com> 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

Re: [BUG] git_path() returns relative paths

[Duy Nguyen]

On Sat, Jul 27, 2013 at 09:35:40PM +0530, Ramkumar Ramachandra wrote:
> 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?

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.

Re: [BUG] git_path() returns relative paths

[Ramkumar Ramachandra]

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?

Re: [BUG] git_path() returns relative paths

[Duy Nguyen]

On Sun, Jul 28, 2013 at 3:49 PM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> 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?

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

Re: [BUG] git_path() returns relative paths

[Ramkumar Ramachandra]

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.

Re: [BUG] git_path() returns relative paths

[Ramkumar Ramachandra]

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.