Patchwork [3,of,3,STABLE] merge: while checking unknown files also check link flag (issue5027)

# HG changeset patch
# User Siddharth Agarwal <sid0@fb.com>
# Date 1451371891 28800
#      Mon Dec 28 22:51:31 2015 -0800
# Branch stable
# Node ID b7b6a2945e1ecc40ff3ce93c1cefeae708d4cd29
# Parent  0562811a731ffdc1d6317d7002e860ba51e8a844
# Available At http://42.netv6.net/sid0-wip/hg/
#              hg pull http://42.netv6.net/sid0-wip/hg/ -r b7b6a2945e1e
merge: while checking unknown files also check link flag (issue5027)

Previously, while checking to see whether files were the same, we wouldn't
compare whether they were symlinks or not on both ends -- just their contents.

This is a very niche case because files are very unlikely to have the same
contents as symlinks: symlinks pointing to the empty string are prohibited on
most platforms, and non-empty text files are likely to have newlines in them.

On 12/28/15 22:55, Siddharth Agarwal wrote:
> # HG changeset patch
> # User Siddharth Agarwal <sid0@fb.com>
> # Date 1451371891 28800
> #      Mon Dec 28 22:51:31 2015 -0800
> # Branch stable
> # Node ID b7b6a2945e1ecc40ff3ce93c1cefeae708d4cd29
> # Parent  0562811a731ffdc1d6317d7002e860ba51e8a844
> # Available At http://42.netv6.net/sid0-wip/hg/
> #              hg pull http://42.netv6.net/sid0-wip/hg/ -r b7b6a2945e1e
> merge: while checking unknown files also check link flag (issue5027)

Patch 2-3 are a bit of an RFC -- I'm not sure what the right behavior is 
here. For a file to symlink or symlink to file transition if the 
contents remain the same should we overwrite or abort?


>
> Previously, while checking to see whether files were the same, we wouldn't
> compare whether they were symlinks or not on both ends -- just their contents.
>
> This is a very niche case because files are very unlikely to have the same
> contents as symlinks: symlinks pointing to the empty string are prohibited on
> most platforms, and non-empty text files are likely to have newlines in them.
>
> diff --git a/hgext/largefiles/overrides.py b/hgext/largefiles/overrides.py
> --- a/hgext/largefiles/overrides.py
> +++ b/hgext/largefiles/overrides.py
> @@ -426,10 +426,10 @@ def overridedebugstate(orig, ui, repo, *
>   # The overridden function filters the unknown files by removing any
>   # largefiles. This makes the merge proceed and we can then handle this
>   # case further in the overridden calculateupdates function below.
> -def overridecheckunknownfile(origfn, repo, wctx, mctx, f, f2):
> +def overridecheckunknownfile(origfn, repo, wctx, mctx, f, f2, islink):
>       if lfutil.standin(repo.dirstate.normalize(f)) in wctx:
>           return False
> -    return origfn(repo, wctx, mctx, f, f2)
> +    return origfn(repo, wctx, mctx, f, f2, islink)
>   
>   # The manifest merge handles conflicts on the manifest level. We want
>   # to handle changes in largefile-ness of files at this level too.
> diff --git a/mercurial/merge.py b/mercurial/merge.py
> --- a/mercurial/merge.py
> +++ b/mercurial/merge.py
> @@ -401,11 +401,13 @@ class mergestate(object):
>           """rerun merge process for file path `dfile`"""
>           return self._resolve(False, dfile, wctx, labels=labels)[1]
>   
> -def _checkunknownfile(repo, wctx, mctx, f, f2):
> +def _checkunknownfile(repo, wctx, mctx, f, f2, islink):
> +    # we only check link rather than both link and exec because the exec flag is
> +    # easy to fix
>       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]))
> +        and (islink != repo.wvfs.islink(f) or mctx[f2].cmp(wctx[f])))
>   
>   def _checkunknownfiles(repo, wctx, mctx, force, actions):
>       """
> @@ -417,10 +419,13 @@ def _checkunknownfiles(repo, wctx, mctx,
>       if not force:
>           for f, (m, args, msg) in actions.iteritems():
>               if m in ('c', 'dc'):
> -                if _checkunknownfile(repo, wctx, mctx, f, f):
> +                flags, = args
> +                if _checkunknownfile(repo, wctx, mctx, f, f, 'l' in flags):
>                       aborts.append(f)
>               elif m == 'dg':
> -                if _checkunknownfile(repo, wctx, mctx, f, args[0]):
> +                flags = args[1]
> +                if _checkunknownfile(repo, wctx, mctx, f, args[0],
> +                                     'l' in flags):
>                       aborts.append(f)
>   
>       for f in sorted(aborts):
> @@ -434,7 +439,7 @@ def _checkunknownfiles(repo, wctx, mctx,
>               actions[f] = ('g', args, msg)
>           elif m == 'cm':
>               fl2, anc = args
> -            different = _checkunknownfile(repo, wctx, mctx, f, f)
> +            different = _checkunknownfile(repo, wctx, mctx, f, f, 'l' in fl2)
>               if different:
>                   actions[f] = ('m', (f, f, None, False, anc),
>                                 "remote differs from untracked local")
> diff --git a/tests/test-merge1.t b/tests/test-merge1.t
> --- a/tests/test-merge1.t
> +++ b/tests/test-merge1.t
> @@ -87,11 +87,12 @@ no merges expected
>     $ hg add a
>     $ hg commit -m "commit #0"
>     $ echo This is file b1 > b
> -  $ hg add b
> +  $ printf this-has-no-newline > no-newline
> +  $ hg add b no-newline
>     $ hg commit -m "commit #1"
>   
>     $ hg update 0
> -  0 files updated, 0 files merged, 1 files removed, 0 files unresolved
> +  0 files updated, 0 files merged, 2 files removed, 0 files unresolved
>     $ echo This is file c1 > c
>     $ hg add c
>     $ hg commit -m "commit #2"
> @@ -119,7 +120,36 @@ symlinks shouldn't be followed
>     b: untracked file differs
>     abort: untracked files in working directory differ from files in requested revision
>     [255]
> +don't overwrite symlinks with files with the same contents
> +  $ ln -s this-has-no-newline no-newline
> +  $ hg merge 1
> +  b: untracked file differs
> +  no-newline: untracked file differs
> +  abort: untracked files in working directory differ from files in requested revision
> +  [255]
> +... or vice versa
> +  $ hg up --clean 0
> +  0 files updated, 0 files merged, 1 files removed, 0 files unresolved
> +  $ ln -s this-has-no-newline-b no-newline-b
> +  $ hg add no-newline-b
> +  $ hg commit -m "commit #3"
> +  created new head
> +  $ hg up --clean 2
> +  1 files updated, 0 files merged, 1 files removed, 0 files unresolved
> +  $ printf this-has-no-newline-b > no-newline-b
> +  $ hg merge 3
> +  no-newline-b: untracked file differs
> +  abort: untracked files in working directory differ from files in requested revision
> +  [255]
> +do overwrite symlinks with identical symlinks
> +  $ rm no-newline-b
> +  $ ln -s this-has-no-newline-b no-newline-b
> +  $ hg merge 3
> +  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
> +  (branch merge, don't forget to commit)
>   
> +  $ hg up --clean 2
> +  0 files updated, 0 files merged, 1 files removed, 0 files unresolved
>     $ rm b
>     $ echo This is file b2 > b
>   #endif
> @@ -128,7 +158,7 @@ merge of b expected
>     $ hg merge -f 1
>     merging b
>     merging for b
> -  0 files updated, 1 files merged, 0 files removed, 0 files unresolved
> +  1 files updated, 1 files merged, 0 files removed, 0 files unresolved
>     (branch merge, don't forget to commit)
>     $ hg diff --nodates
>     diff -r 49035e18a8e6 b
> @@ -136,8 +166,15 @@ merge of b expected
>     +++ b/b
>     @@ -0,0 +1,1 @@
>     +This is file b2
> +  diff -r 49035e18a8e6 no-newline
> +  --- /dev/null
> +  +++ b/no-newline
> +  @@ -0,0 +1,1 @@
> +  +this-has-no-newline
> +  \ No newline at end of file
>     $ hg status
>     M b
> +  M no-newline
>     $ cd ..; rm -r t
>   
>     $ hg init t
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel

Siddharth Agarwal wrote:
> Patch 2-3 are a bit of an RFC -- I'm not sure what the right behavior is
> here. For a file to symlink or symlink to file transition if the contents
> remain the same should we overwrite or abort?

If the contents remain the same, I think that warning might be enough.
Certainly, if the file it's pointing to is managed in the repository
with that value, warning should be sufficient. If it points to an
unmanaged file, that's more serious.

On 12/29/15 07:15, timeless wrote:
> Siddharth Agarwal wrote:
>> Patch 2-3 are a bit of an RFC -- I'm not sure what the right behavior is
>> here. For a file to symlink or symlink to file transition if the contents
>> remain the same should we overwrite or abort?
> If the contents remain the same, I think that warning might be enough.
> Certainly, if the file it's pointing to is managed in the repository
> with that value, warning should be sufficient. If it points to an
> unmanaged file, that's more serious.

Yeah, I think it's probably more complicated than just aborting on 
symlink to file transitions. Too complicated for stable anyhow. Please 
drop patches 2-3 (but keep patch 1 -- I'm pretty sure that one's fine 
for stable).