Patchwork [2,of,3,STABLE] rename: properly report removed and added file as modified (issue4458)

login
register
mail settings
Submitter Pierre-Yves David
Date Nov. 26, 2014, 11:30 p.m.
Message ID <1866e1771d90ea39c1ab.1417044629@marginatus.alto.octopoid.net>
Download mbox | patch
Permalink /patch/6867/
State Superseded
Headers show

Comments

Pierre-Yves David - Nov. 26, 2014, 11:30 p.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@fb.com>
# Date 1416883376 28800
#      Mon Nov 24 18:42:56 2014 -0800
# Branch stable
# Node ID 1866e1771d90ea39c1abc2bf677f7acd48bca701
# Parent  82ede43751eae4b870b598326d30105960ce0e10
rename: properly report removed and added file as modified (issue4458)

The result of 'hg rm' + 'hg rename' disagreed with the one from
'hg rename --force'. We align them on 'hg move --force' because it agree with
what 'hg status' says after the commit.

Stopping to repor modified file as added put an end to hg revert confusion in this
situation (issue4458).

However, reporting file as modified also prevent revert to restore the copy
source. We fix this in a later changesets.

Git diff also stop reporting the add in the middle of the chain as add. Not
sure how important (and even wrong) it is.
Martin von Zweigbergk - Nov. 27, 2014, 12:26 a.m.
On Wed Nov 26 2014 at 3:31:32 PM Pierre-Yves David <
pierre-yves.david@ens-lyon.org> wrote:

> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@fb.com>
> # Date 1416883376 28800
> #      Mon Nov 24 18:42:56 2014 -0800
> # Branch stable
> # Node ID 1866e1771d90ea39c1abc2bf677f7acd48bca701
> # Parent  82ede43751eae4b870b598326d30105960ce0e10
> rename: properly report removed and added file as modified (issue4458)
>
> The result of 'hg rm' + 'hg rename' disagreed with the one from
> 'hg rename --force'. We align them on 'hg move --force' because it agree
> with
> what 'hg status' says after the commit.
>
> Stopping to repor modified file as added put an end to hg revert confusion
> in this
> situation (issue4458).
>
> However, reporting file as modified also prevent revert to restore the copy
> source. We fix this in a later changesets.
>
> Git diff also stop reporting the add in the middle of the chain as add. Not
> sure how important (and even wrong) it is.
>

I think you said it's consistent with how it already appears after commit,
so I agree that changing that seems like a separate problem (if we even
want to change it).


>
> diff --git a/mercurial/context.py b/mercurial/context.py
> --- a/mercurial/context.py
> +++ b/mercurial/context.py
> @@ -1335,12 +1335,14 @@ class workingctx(committablectx):
>              self._repo.ui.warn(_("copy failed: %s is not a file or a "
>                                   "symbolic link\n") % dest)
>          else:
>              wlock = self._repo.wlock()
>              try:
> -                if self._repo.dirstate[dest] in '?r':
> +                if self._repo.dirstate[dest] in '?':
>                      self._repo.dirstate.add(dest)
> +                elif self._repo.dirstate[dest] in 'r':
> +                    self._repo.dirstate.normallookup(dest)
>                  self._repo.dirstate.copy(source, dest)
>              finally:
>                  wlock.release()
>
>      def _filtersuspectsymlink(self, files):
> diff --git a/mercurial/scmutil.py b/mercurial/scmutil.py
> --- a/mercurial/scmutil.py
> +++ b/mercurial/scmutil.py
> @@ -802,12 +802,16 @@ def dirstatecopy(ui, repo, wctx, src, ds
>          if repo.dirstate[origsrc] == 'a' and origsrc == src:
>              if not ui.quiet:
>                  ui.warn(_("%s has not been committed yet, so no copy "
>                            "data will be stored for %s.\n")
>                          % (repo.pathto(origsrc, cwd), repo.pathto(dst,
> cwd)))
> -            if repo.dirstate[dst] in '?r' and not dryrun:
> -                wctx.add([dst])
> +            if not dryrun:
> +                if repo.dirstate[dst] == '?':
> +                    wctx.add([dst])
> +                elif repo.dirstate[dst] == 'r':
> +                    repo.dirstate.normallookup(dst)
> +
>

Would bypassing wctx.add() and going straight to repo.dirstate.normallookup()
when the file had been previously removed be equivalent? It seems like that
bypasses the warning about adding big files and odd file types. Is the
thinking that that would have been already checked when the source was
added? It would also bypass the repo.wlock(). I don't know if that has any
effect. What's the reason for not calling wctx.add() in both cases?


>          elif not dryrun:
>              wctx.copy(origsrc, dst)
>
>  def readrequires(opener, supported):
>      '''Reads and parses .hg/requires and checks if all entries found
> diff --git a/tests/test-mq-qrename.t b/tests/test-mq-qrename.t
> --- a/tests/test-mq-qrename.t
> +++ b/tests/test-mq-qrename.t
> @@ -74,12 +74,12 @@ Test overlapping renames (issue2388)
>    $ hg qnew patchb
>    $ hg ci --mq -m c1
>    $ hg qrename patchb patchc
>    $ hg qrename patcha patchb
>    $ hg st --mq
> +  M patchb
>    M series
> -  A patchb
>    A patchc
>    R patcha
>    $ cd ..
>
>  Test renames with mq repo (issue2097)
> diff --git a/tests/test-rename.t b/tests/test-rename.t
> --- a/tests/test-rename.t
> +++ b/tests/test-rename.t
> @@ -569,19 +569,28 @@ transitive rename --after
>  overwriting with renames (issue1959)
>
>    $ hg rename d1/a d1/c
>    $ hg rename d1/b d1/a
>    $ hg status -C
> -  A d1/a
> +  M d1/a
>      d1/b
>    A d1/c
>      d1/a
>    R d1/b
>    $ hg diff --git
> -  diff --git a/d1/b b/d1/a
> -  rename from d1/b
> -  rename to d1/a
> +  diff --git a/d1/a b/d1/a
> +  --- a/d1/a
> +  +++ b/d1/a
> +  @@ -1,1 +1,1 @@
> +  -d1/a
> +  +d1/b
> +  diff --git a/d1/b b/d1/b
> +  deleted file mode 100644
> +  --- a/d1/b
> +  +++ /dev/null
> +  @@ -1,1 +0,0 @@
> +  -d1/b
>    diff --git a/d1/a b/d1/c
>    copy from d1/a
>    copy to d1/c
>    $ hg update -C
>    2 files updated, 0 files merged, 0 files removed, 0 files unresolved
> diff --git a/tests/test-status.t b/tests/test-status.t
> --- a/tests/test-status.t
> +++ b/tests/test-status.t
> @@ -388,5 +388,51 @@ warning message about such pattern.
>    $ hg --config ui.slash=false status -A --rev 1 1
>    R 1\2\3\4\5\b.txt
>  #endif
>
>    $ cd ..
> +
> +Status after move overwriting a file (issue4458)
> +=================================================
> +
> +
> +  $ hg init issue4458
> +  $ cd issue4458
> +  $ echo a > a
> +  $ echo b > b
> +  $ hg commit -Am base
> +  adding a
> +  adding b
> +
> +
> +with --force
> +
> +  $ hg mv b --force a
> +  $ hg st --copies
> +  M a
> +    b
> +  R b
> +  $ hg revert --all
> +  reverting a
> +  undeleting b
> +  $ rm *.orig
> +
> +without force
> +
> +  $ hg rm a
> +  $ hg st --copies
> +  R a
> +  $ hg mv b a
> +  $ hg st --copies
> +  M a
> +    b
> +  R b
> +
> +Other "bug" highlight, the revision status does not report the copy
> information.
> +This is buggy behavior.
> +
> +  $ hg commit -m 'blah'
> +  $ hg st --copies --change .
> +  M a
> +  R b
> +
> +  $ cd ..
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
>
Pierre-Yves David - Nov. 27, 2014, 12:44 a.m.
On 11/26/2014 04:26 PM, Martin von Zweigbergk wrote:
>     diff --git a/mercurial/scmutil.py b/mercurial/scmutil.py
>     --- a/mercurial/scmutil.py
>     +++ b/mercurial/scmutil.py
>     @@ -802,12 +802,16 @@ def dirstatecopy(ui, repo, wctx, src, ds
>               if repo.dirstate[origsrc] == 'a' and origsrc == src:
>                   if not ui.quiet:
>                       ui.warn(_("%s has not been committed yet, so no copy "
>                                 "data will be stored for %s.\n")
>                               % (repo.pathto(origsrc, cwd),
>     repo.pathto(dst, cwd)))
>     -            if repo.dirstate[dst] in '?r' and not dryrun:
>     -                wctx.add([dst])
>     +            if not dryrun:
>     +                if repo.dirstate[dst] == '?':
>     +                    wctx.add([dst])
>     +                elif repo.dirstate[dst] == 'r':
>     +                    repo.dirstate.normallookup(__dst)
>     +
>
>
> Would bypassing wctx.add() and going straight to
> repo.dirstate.normallookup() when the file had been previously removed
> be equivalent? It seems like that bypasses the warning about adding big
> files and odd file types. Is the thinking that that would have been
> already checked when the source was added? It would also bypass the
> repo.wlock(). I don't know if that has any effect. What's the reason for
> not calling wctx.add() in both cases?

You are right, this is useless and wrong rubbish. Uncommiting it and 
sending a V2.

Patch

diff --git a/mercurial/context.py b/mercurial/context.py
--- a/mercurial/context.py
+++ b/mercurial/context.py
@@ -1335,12 +1335,14 @@  class workingctx(committablectx):
             self._repo.ui.warn(_("copy failed: %s is not a file or a "
                                  "symbolic link\n") % dest)
         else:
             wlock = self._repo.wlock()
             try:
-                if self._repo.dirstate[dest] in '?r':
+                if self._repo.dirstate[dest] in '?':
                     self._repo.dirstate.add(dest)
+                elif self._repo.dirstate[dest] in 'r':
+                    self._repo.dirstate.normallookup(dest)
                 self._repo.dirstate.copy(source, dest)
             finally:
                 wlock.release()
 
     def _filtersuspectsymlink(self, files):
diff --git a/mercurial/scmutil.py b/mercurial/scmutil.py
--- a/mercurial/scmutil.py
+++ b/mercurial/scmutil.py
@@ -802,12 +802,16 @@  def dirstatecopy(ui, repo, wctx, src, ds
         if repo.dirstate[origsrc] == 'a' and origsrc == src:
             if not ui.quiet:
                 ui.warn(_("%s has not been committed yet, so no copy "
                           "data will be stored for %s.\n")
                         % (repo.pathto(origsrc, cwd), repo.pathto(dst, cwd)))
-            if repo.dirstate[dst] in '?r' and not dryrun:
-                wctx.add([dst])
+            if not dryrun:
+                if repo.dirstate[dst] == '?':
+                    wctx.add([dst])
+                elif repo.dirstate[dst] == 'r':
+                    repo.dirstate.normallookup(dst)
+
         elif not dryrun:
             wctx.copy(origsrc, dst)
 
 def readrequires(opener, supported):
     '''Reads and parses .hg/requires and checks if all entries found
diff --git a/tests/test-mq-qrename.t b/tests/test-mq-qrename.t
--- a/tests/test-mq-qrename.t
+++ b/tests/test-mq-qrename.t
@@ -74,12 +74,12 @@  Test overlapping renames (issue2388)
   $ hg qnew patchb
   $ hg ci --mq -m c1
   $ hg qrename patchb patchc
   $ hg qrename patcha patchb
   $ hg st --mq
+  M patchb
   M series
-  A patchb
   A patchc
   R patcha
   $ cd ..
 
 Test renames with mq repo (issue2097)
diff --git a/tests/test-rename.t b/tests/test-rename.t
--- a/tests/test-rename.t
+++ b/tests/test-rename.t
@@ -569,19 +569,28 @@  transitive rename --after
 overwriting with renames (issue1959)
 
   $ hg rename d1/a d1/c
   $ hg rename d1/b d1/a
   $ hg status -C
-  A d1/a
+  M d1/a
     d1/b
   A d1/c
     d1/a
   R d1/b
   $ hg diff --git
-  diff --git a/d1/b b/d1/a
-  rename from d1/b
-  rename to d1/a
+  diff --git a/d1/a b/d1/a
+  --- a/d1/a
+  +++ b/d1/a
+  @@ -1,1 +1,1 @@
+  -d1/a
+  +d1/b
+  diff --git a/d1/b b/d1/b
+  deleted file mode 100644
+  --- a/d1/b
+  +++ /dev/null
+  @@ -1,1 +0,0 @@
+  -d1/b
   diff --git a/d1/a b/d1/c
   copy from d1/a
   copy to d1/c
   $ hg update -C
   2 files updated, 0 files merged, 0 files removed, 0 files unresolved
diff --git a/tests/test-status.t b/tests/test-status.t
--- a/tests/test-status.t
+++ b/tests/test-status.t
@@ -388,5 +388,51 @@  warning message about such pattern.
   $ hg --config ui.slash=false status -A --rev 1 1
   R 1\2\3\4\5\b.txt
 #endif
 
   $ cd ..
+
+Status after move overwriting a file (issue4458)
+=================================================
+
+
+  $ hg init issue4458
+  $ cd issue4458
+  $ echo a > a
+  $ echo b > b
+  $ hg commit -Am base
+  adding a
+  adding b
+
+
+with --force
+
+  $ hg mv b --force a
+  $ hg st --copies
+  M a
+    b
+  R b
+  $ hg revert --all
+  reverting a
+  undeleting b
+  $ rm *.orig
+
+without force
+
+  $ hg rm a
+  $ hg st --copies
+  R a
+  $ hg mv b a
+  $ hg st --copies
+  M a
+    b
+  R b
+
+Other "bug" highlight, the revision status does not report the copy information.
+This is buggy behavior.
+
+  $ hg commit -m 'blah'
+  $ hg st --copies --change .
+  M a
+  R b
+
+  $ cd ..