Patchwork D7135: copies: filter out file already in parent in duplicatecopies()

login
register
mail settings
Submitter phabricator
Date Oct. 18, 2019, 9:24 p.m.
Message ID <differential-rev-PHID-DREV-4ntoly6auldfjveyed5l-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/42479/
State New
Headers show

Comments

phabricator - Oct. 18, 2019, 9:24 p.m.
martinvonz created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  I noticed that duplicatecopies() can end up marking a file as a copy
  even if the file already exists in the parent. At least when using
  overlayworkingctx for creating the commit, it ended up in the commit's
  "files" field, which means it showed as a modified in `hg status
  --change .`. It doesn't seem like we produced any such commit in core,
  but we did end up producing some empty commits, as can be seen in the
  updated tests.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D7135

AFFECTED FILES
  mercurial/copies.py
  tests/test-graft.t
  tests/test-issue1175.t

CHANGE DETAILS




To: martinvonz, #hg-reviewers
Cc: mercurial-devel
Yuya Nishihara - Oct. 19, 2019, 5:07 a.m.
> --- a/tests/test-issue1175.t
> +++ b/tests/test-issue1175.t
> @@ -82,7 +82,6 @@
>    continue: hg graft --continue
>    $ hg graft --continue
>    grafting 1:5974126fad84 "b1"
> -  warning: can't find ancestor for 'b' copied from 'a'!
>    $ hg log -f b -T 'changeset:   {rev}:{node|short}\nsummary:     {desc}\n\n'
>    changeset:   3:376d30ccffc0
>    summary:     b1

This test was added at a43fdf33a6be, which might need bad copy information.

> --- a/mercurial/copies.py
> +++ b/mercurial/copies.py
> @@ -810,10 +810,11 @@
>          # of the function is much faster (and is required for carrying copy
>          # metadata across the rebase anyway).
>          exclude = pathcopies(repo[fromrev], repo[skiprev])
> +    pctx = wctx.p1()
>      for dst, src in pycompat.iteritems(pathcopies(repo[fromrev], repo[rev])):
>          if dst in exclude:
>              continue
> -        if dst in wctx:
> +        if dst in wctx and dst not in pctx:
>              wctx[dst].markcopied(src)

Seems fine as we don't support `hg rm b; hg cp a b` case.
phabricator - Oct. 19, 2019, 5:09 a.m.
yuja added a comment.


  > - a/tests/test-issue1175.t
  >
  > +++ b/tests/test-issue1175.t
  > @@ -82,7 +82,6 @@
  >
  >   continue: hg graft --continue
  >   $ hg graft --continue
  >   grafting 1:5974126fad84 "b1"
  >
  > - warning: can't find ancestor for 'b' copied from 'a'! $ hg log -f b -T 'changeset:   {rev}:{node|short}\nsummary:     {desc}\n\n' changeset:   3:376d30ccffc0 summary:     b1
  
  This test was added at a43fdf33a6be <https://phab.mercurial-scm.org/rHGa43fdf33a6beb697945a3dbb7253f0436ea278a6>, which might need bad copy information.
  
  > - a/mercurial/copies.py
  >
  > +++ b/mercurial/copies.py
  > @@ -810,10 +810,11 @@
  >
  > 1. of the function is much faster (and is required for carrying copy
  > 2. metadata across the rebase anyway). exclude = pathcopies(repo[fromrev], repo[skiprev])
  >
  > +    pctx = wctx.p1()
  >
  >   for dst, src in pycompat.iteritems(pathcopies(repo[fromrev], repo[rev])):
  >       if dst in exclude:
  >           continue
  >
  > - if dst in wctx:
  >
  > +        if dst in wctx and dst not in pctx:
  >
  >   wctx[dst].markcopied(src)
  
  Seems fine as we don't support `hg rm b; hg cp a b` case.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7135/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7135

To: martinvonz, #hg-reviewers
Cc: yuja, mercurial-devel
phabricator - Oct. 19, 2019, 5:14 a.m.
martinvonz added a comment.


  In D7135#104894 <https://phab.mercurial-scm.org/D7135#104894>, @yuja wrote:
  
  >> - a/tests/test-issue1175.t
  >>
  >> +++ b/tests/test-issue1175.t
  >> @@ -82,7 +82,6 @@
  >>
  >>   continue: hg graft --continue
  >>   $ hg graft --continue
  >>   grafting 1:5974126fad84 "b1"
  >>
  >> - warning: can't find ancestor for 'b' copied from 'a'! $ hg log -f b -T 'changeset:   {rev}:{node|short}\nsummary:     {desc}\n\n' changeset:   3:376d30ccffc0 summary:     b1
  >
  > This test was added at a43fdf33a6be <https://phab.mercurial-scm.org/rHGa43fdf33a6beb697945a3dbb7253f0436ea278a6>, which might need bad copy information.
  
  Ah, so maybe the bug used to be incorrectly suppressed by the logic for finding the source in an old version. Thanks for digging that up. I somehow didn't think to check.
  
  >> - a/mercurial/copies.py
  >>
  >> +++ b/mercurial/copies.py
  >> @@ -810,10 +810,11 @@
  >>
  >> 1. of the function is much faster (and is required for carrying copy
  >> 2. metadata across the rebase anyway). exclude = pathcopies(repo[fromrev], repo[skiprev])
  >>
  >> +    pctx = wctx.p1()
  >>
  >>   for dst, src in pycompat.iteritems(pathcopies(repo[fromrev], repo[rev])):
  >>       if dst in exclude:
  >>           continue
  >>
  >> - if dst in wctx:
  >>
  >> +        if dst in wctx and dst not in pctx:
  >>
  >>   wctx[dst].markcopied(src)
  >
  > Seems fine as we don't support `hg rm b; hg cp a b` case.
  
  Yes, exactly. I should have mentioned that.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7135/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7135

To: martinvonz, #hg-reviewers
Cc: yuja, mercurial-devel
phabricator - Oct. 19, 2019, 6:12 a.m.
martinvonz added a comment.
martinvonz planned changes to this revision.


  In D7135#104907 <https://phab.mercurial-scm.org/D7135#104907>, @martinvonz wrote:
  
  > In D7135#104894 <https://phab.mercurial-scm.org/D7135#104894>, @yuja wrote:
  >
  >>> - a/tests/test-issue1175.t
  >>>
  >>> +++ b/tests/test-issue1175.t
  >>> @@ -82,7 +82,6 @@
  >>>
  >>>   continue: hg graft --continue
  >>>   $ hg graft --continue
  >>>   grafting 1:5974126fad84 "b1"
  >>>
  >>> - warning: can't find ancestor for 'b' copied from 'a'! $ hg log -f b -T 'changeset:   {rev}:{node|short}\nsummary:     {desc}\n\n' changeset:   3:376d30ccffc0 summary:     b1
  >>
  >> This test was added at a43fdf33a6be <https://phab.mercurial-scm.org/rHGa43fdf33a6beb697945a3dbb7253f0436ea278a6>, which might need bad copy information.
  >
  > Ah, so maybe the bug used to be incorrectly suppressed by the logic for finding the source in an old version. Thanks for digging that up. I somehow didn't think to check.
  >
  >>> - a/mercurial/copies.py
  >>>
  >>> +++ b/mercurial/copies.py
  >>> @@ -810,10 +810,11 @@
  >>>
  >>> 1. of the function is much faster (and is required for carrying copy
  >>> 2. metadata across the rebase anyway). exclude = pathcopies(repo[fromrev], repo[skiprev])
  >>>
  >>> +    pctx = wctx.p1()
  >>>
  >>>   for dst, src in pycompat.iteritems(pathcopies(repo[fromrev], repo[rev])):
  >>>       if dst in exclude:
  >>>           continue
  >>>
  >>> - if dst in wctx:
  >>>
  >>> +        if dst in wctx and dst not in pctx:
  >>>
  >>>   wctx[dst].markcopied(src)
  >>
  >> Seems fine as we don't support `hg rm b; hg cp a b` case.
  >
  > Yes, exactly. I should have mentioned that.
  
  I was working on something unrelated just now and it turned out to be a little related to this. I was reminded that we have `hg cp --force` to "forcibly copy over an existing managed file". If you don't use that and try to copy onto an existing file, you'll get this hint: "'hg copy --force' to replace the file by recording a copy". It sounds like the plan was to allow you to copy a file onto an existing one to record it as a copy. We also have tests in `test-copy.t` showing the status of that. So I think I should rethink this patch.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7135/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7135

To: martinvonz, #hg-reviewers
Cc: yuja, mercurial-devel

Patch

diff --git a/tests/test-issue1175.t b/tests/test-issue1175.t
--- a/tests/test-issue1175.t
+++ b/tests/test-issue1175.t
@@ -82,7 +82,6 @@ 
   continue: hg graft --continue
   $ hg graft --continue
   grafting 1:5974126fad84 "b1"
-  warning: can't find ancestor for 'b' copied from 'a'!
   $ hg log -f b -T 'changeset:   {rev}:{node|short}\nsummary:     {desc}\n\n'
   changeset:   3:376d30ccffc0
   summary:     b1
diff --git a/tests/test-graft.t b/tests/test-graft.t
--- a/tests/test-graft.t
+++ b/tests/test-graft.t
@@ -500,7 +500,7 @@ 
 Graft with --log
 
   $ hg up -Cq 1
-  $ hg graft 3 --log -u foo
+  $ hg graft 3 --log -u foo --config ui.allowemptycommit=yes
   grafting 3:4c60f11aa304 "3"
   warning: can't find ancestor for 'c' copied from 'b'!
   $ hg log --template '{rev}:{node|short} {parents} {desc}\n' -r tip
@@ -752,7 +752,7 @@ 
   summary:     2
   
 ... grafts of grafts unfortunately can't
-  $ hg graft -q 13 --debug
+  $ hg graft -q 13 --debug --config ui.allowemptycommit=yes
   scanning for duplicate grafts
   grafting 13:7a4785234d87 "2"
     all copies found (* = to merge, ! = divergent, % = renamed and deleted):
@@ -762,10 +762,7 @@ 
    branchmerge: True, force: True, partial: False
    ancestor: b592ea63bb0c, local: 7e61b508e709+, remote: 7a4785234d87
   starting 4 threads for background file closing (?)
-  committing files:
-  b
-  warning: can't find ancestor for 'b' copied from 'a'!
-  reusing manifest from p1 (listed files actually unchanged)
+  reusing manifest from p1 (no file change)
   committing changelog
   updating the branch cache
   $ hg log -r 'destination(13)'
@@ -808,30 +805,27 @@ 
 
 graft works on complex revset
 
-  $ hg graft 'origin(13) or destination(origin(13))'
+  $ hg graft 'origin(13) or destination(origin(13))' \
+  > --config ui.allowemptycommit=yes
   skipping ancestor revision 21:7e61b508e709
   skipping ancestor revision 22:3a4e92d81b97
   skipping revision 2:5c095ad7e90f (already grafted to 22:3a4e92d81b97)
   grafting 7:ef0ef43d49e7 "2"
-  warning: can't find ancestor for 'b' copied from 'a'!
   grafting 13:7a4785234d87 "2"
-  warning: can't find ancestor for 'b' copied from 'a'!
   grafting 19:9627f653b421 "2"
   merging b
-  warning: can't find ancestor for 'b' copied from 'a'!
 
 graft with --force (still doesn't graft merges)
 
-  $ hg graft 19 0 6
+  $ hg graft 19 0 6 --config ui.allowemptycommit=yes
   skipping ungraftable merge revision 6
   skipping ancestor revision 0:68795b066622
   skipping already grafted revision 19:9627f653b421 (22:3a4e92d81b97 also has origin 2:5c095ad7e90f)
   [255]
-  $ hg graft 19 0 6 --force
+  $ hg graft 19 0 6 --force --config ui.allowemptycommit=yes
   skipping ungraftable merge revision 6
   grafting 19:9627f653b421 "2"
   merging b
-  warning: can't find ancestor for 'b' copied from 'a'!
   grafting 0:68795b066622 "0"
 
 graft --force after backout
@@ -1046,11 +1040,11 @@ 
 two renames actually converge to the same name (thus no actual divergence).
 
   $ hg up -q 'desc("A0")'
-  $ HGEDITOR="echo C1 >" hg graft -r 'desc("C0")' --edit
+  $ HGEDITOR="echo C1 >" hg graft -r 'desc("C0")' --edit \
+  > --config ui.allowemptycommit=yes
   grafting 2:f58c7e2b28fa "C0"
   merging f1a and f1b to f1a
   merging f5a
-  warning: can't find ancestor for 'f5a' copied from 'f5b'!
   $ hg status --change .
   M f1a
   M f5a
@@ -1064,7 +1058,8 @@ 
 
 Test the cases A.0 (f4x) and A.6 (f3x)
 
-  $ HGEDITOR="echo D1 >" hg graft -r 'desc("D0")' --edit
+  $ HGEDITOR="echo D1 >" hg graft -r 'desc("D0")' --edit \
+  > --config ui.allowemptycommit=yes
   grafting 3:b69f5839d2d9 "D0"
   note: possible conflict - f3b was renamed multiple times to:
    f3a
@@ -1150,7 +1145,8 @@ 
 
 Test the cases A.1 (f4x) and A.7 (f3x).
 
-  $ HGEDITOR="echo D2 >" hg graft -r 'desc("D0")' --edit
+  $ HGEDITOR="echo D2 >" hg graft -r 'desc("D0")' --edit \
+  > --config ui.allowemptycommit=yes
   grafting 3:b69f5839d2d9 "D0"
   note: possible conflict - f3b was renamed multiple times to:
    f3d
@@ -1165,7 +1161,8 @@ 
 
   $ hg up -q "desc("C0")"
 BROKEN: Shouldn't get the warning about missing ancestor
-  $ HGEDITOR="echo E1 >" hg graft -r 'desc("E0")' --edit
+  $ HGEDITOR="echo E1 >" hg graft -r 'desc("E0")' --edit \
+  > --config ui.allowemptycommit=yes
   grafting 6:6bd1736cab86 "E0"
   note: possible conflict - f1a was renamed multiple times to:
    f1b
@@ -1184,7 +1181,8 @@ 
 
   $ hg up -q "desc("C0")"
 BROKEN: Shouldn't get the warning about missing ancestor
-  $ HGEDITOR="echo F1 >" hg graft -r 'desc("F0")' --edit
+  $ HGEDITOR="echo F1 >" hg graft -r 'desc("F0")' --edit \
+  > --config ui.allowemptycommit=yes
   grafting 7:d376ab0d7fda "F0"
   warning: can't find ancestor for 'f1f' copied from 'f1a'!
 BROKEN: f1f should be marked a copy from f1b
@@ -1200,7 +1198,8 @@ 
 BROKEN: We should get a merge conflict from the 3-way merge between f1b in C0
 (content "c1c") and f1g in G0 (content "c1g") with f1a in A0 as base (content
 "c1a")
-  $ HGEDITOR="echo G1 >" hg graft -r 'desc("G0")' --edit
+  $ HGEDITOR="echo G1 >" hg graft -r 'desc("G0")' --edit \
+  > --config ui.allowemptycommit=yes
   grafting 8:ba67f08fb15a "G0"
   warning: can't find ancestor for 'f1g' copied from 'f1a'!
 
diff --git a/mercurial/copies.py b/mercurial/copies.py
--- a/mercurial/copies.py
+++ b/mercurial/copies.py
@@ -810,10 +810,11 @@ 
         # of the function is much faster (and is required for carrying copy
         # metadata across the rebase anyway).
         exclude = pathcopies(repo[fromrev], repo[skiprev])
+    pctx = wctx.p1()
     for dst, src in pycompat.iteritems(pathcopies(repo[fromrev], repo[rev])):
         if dst in exclude:
             continue
-        if dst in wctx:
+        if dst in wctx and dst not in pctx:
             wctx[dst].markcopied(src)