threads / discuss / 19669

Troubles when directory is replaced by symlink

Subject: Troubles when directory is replaced by symlink

## tl;dr

9 messages between Jun 5, 2009 and Jun 26, 2009.

replies: 8people: 5as markdown or json

Alexander Gladysh· Jun 5, 2009, 05:43 UTC · lore
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
Alexander Gladysh· Jun 9, 2009, 08:18 UTC · re: Alexander Gladysh · lore

Re: Troubles when directory is replaced by symlink

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:
Show 154 quoted lines
> 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
>
Jeff King· Jun 11, 2009, 11:48 UTC · re: Alexander Gladysh · lore

Re: Troubles when directory is replaced by symlink

[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
Kjetil Barvik· Jun 11, 2009, 14:37 UTC · re: Jeff King · lore

Re: Troubles when directory is replaced by symlink

Jeff King <peff@peff.net> writes:
Show 51 quoted lines
> [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
Kjetil Barvik· Jun 14, 2009, 14:34 UTC · re: Jeff King · lore

Re: Troubles when directory is replaced by symlink

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
James Pickens· Jun 24, 2009, 22:07 UTC · re: Kjetil Barvik · lore

Re: Troubles when directory is replaced by symlink

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
Kjetil Barvik· Jun 25, 2009, 22:51 UTC · re: James Pickens · lore

Re: Troubles when directory is replaced by symlink

James Pickens <jepicken@gmail.com> writes:
Show 11 quoted lines
> 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
Kjetil Barvik· Jun 26, 2009, 13:14 UTC · re: James Pickens · lore

Re: Troubles when directory is replaced by symlink

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:
Show 11 quoted lines
> 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
Johannes Sixt· Jun 26, 2009, 14:14 UTC · re: Kjetil Barvik · lore

Re: Troubles when directory is replaced by symlink

Kjetil Barvik schrieb:
Show 6 quoted lines
> 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

← back to recent threads