Page MenuHomePhabricator

copies: filter out file already in parent in duplicatecopies()
Changes PlannedPublic

Authored by martinvonz on Oct 18 2019, 5:24 PM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
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.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

martinvonz created this revision.Oct 18 2019, 5:24 PM
yuja added a subscriber: yuja.Oct 19 2019, 1:09 AM
  • 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 @@

  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.

In 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, 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.

martinvonz planned changes to this revision.Oct 19 2019, 2:12 AM
In 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, 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.