Patchwork [1,of,3,STABLE] merge: while checking for unknown files don't follow symlinks (issue5027)

login
register
mail settings
Submitter Siddharth Agarwal
Date Dec. 29, 2015, 6:55 a.m.
Message ID <5e44604d31266e416de2.1451372125@dev666.prn1.facebook.com>
Download mbox | patch
Permalink /patch/12393/
State Accepted
Headers show

Comments

Siddharth Agarwal - Dec. 29, 2015, 6:55 a.m.
# HG changeset patch
# User Siddharth Agarwal <sid0@fb.com>
# Date 1451371897 28800
#      Mon Dec 28 22:51:37 2015 -0800
# Branch stable
# Node ID 5e44604d31266e416de244171ab71df60ff8fd96
# Parent  1be02894dd6fdc489b90c784456f82d8e7f95178
# Available At http://42.netv6.net/sid0-wip/hg/
#              hg pull http://42.netv6.net/sid0-wip/hg/ -r 5e44604d3126
merge: while checking for unknown files don't follow symlinks (issue5027)

Previously, we were using Python's native 'os.path.isfile' method which follows
symlinks. In this case, since we're operating on repo contents, we don't want
to follow symlinks.

There's a behaviour change here, as shown by the second part of the added test.
Consider a symlink 'f' pointing to a file containing 'abc'. If we try and
replace it with a file with contents 'abc', previously we would have let it
though. Now we don't. Although this breaks naive inspection with tools like
'cat' and 'diff', on balance I believe this is the right change.
Siddharth Agarwal - Dec. 29, 2015, 7:20 p.m.
On 12/28/15 22:55, Siddharth Agarwal wrote:
> # HG changeset patch
> # User Siddharth Agarwal <sid0@fb.com>
> # Date 1451371897 28800
> #      Mon Dec 28 22:51:37 2015 -0800
> # Branch stable
> # Node ID 5e44604d31266e416de244171ab71df60ff8fd96
> # Parent  1be02894dd6fdc489b90c784456f82d8e7f95178
> # Available At http://42.netv6.net/sid0-wip/hg/
> #              hg pull http://42.netv6.net/sid0-wip/hg/ -r 5e44604d3126
> merge: while checking for unknown files don't follow symlinks (issue5027)
>
> Previously, we were using Python's native 'os.path.isfile' method which follows
> symlinks. In this case, since we're operating on repo contents, we don't want
> to follow symlinks.
>
> There's a behaviour change here, as shown by the second part of the added test.
> Consider a symlink 'f' pointing to a file containing 'abc'. If we try and
> replace it with a file with contents 'abc', previously we would have let it
> though.

timeless helpfully pointed out that this should be 'through'. Thanks.


>   Now we don't. Although this breaks naive inspection with tools like
> 'cat' and 'diff', on balance I believe this is the right change.
>
> diff --git a/mercurial/merge.py b/mercurial/merge.py
> --- a/mercurial/merge.py
> +++ b/mercurial/merge.py
> @@ -404,7 +404,7 @@ class mergestate(object):
>   def _checkunknownfile(repo, wctx, mctx, f, f2=None):
>       if f2 is None:
>           f2 = f
> -    return (os.path.isfile(repo.wjoin(f))
> +    return (repo.wvfs.isfileorlink(f)
>           and repo.wvfs.audit.check(f)
>           and repo.dirstate.normalize(f) not in repo.dirstate
>           and mctx[f2].cmp(wctx[f]))
> diff --git a/mercurial/scmutil.py b/mercurial/scmutil.py
> --- a/mercurial/scmutil.py
> +++ b/mercurial/scmutil.py
> @@ -312,6 +312,17 @@ class abstractvfs(object):
>       def islink(self, path=None):
>           return os.path.islink(self.join(path))
>   
> +    def isfileorlink(self, path=None):
> +        '''return whether path is a regular file or a symlink
> +
> +        Unlike isfile, this doesn't follow symlinks.'''
> +        try:
> +            st = self.lstat(path)
> +        except OSError:
> +            return False
> +        mode = st.st_mode
> +        return stat.S_ISREG(mode) or stat.S_ISLNK(mode)
> +
>       def reljoin(self, *paths):
>           """join various elements of a path together (as os.path.join would do)
>   
> diff --git a/tests/test-merge1.t b/tests/test-merge1.t
> --- a/tests/test-merge1.t
> +++ b/tests/test-merge1.t
> @@ -102,6 +102,28 @@ merge should fail
>     b: untracked file differs
>     abort: untracked files in working directory differ from files in requested revision
>     [255]
> +
> +#if symlink
> +symlinks to directories should be treated as regular files (issue5027)
> +  $ rm b
> +  $ ln -s 'This is file b2' b
> +  $ hg merge 1
> +  b: untracked file differs
> +  abort: untracked files in working directory differ from files in requested revision
> +  [255]
> +symlinks shouldn't be followed
> +  $ rm b
> +  $ echo This is file b1 > .hg/b
> +  $ ln -s .hg/b b
> +  $ hg merge 1
> +  b: untracked file differs
> +  abort: untracked files in working directory differ from files in requested revision
> +  [255]
> +
> +  $ rm b
> +  $ echo This is file b2 > b
> +#endif
> +
>   merge of b expected
>     $ hg merge -f 1
>     merging b
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel
Augie Fackler - Dec. 31, 2015, 4:25 p.m.
On Mon, Dec 28, 2015 at 10:55:25PM -0800, Siddharth Agarwal wrote:
> # HG changeset patch
> # User Siddharth Agarwal <sid0@fb.com>
> # Date 1451371897 28800
> #      Mon Dec 28 22:51:37 2015 -0800
> # Branch stable
> # Node ID 5e44604d31266e416de244171ab71df60ff8fd96
> # Parent  1be02894dd6fdc489b90c784456f82d8e7f95178
> # Available At http://42.netv6.net/sid0-wip/hg/
> #              hg pull http://42.netv6.net/sid0-wip/hg/ -r 5e44604d3126
> merge: while checking for unknown files don't follow symlinks (issue5027)

This looks unambiguously correct. Queued for stable.

>
> Previously, we were using Python's native 'os.path.isfile' method which follows
> symlinks. In this case, since we're operating on repo contents, we don't want
> to follow symlinks.
>
> There's a behaviour change here, as shown by the second part of the added test.
> Consider a symlink 'f' pointing to a file containing 'abc'. If we try and
> replace it with a file with contents 'abc', previously we would have let it
> though. Now we don't. Although this breaks naive inspection with tools like
> 'cat' and 'diff', on balance I believe this is the right change.
>
> diff --git a/mercurial/merge.py b/mercurial/merge.py
> --- a/mercurial/merge.py
> +++ b/mercurial/merge.py
> @@ -404,7 +404,7 @@ class mergestate(object):
>  def _checkunknownfile(repo, wctx, mctx, f, f2=None):
>      if f2 is None:
>          f2 = f
> -    return (os.path.isfile(repo.wjoin(f))
> +    return (repo.wvfs.isfileorlink(f)
>          and repo.wvfs.audit.check(f)
>          and repo.dirstate.normalize(f) not in repo.dirstate
>          and mctx[f2].cmp(wctx[f]))
> diff --git a/mercurial/scmutil.py b/mercurial/scmutil.py
> --- a/mercurial/scmutil.py
> +++ b/mercurial/scmutil.py
> @@ -312,6 +312,17 @@ class abstractvfs(object):
>      def islink(self, path=None):
>          return os.path.islink(self.join(path))
>
> +    def isfileorlink(self, path=None):
> +        '''return whether path is a regular file or a symlink
> +
> +        Unlike isfile, this doesn't follow symlinks.'''
> +        try:
> +            st = self.lstat(path)
> +        except OSError:
> +            return False
> +        mode = st.st_mode
> +        return stat.S_ISREG(mode) or stat.S_ISLNK(mode)
> +
>      def reljoin(self, *paths):
>          """join various elements of a path together (as os.path.join would do)
>
> diff --git a/tests/test-merge1.t b/tests/test-merge1.t
> --- a/tests/test-merge1.t
> +++ b/tests/test-merge1.t
> @@ -102,6 +102,28 @@ merge should fail
>    b: untracked file differs
>    abort: untracked files in working directory differ from files in requested revision
>    [255]
> +
> +#if symlink
> +symlinks to directories should be treated as regular files (issue5027)
> +  $ rm b
> +  $ ln -s 'This is file b2' b
> +  $ hg merge 1
> +  b: untracked file differs
> +  abort: untracked files in working directory differ from files in requested revision
> +  [255]
> +symlinks shouldn't be followed
> +  $ rm b
> +  $ echo This is file b1 > .hg/b
> +  $ ln -s .hg/b b
> +  $ hg merge 1
> +  b: untracked file differs
> +  abort: untracked files in working directory differ from files in requested revision
> +  [255]
> +
> +  $ rm b
> +  $ echo This is file b2 > b
> +#endif
> +
>  merge of b expected
>    $ hg merge -f 1
>    merging b
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel

Patch

diff --git a/mercurial/merge.py b/mercurial/merge.py
--- a/mercurial/merge.py
+++ b/mercurial/merge.py
@@ -404,7 +404,7 @@  class mergestate(object):
 def _checkunknownfile(repo, wctx, mctx, f, f2=None):
     if f2 is None:
         f2 = f
-    return (os.path.isfile(repo.wjoin(f))
+    return (repo.wvfs.isfileorlink(f)
         and repo.wvfs.audit.check(f)
         and repo.dirstate.normalize(f) not in repo.dirstate
         and mctx[f2].cmp(wctx[f]))
diff --git a/mercurial/scmutil.py b/mercurial/scmutil.py
--- a/mercurial/scmutil.py
+++ b/mercurial/scmutil.py
@@ -312,6 +312,17 @@  class abstractvfs(object):
     def islink(self, path=None):
         return os.path.islink(self.join(path))
 
+    def isfileorlink(self, path=None):
+        '''return whether path is a regular file or a symlink
+
+        Unlike isfile, this doesn't follow symlinks.'''
+        try:
+            st = self.lstat(path)
+        except OSError:
+            return False
+        mode = st.st_mode
+        return stat.S_ISREG(mode) or stat.S_ISLNK(mode)
+
     def reljoin(self, *paths):
         """join various elements of a path together (as os.path.join would do)
 
diff --git a/tests/test-merge1.t b/tests/test-merge1.t
--- a/tests/test-merge1.t
+++ b/tests/test-merge1.t
@@ -102,6 +102,28 @@  merge should fail
   b: untracked file differs
   abort: untracked files in working directory differ from files in requested revision
   [255]
+
+#if symlink
+symlinks to directories should be treated as regular files (issue5027)
+  $ rm b
+  $ ln -s 'This is file b2' b
+  $ hg merge 1
+  b: untracked file differs
+  abort: untracked files in working directory differ from files in requested revision
+  [255]
+symlinks shouldn't be followed
+  $ rm b
+  $ echo This is file b1 > .hg/b
+  $ ln -s .hg/b b
+  $ hg merge 1
+  b: untracked file differs
+  abort: untracked files in working directory differ from files in requested revision
+  [255]
+
+  $ rm b
+  $ echo This is file b2 > b
+#endif
+
 merge of b expected
   $ hg merge -f 1
   merging b