Troubles when directory is replaced by symlink

Troubles when directory is replaced by symlink

[Alexander Gladysh]

Hi, list!

OS X 10.5.7

$ git --version
git version 1.6.3.2

In my master branch I've had this structure:

root /
    directory1 /
        some-files
    directory2 /
        copy-of-directory1 /
            some-files

In a separate branch I replaced copy-of-directory1 with a symlink to
../directory1. I did it in two commits: first remove
copy-of-directory1, in second commit -- create the symlink.

My master branch is then moved by couple commits ahead.

Now I have nasty problems when I try to merge that separate branch
back to master (see log below).

If I split the branch history in two, and first merge up to commit
with deletion, and then separately merge the rest of it with symlink
creation -- all goes well.

Below is the log on how to reproduce the problem:

Alexander.

$ mkdir test
$ cd test
$ git init
Initialized empty Git repository in /users/agladysh/tmp/test/.git/
$ mkdir alpha
$ echo "file1" > alpha/file1
$ git add .
$ git commit -m "first commit"
[master (root-commit) 1df90a8] first commit
 1 files changed, 1 insertions(+), 0 deletions(-)
 create mode 100644 alpha/file1

$ mkdir beta
$ cp -R alpha beta/
$ git add .
$ git commit -m "second commit"
[master 0eddb98] second commit
 1 files changed, 1 insertions(+), 0 deletions(-)
 create mode 100644 beta/alpha/file1

$ git checkout -b branch
Switched to a new branch 'branch'
$ rm -rf beta/alpha
$ git add -u
$ git commit -m "deleted"
[branch 34134ea] deleted
 1 files changed, 0 insertions(+), 1 deletions(-)
 delete mode 100644 beta/alpha/file1

$ cd beta/
$ ln -s ../alpha
$ cd ..
$ git add .
$ git commit -m "symlink"
[branch b8dfba0] symlink
 1 files changed, 1 insertions(+), 0 deletions(-)
 create mode 120000 beta/alpha

$ git checkout master
Switched to branch 'master'

### All works if we're just fast-forwarding ###

$ git branch backup
$ git merge branch
Updating 0eddb98..b8dfba0
Fast forward
 beta/alpha       |    1 +
 beta/alpha/file1 |    1 -
 2 files changed, 1 insertions(+), 1 deletions(-)
 create mode 120000 beta/alpha
 delete mode 100644 beta/alpha/file1

### Trying to restore to backup
$ git reset --hard backup
error: Invalid path ''
HEAD is now at 0eddb98 second commit
$ git status
# On branch master
# Changed but not updated:
#   (use "git add/rm <file>..." to update what will be committed)
#   (use "git checkout -- <file>..." to discard changes in working directory)
#
#	deleted:    alpha/file1
#
no changes added to commit (use "git add" and/or "git commit -a")

### Needed second reset --hard.

$ git reset --hard
HEAD is now at 0eddb98 second commit

$ git status
# On branch master
nothing to commit (working directory clean)

### We're back to original master now

$ echo "change" >> alpha/file1
$ git commit -a -m "third commit"
[master 0f64b88] third commit
 1 files changed, 1 insertions(+), 0 deletions(-)

### Here it goes

$ git merge branch
CONFLICT (directory/file): There is a directory with name beta/alpha
in HEAD. Adding beta/alpha as beta/alpha~branch
Removing beta/alpha/file1
Automatic merge failed; fix conflicts and then commit the result.
[4168][agladysh.agmac: test]$ git status
beta/alpha: needs merge
# On branch master
# Changes to be committed:
#   (use "git reset HEAD <file>..." to unstage)
#
#	deleted:    beta/alpha/file1
#
# Changed but not updated:
#   (use "git add <file>..." to update what will be committed)
#   (use "git checkout -- <file>..." to discard changes in working directory)
#
#	unmerged:   beta/alpha
#
# Untracked files:
#   (use "git add <file>..." to include in what will be committed)
#
#	beta/alpha~branch
$ git mergetool
Merging the files: beta/alpha

mv: rename beta/alpha to ./beta/alpha.BACKUP.55777: No such file or directory
cp: ./beta/alpha.BACKUP.55777: No such file or directory
Deleted merge conflict for 'beta/alpha':
  {local}: deleted
  {remote}: a symbolic link -> '../alpha'
Use (c)reated or (d)eleted file, or (a)bort? c
fatal: beta/alpha: unable to stat (No such file or directory)
mv: rename ./beta/alpha.BACKUP.55777 to beta/alpha.orig: No such file
or directory

Re: Troubles when directory is replaced by symlink

[Alexander Gladysh]

Any help here? This bug is pretty annoying (especially that it
requires double hard resets).

Should I provide any extra information?

Alexander.

On Fri, Jun 5, 2009 at 9:43 AM, Alexander Gladysh<agladysh@gmail.com> wrote:
> Hi, list!
>
> OS X 10.5.7
>
> $ git --version
> git version 1.6.3.2
>
> In my master branch I've had this structure:
>
> root /
>    directory1 /
>        some-files
>    directory2 /
>        copy-of-directory1 /
>            some-files
>
> In a separate branch I replaced copy-of-directory1 with a symlink to
> ../directory1. I did it in two commits: first remove
> copy-of-directory1, in second commit -- create the symlink.
>
> My master branch is then moved by couple commits ahead.
>
> Now I have nasty problems when I try to merge that separate branch
> back to master (see log below).
>
> If I split the branch history in two, and first merge up to commit
> with deletion, and then separately merge the rest of it with symlink
> creation -- all goes well.
>
> Below is the log on how to reproduce the problem:
>
> Alexander.
>
> $ mkdir test
> $ cd test
> $ git init
> Initialized empty Git repository in /users/agladysh/tmp/test/.git/
> $ mkdir alpha
> $ echo "file1" > alpha/file1
> $ git add .
> $ git commit -m "first commit"
> [master (root-commit) 1df90a8] first commit
>  1 files changed, 1 insertions(+), 0 deletions(-)
>  create mode 100644 alpha/file1
>
> $ mkdir beta
> $ cp -R alpha beta/
> $ git add .
> $ git commit -m "second commit"
> [master 0eddb98] second commit
>  1 files changed, 1 insertions(+), 0 deletions(-)
>  create mode 100644 beta/alpha/file1
>
> $ git checkout -b branch
> Switched to a new branch 'branch'
> $ rm -rf beta/alpha
> $ git add -u
> $ git commit -m "deleted"
> [branch 34134ea] deleted
>  1 files changed, 0 insertions(+), 1 deletions(-)
>  delete mode 100644 beta/alpha/file1
>
> $ cd beta/
> $ ln -s ../alpha
> $ cd ..
> $ git add .
> $ git commit -m "symlink"
> [branch b8dfba0] symlink
>  1 files changed, 1 insertions(+), 0 deletions(-)
>  create mode 120000 beta/alpha
>
> $ git checkout master
> Switched to branch 'master'
>
> ### All works if we're just fast-forwarding ###
>
> $ git branch backup
> $ git merge branch
> Updating 0eddb98..b8dfba0
> Fast forward
>  beta/alpha       |    1 +
>  beta/alpha/file1 |    1 -
>  2 files changed, 1 insertions(+), 1 deletions(-)
>  create mode 120000 beta/alpha
>  delete mode 100644 beta/alpha/file1
>
> ### Trying to restore to backup
> $ git reset --hard backup
> error: Invalid path ''
> HEAD is now at 0eddb98 second commit
> $ git status
> # On branch master
> # Changed but not updated:
> #   (use "git add/rm <file>..." to update what will be committed)
> #   (use "git checkout -- <file>..." to discard changes in working directory)
> #
> #       deleted:    alpha/file1
> #
> no changes added to commit (use "git add" and/or "git commit -a")
>
> ### Needed second reset --hard.
>
> $ git reset --hard
> HEAD is now at 0eddb98 second commit
>
> $ git status
> # On branch master
> nothing to commit (working directory clean)
>
> ### We're back to original master now
>
> $ echo "change" >> alpha/file1
> $ git commit -a -m "third commit"
> [master 0f64b88] third commit
>  1 files changed, 1 insertions(+), 0 deletions(-)
>
> ### Here it goes
>
> $ git merge branch
> CONFLICT (directory/file): There is a directory with name beta/alpha
> in HEAD. Adding beta/alpha as beta/alpha~branch
> Removing beta/alpha/file1
> Automatic merge failed; fix conflicts and then commit the result.
> [4168][agladysh.agmac: test]$ git status
> beta/alpha: needs merge
> # On branch master
> # Changes to be committed:
> #   (use "git reset HEAD <file>..." to unstage)
> #
> #       deleted:    beta/alpha/file1
> #
> # Changed but not updated:
> #   (use "git add <file>..." to update what will be committed)
> #   (use "git checkout -- <file>..." to discard changes in working directory)
> #
> #       unmerged:   beta/alpha
> #
> # Untracked files:
> #   (use "git add <file>..." to include in what will be committed)
> #
> #       beta/alpha~branch
> $ git mergetool
> Merging the files: beta/alpha
>
> mv: rename beta/alpha to ./beta/alpha.BACKUP.55777: No such file or directory
> cp: ./beta/alpha.BACKUP.55777: No such file or directory
> Deleted merge conflict for 'beta/alpha':
>  {local}: deleted
>  {remote}: a symbolic link -> '../alpha'
> Use (c)reated or (d)eleted file, or (a)bort? c
> fatal: beta/alpha: unable to stat (No such file or directory)
> mv: rename ./beta/alpha.BACKUP.55777 to beta/alpha.orig: No such file
> or directory
>

Re: Troubles when directory is replaced by symlink

[Jeff King]

[Kjetil: I bisected this bug to your 92604b4. Details below.]

On Tue, Jun 09, 2009 at 12:18:16PM +0400, Alexander Gladysh wrote:

> Any help here? This bug is pretty annoying (especially that it
> requires double hard resets).
> 
> Should I provide any extra information?

Actually, I think the problem happens earlier than you realize; I see
problems when switching back to master:

-- >8 --
# make a repo
mkdir repo && cd repo && git init

# content in alpha
mkdir alpha && echo content >alpha/file && git add . && git commit -m one
# and duplicate content inside beta
mkdir beta && cp -R alpha beta && git add . && git commit -m two

# now replace the duplicate with a symlink
git checkout -b branch
rm -rf beta/alpha && git add -u && git commit -m deleted
ln -s ../alpha beta/alpha && git add . && git commit -m symlink

# now checkout master again; alpha/file will be missing, even
# though it wasn't touched at all
git checkout master
git status
-- 8< --

We handled this correctly in v1.6.1, so I was able to bisect it down to:

    commit 92604b466344b2157efc42ef3521dac22d7906a2
    Author: Kjetil Barvik <barvik@broadpark.no>
    Date:   Sun Jan 18 16:14:50 2009 +0100

        lstat_cache(): more cache effective symlink/directory detection

        Make the cache functionality more effective.  Previously when
        A/B/C/D was in the cache and A/B/C/E/file.c was called for,
        there was no match at all from the cache.  Now we use the fact
        that the paths "A", "A/B" and "A/B/C" are already tested, and we
        only need to do an lstat() call on "A/B/C/E".

I don't have time to look more closely now, but I'm guessing we are
treating the symlink to the directory like the directory itself in a
place that we shouldn't.

-Peff

Re: Troubles when directory is replaced by symlink

[Kjetil Barvik]

Jeff King <peff@peff.net> writes:

> [Kjetil: I bisected this bug to your 92604b4. Details below.]
>
> On Tue, Jun 09, 2009 at 12:18:16PM +0400, Alexander Gladysh wrote:
>
>> Any help here? This bug is pretty annoying (especially that it
>> requires double hard resets).
>> 
>> Should I provide any extra information?
>
> Actually, I think the problem happens earlier than you realize; I see
> problems when switching back to master:
>
> -- >8 --
> # make a repo
> mkdir repo && cd repo && git init
>
> # content in alpha
> mkdir alpha && echo content >alpha/file && git add . && git commit -m one
> # and duplicate content inside beta
> mkdir beta && cp -R alpha beta && git add . && git commit -m two
>
> # now replace the duplicate with a symlink
> git checkout -b branch
> rm -rf beta/alpha && git add -u && git commit -m deleted
> ln -s ../alpha beta/alpha && git add . && git commit -m symlink
>
> # now checkout master again; alpha/file will be missing, even
> # though it wasn't touched at all
> git checkout master
> git status
> -- 8< --
>
> We handled this correctly in v1.6.1, so I was able to bisect it down to:
>
>     commit 92604b466344b2157efc42ef3521dac22d7906a2
>     Author: Kjetil Barvik <barvik@broadpark.no>
>     Date:   Sun Jan 18 16:14:50 2009 +0100
>
>         lstat_cache(): more cache effective symlink/directory detection
>
>         Make the cache functionality more effective.  Previously when
>         A/B/C/D was in the cache and A/B/C/E/file.c was called for,
>         there was no match at all from the cache.  Now we use the fact
>         that the paths "A", "A/B" and "A/B/C" are already tested, and we
>         only need to do an lstat() call on "A/B/C/E".
>
> I don't have time to look more closely now, but I'm guessing we are
> treating the symlink to the directory like the directory itself in a
> place that we shouldn't.
>
> -Peff

  OK, I start debuging in a few minutes.  Thanks for mail!

  -- kjetil

Re: Troubles when directory is replaced by symlink

[Kjetil Barvik]

From: Kjetil Barvik <barvik@broadpark.no>
Date: Sun, 14 Jun 2009 15:08:28 +0200
Subject: [PATCH] lstat_cache: guard against full match of length of 'name' parameter

longest_path_match() in symlinks.c does exactly what it's name says,
but in some cases that match can be too long, since the
has_*_leading_path() functions assumes that the match will newer be as
long as the name string given to the function.

fix this by adding an extra if test which checks if the match length
is equal to the 'len' parameter.
---

  OK, this is a first version of a patch for this bug.  I have not added
  a "Signed-off-by:"-tag yet, but I intend to do that when I am more
  shure that this is the real fix for the bug.  Will also add a test
  case in a later version.

  The given instructions(1) to repeat the failure, gave me the following
  4 calls to has_symlink_leading_path() for the failed 'git checkout
  master' command, and I made the following the table:

   'name' parameter       Kjetil      Linus      tested
  -------------------------------------------------------
   "beta/alpha/file"        2           1           2
   "beta/alpha"             2C          0           0
   "alpha/file"             0           0           0
   "beta/alpha/file"        0           1C          0

  The "Kjetil" column is what the h_s_l_p() function returns as of
  commit 92604b46 by me.  The "Linus" column is what the h_s_l_p()
  function returns as of commit c40641b7 by Linus.  The "tested" column
  is what will be returned when the cache is clear'ed each time.  An
  added "C" indicate that the result was cached/taken from the cache.

  My bug is from the incorrect cached value from line 2, and since I
  noticed that it take the symbolic link result, "beta/alpha", which was
  the full length of the 'name' parameter and not the leading part, I
  have based the included patch on this fact.

  The function is named has_symlink_leading_path(), so I guess it is a
  reason for that "leading" part of the name.  So, is this a reasonable
  fix?  Other comments?

  -- kjetil

  1) the following commands:

# make a repo
mkdir repo && cd repo && git init

# content in alpha
mkdir alpha && echo content >alpha/file && git add . && git commit -m one
# and duplicate content inside beta
mkdir beta && cp -R alpha beta && git add . && git commit -m two

# now replace the duplicate with a symlink
git checkout -b branch
rm -rf beta/alpha && git add -u && git commit -m deleted
ln -s ../alpha beta/alpha && git add . && git commit -m symlink

# now checkout master again; alpha/file will be missing, even
# though it wasn't touched at all
git checkout master



 symlinks.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/symlinks.c b/symlinks.c
index 8dcd632..934abc6 100644
--- a/symlinks.c
+++ b/symlinks.c
@@ -97,6 +97,10 @@ static int lstat_cache(const char *name, int len,
 			longest_path_match(name, len, cache.path, cache.len,
 					   &previous_slash);
 		match_flags = cache.flags & track_flags & (FL_NOENT|FL_SYMLINK);
+
+		if (!(track_flags & FL_FULLPATH) && match_len == len)
+			match_len = last_slash = previous_slash;
+
 		if (match_flags && match_len == cache.len)
 			return match_flags;
 		/*
-- 
1.6.3.2.277.gd10543

Re: Troubles when directory is replaced by symlink

[James Pickens]

On Sun, Jun 14, 2009, Kjetil Barvik<barvik@broadpark.no> wrote:
> From: Kjetil Barvik <barvik@broadpark.no>
> Date: Sun, 14 Jun 2009 15:08:28 +0200
> Subject: [PATCH] lstat_cache: guard against full match of length of 'name' parameter

My project ran into this bug today, and I can confirm that this patch
fixes it.  I think it's an important bug; it hasn't been mentioned yet,
but this can result in lost work if the user had modified, but not added,
one of the files that Git wrongly deleted.

So, what's the status of this patch?

James

Re: Troubles when directory is replaced by symlink

[Kjetil Barvik]

James Pickens <jepicken@gmail.com> writes:

> On Sun, Jun 14, 2009, Kjetil Barvik<barvik@broadpark.no> wrote:
>> From: Kjetil Barvik <barvik@broadpark.no>
>> Date: Sun, 14 Jun 2009 15:08:28 +0200
>> Subject: [PATCH] lstat_cache: guard against full match of length of 'name' parameter
>
> My project ran into this bug today, and I can confirm that this patch
> fixes it.  I think it's an important bug; it hasn't been mentioned yet,
> but this can result in lost work if the user had modified, but not added,
> one of the files that Git wrongly deleted.
>
> So, what's the status of this patch?

  Sorry, have not have much time, and did not get any response on the
  patch, and I almost forgot about it for a while.

  But, since you say that the patch is ok, I shall try make a more
  "final" patch by 14:00 UTC Friday 26.

  Thanks for reminding me!

  -- kjetil

Re: Troubles when directory is replaced by symlink

[Kjetil Barvik]

From: Kjetil Barvik <barvik@broadpark.no>
Date: Fri, 26 Jun 2009 14:51:02 +0200
Subject: [PATCH] lstat_cache: guard against full match of length of 'name' parameter

longest_path_match() in symlinks.c does exactly what it's name says,
but in some cases that match can be too long, since the
has_*_leading_path() functions assumes that the match will newer be as
long as the name string given to the function.

Fix this by adding an extra if test which checks if the match length
is equal to the 'len' parameter.  Also, add a test-case to catch this

Signed-off-by: Kjetil Barvik <barvik@broadpark.no>
---

* James Pickens <jepicken@gmail.com> writes:
> On Sun, Jun 14, 2009, Kjetil Barvik<barvik@broadpark.no> wrote:
>> From: Kjetil Barvik <barvik@broadpark.no>
>> Date: Sun, 14 Jun 2009 15:08:28 +0200
>> Subject: [PATCH] lstat_cache: guard against full match of length of 'name' parameter
>
> My project ran into this bug today, and I can confirm that this patch
> fixes it.  I think it's an important bug; it hasn't been mentioned yet,
> but this can result in lost work if the user had modified, but not added,
> one of the files that Git wrongly deleted.
>
> So, what's the status of this patch?

  OK, here is a more final patch with a test case.  

  -- kjetil


 symlinks.c                  |    2 ++
 t/t2007-checkout-symlink.sh |   31 +++++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 0 deletions(-)

diff --git a/symlinks.c b/symlinks.c
index 8dcd632..66a0e16 100644
--- a/symlinks.c
+++ b/symlinks.c
@@ -96,6 +96,8 @@ static int lstat_cache(const char *name, int len,
 		match_len = last_slash =
 			longest_path_match(name, len, cache.path, cache.len,
 					   &previous_slash);
+		if (!(track_flags & FL_FULLPATH) && match_len == len)
+			match_len = last_slash = previous_slash;
 		match_flags = cache.flags & track_flags & (FL_NOENT|FL_SYMLINK);
 		if (match_flags && match_len == cache.len)
 			return match_flags;
diff --git a/t/t2007-checkout-symlink.sh b/t/t2007-checkout-symlink.sh
index 20f3343..08e3c32 100755
--- a/t/t2007-checkout-symlink.sh
+++ b/t/t2007-checkout-symlink.sh
@@ -53,4 +53,35 @@ test_expect_success 'switch from dir to symlink' '
 
 '
 
+test_expect_success 'setup of new directories and a branch' '
+
+	git checkout master &&
+
+	mkdir alpha &&
+	echo content >alpha/file &&
+	git add . &&
+	git commit -m one &&
+
+	mkdir beta &&
+	cp -R alpha beta &&
+	git add . &&
+	git commit -m two &&
+
+	git checkout -b branch &&
+	rm -rf beta/alpha &&
+	git add -u &&
+	git commit -m deleted &&
+	ln -s ../alpha beta/alpha &&
+	git add . &&
+	git commit -m symlink
+
+'
+
+test_expect_success 'checkout of master - alpha/file and beta/alpha/file should exist' '
+
+	git checkout master &&
+	ls alpha/file beta/alpha/file
+
+'
+
 test_done
-- 
1.6.3.2.277.gd10543

Re: Troubles when directory is replaced by symlink

[Johannes Sixt]

Kjetil Barvik schrieb:
> Subject: [PATCH] lstat_cache: guard against full match of length of 'name' parameter
> 
> longest_path_match() in symlinks.c does exactly what it's name says,
> but in some cases that match can be too long, since the
> has_*_leading_path() functions assumes that the match will newer be as
> long as the name string given to the function.

And these "some cases" are? When "a directory was replaced by a symlink"
("or what??" I am inclined to add). Would you please be very specific
here, perhaps with an example, so that we still know the details in 6 months.

> +test_expect_success 'checkout of master - alpha/file and beta/alpha/file should exist' '

test_expect_success 'checkout replaces symlink by directory' '

BTW, this is what the test seems to check, and it is the opposite of what
the mail's Subject says. So, which one is it?

> +
> +	git checkout master &&
> +	ls alpha/file beta/alpha/file

	git checkout master &&
	! test -h alpha &&
	test -f alpha/file &&
	test -f beta/alpha/file

-- Hannes