This is an archive of the discontinued Mercurial Phabricator instance.

Differential D9130

salvaged: properly deal with salvaged file during copy tracing
ClosedPublic

Authored by marmoute on Sep 30 2020, 9:11 AM.

Details

Reviewers
Alphare
Group Reviewers
hg-reviewers
Commits
rHGa8fb29b05f92: salvaged: properly deal with salvaged file during copy tracing
rHGef52dedd6c66: salvaged: properly deal with salvaged file during copy tracing
rHG50b33650fb69: salvaged: properly deal with salvaged file during copy tracing
Summary

When salvaged files are encountered, the removal have been reverted and we
should keep the rename information from the other side.

The conditional was starting to be quite hairy, so we spell it out in multiple
elif case for readability.

This fixes the associated test cases introduced a while back. The changeset centric copy tracing is now (known) bug free.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

marmoute created this revision.Sep 30 2020, 9:11 AM
marmoute updated this revision to Diff 22997.Oct 2 2020, 5:09 AM
Alphare accepted this revision.Oct 2 2020, 9:04 AM
Alphare added a subscriber: Alphare.
Alphare added inline comments.
tests/test-copies-chain-merge.t
968

I can't seem to find the hghave for this. Did I miss something?

marmoute added inline comments.Oct 2 2020, 12:26 PM
tests/test-copies-chain-merge.t
968

You are missing the #testcases filelog compatibility sidedata at the start of the file.

martinvonz added a subscriber: martinvonz.Oct 6 2020, 8:06 PM

https://bz.mercurial-scm.org/show_bug.cgi?id=6163 seems relevant. What do you think about it? Should we consider a file that was deleted and then added back to be the same file or not? I sent D9159 to show the effect of removing the constraint.

marmoute added a comment.Oct 7 2020, 3:32 AM

issue6163 is a bit tricky, the current semantic seems like a good default, but I agree it anoyingly gets in the ways in a couple of important cases.

The main source of pain is probably the one from rebase/evolve. On that regards, I wonder if we could leverage obsolescence markers and the changesets centric algorithm to fix it. It would probably be enough to remove most of the pain around issue6153.

The "re-adding a deleted files case" is a bit trickier. we currently do not have any user facing concept for this kind of revival, neither we have any format or any code to track this information. Unless by doing an explicit œdipus merge with the ancestors your bring that file back from.

My current thinking is that the current behavior seems "right", but that adding some options to allow user to explicitily record this might be good. Maybe we should just start with documenting and making it easier to create sur œdipus merge and see if they are a real demand.

martinvonz added a comment.Oct 7 2020, 11:37 AM
In D9130#137653, @marmoute wrote:

issue6163 is a bit tricky, the current semantic seems like a good default, but I agree it anoyingly gets in the ways in a couple of important cases.
The main source of pain is probably the one from rebase/evolve. On that regards, I wonder if we could leverage obsolescence markers and the changesets centric algorithm to fix it. It would probably be enough to remove most of the pain around issue6153.

Leveraging obsmarkers is something we're talked about on https://bz.mercurial-scm.org/show_bug.cgi?id=5457.

D9159 is nice in that it just *removes* code and fixes bugs in doing so (including the one you're fixing in this series, I think). The only question seems to be if it breaks any important use cases. Maybe I should send a patch to add a config to enable tracing of unrelated copies (experimental.copies.follow-unrelated?). Then we can roll that out to our users at Google and if we don't hear too many complaints, we can consider changing the behavior in core. If that works out, then we won't need this series (right?), and we can also remove the added and removed file sets I added to changeset extras (and you added to the sidedata, I think).

The "re-adding a deleted files case" is a bit trickier. we currently do not have any user facing concept for this kind of revival, neither we have any format or any code to track this information. Unless by doing an explicit œdipus merge with the ancestors your bring that file back from.
My current thinking is that the current behavior seems "right", but that adding some options to allow user to explicitily record this might be good. Maybe we should just start with documenting and making it easier to create sur œdipus merge and see if they are a real demand.

I think a more typical way of reviving a file is to use hg revert -r <ancestor> file.

martinvonz added a comment.Oct 7 2020, 11:44 AM

Oh, let me also say that even if we decide that D9159 (with a config knob) is a good idea to at least test on Googlers, I'm fine with taking this series if it helps you make progress until we have data. Then we can back it out if it turns out that following unrelated copies is a good idea.

marmoute added a comment.Oct 8 2020, 5:35 AM
In D9130#137717, @martinvonz wrote:
In D9130#137653, @marmoute wrote:

issue6163 is a bit tricky, the current semantic seems like a good default, but I agree it anoyingly gets in the ways in a couple of important cases.
The main source of pain is probably the one from rebase/evolve. On that regards, I wonder if we could leverage obsolescence markers and the changesets centric algorithm to fix it. It would probably be enough to remove most of the pain around issue6153.

Leveraging obsmarkers is something we're talked about on https://bz.mercurial-scm.org/show_bug.cgi?id=5457.
D9159 is nice in that it just *removes* code and fixes bugs in doing so (including the one you're fixing in this series, I think). The only question seems to be if it breaks any important use cases.

Yes, it does ;-) See my comment on D9159

Maybe I should send a patch to add a config to enable tracing of unrelated copies (experimental.copies.follow-unrelated?). Then we can roll that out to our users at Google and if we don't hear too many complaints, we can consider changing the behavior in core.

I don't think that doing it by default is the right approach and while I am sympathetic to the issue of makeing issue6153 better, I think have some explicit UX around it would be a better approach. In all cases D9159 si too simple to be the way forward. In addition, Google have a large number of user, but they use Mercurial is a very specific, controlled and unified way. While providing interesting feedback, I would not use Google alone to picture a wider picture of the Mercurial community.

If that works out, then we won't need this series (right?), and we can also remove the added and removed file sets I added to changeset extras (and you added to the sidedata, I think).

I don't think we can do that. And I need to this series to move forward. In addition, the new sidedata encoding have value beside the copy tracing as it provide flexible/correct en efficient storage of complex information.

The "re-adding a deleted files case" is a bit trickier. we currently do not have any user facing concept for this kind of revival, neither we have any format or any code to track this information. Unless by doing an explicit œdipus merge with the ancestors your bring that file back from.
My current thinking is that the current behavior seems "right", but that adding some options to allow user to explicitily record this might be good. Maybe we should just start with documenting and making it easier to create sur œdipus merge and see if they are a real demand.

I think a more typical way of reviving a file is to use hg revert -r <ancestor> file.

I agree this is probably the most common user action.

They are two aspects of the problem:

(1) User interface: How to we get the user to specify they intend,
(2) Internal: how do we record the user intend (and update algorithm to take it in account).

Both aspect of the problem need to be iron out.

martinvonz added a comment.Oct 8 2020, 11:16 AM

I feel like this series is overcomplicated, but I'll queue it so you can at least make progress.

marmoute added a commit: rHG50b33650fb69: salvaged: properly deal with salvaged file during copy tracing.Oct 8 2020, 11:24 AM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHG50b33650fb69: salvaged: properly deal with salvaged file during copy tracing (authored by marmoute). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/copies.py (14 lines)
Mtests/test-copies-chain-merge.t (12 lines)

Diff 23094

mercurial/copies.py

minor[dest] = value minor[dest] = value
else: else:
new_tt = value[0] new_tt = value[0]
other_tt = other[0] other_tt = other[0]
if value[1] == other[1]: if value[1] == other[1]:
continue continue
# content from "major" wins, unless it is older # content from "major" wins, unless it is older
# than the branch point or there is a merge # than the branch point or there is a merge
if ( if new_tt == other_tt:
new_tt == other_tt minor[dest] = value
or not isancestor(new_tt, other_tt) elif value[1] is None and dest in changes.salvaged:
or dest in changes.merged pass
): elif other[1] is None and dest in changes.salvaged:
minor[dest] = value
elif not isancestor(new_tt, other_tt):
minor[dest] = value
elif dest in changes.merged:
minor[dest] = value minor[dest] = value
def _revinfo_getter_extra(repo): def _revinfo_getter_extra(repo):
"""return a function that return multiple data given a <rev>"i """return a function that return multiple data given a <rev>"i
* p1: revision number of first parent * p1: revision number of first parent
* p2: revision number of first parent * p2: revision number of first parent

tests/test-copies-chain-merge.t

|/ |/
o 2 i-2: c -move-> d o 2 i-2: c -move-> d
| |
o 1 i-1: a -move-> c o 1 i-1: a -move-> c
| |
o 0 i-0 initial commit: a b h o 0 i-0 initial commit: a b h
BROKEN: 'a' should be the the source of 'd' in the changeset centric algorithm too 'a' is the copy source of 'd'
$ hg status --copies --rev 'desc("i-0")' --rev 'desc("mCGm-0")' $ hg status --copies --rev 'desc("i-0")' --rev 'desc("mCGm-0")'
A d A d
a (filelog !) a (no-compatibility !)
R a R a
$ hg status --copies --rev 'desc("i-0")' --rev 'desc("mGCm-0")' $ hg status --copies --rev 'desc("i-0")' --rev 'desc("mGCm-0")'
A d A d
a (filelog !) a (no-compatibility !)
R a R a
$ hg status --copies --rev 'desc("c-1")' --rev 'desc("mCGm-0")' $ hg status --copies --rev 'desc("c-1")' --rev 'desc("mCGm-0")'
A d A d
$ hg status --copies --rev 'desc("c-1")' --rev 'desc("mGCm-0")' $ hg status --copies --rev 'desc("c-1")' --rev 'desc("mGCm-0")'
A d A d
$ hg status --copies --rev 'desc("g-1")' --rev 'desc("mCGm-0")' $ hg status --copies --rev 'desc("g-1")' --rev 'desc("mCGm-0")'
$ hg status --copies --rev 'desc("g-1")' --rev 'desc("mGCm-0")' $ hg status --copies --rev 'desc("g-1")' --rev 'desc("mGCm-0")'
|/ |/
o 2 i-2: c -move-> d o 2 i-2: c -move-> d
| |
o 1 i-1: a -move-> c o 1 i-1: a -move-> c
| |
o 0 i-0 initial commit: a b h o 0 i-0 initial commit: a b h
BROKEN: 'a' should be the the source of 'd' in the changeset centric algorithm too 'a' is the the copy source of 'd'
$ hg status --copies --rev 'desc("i-0")' --rev 'desc("mCB-revert-m-0")' $ hg status --copies --rev 'desc("i-0")' --rev 'desc("mCB-revert-m-0")'
M b M b
A d A d
a (filelog !) a (no-compatibility !)
AlphareUnsubmitted
Not Done

I can't seem to find the hghave for this. Did I miss something?

Alphare: I can't seem to find the `hghave` for this. Did I miss something?
marmouteAuthorUnsubmitted
Done

You are missing the #testcases filelog compatibility sidedata at the start of the file.

marmoute: You are missing the `#testcases filelog compatibility sidedata` at the start of the file.
R a R a
$ hg status --copies --rev 'desc("i-0")' --rev 'desc("mBC-revert-m-0")' $ hg status --copies --rev 'desc("i-0")' --rev 'desc("mBC-revert-m-0")'
M b M b
A d A d
a (filelog !) a (no-compatibility !)
R a R a
$ hg status --copies --rev 'desc("c-1")' --rev 'desc("mCB-revert-m-0")' $ hg status --copies --rev 'desc("c-1")' --rev 'desc("mCB-revert-m-0")'
M b M b
A d A d
$ hg status --copies --rev 'desc("c-1")' --rev 'desc("mBC-revert-m-0")' $ hg status --copies --rev 'desc("c-1")' --rev 'desc("mBC-revert-m-0")'
M b M b
A d A d
$ hg status --copies --rev 'desc("b-1")' --rev 'desc("mCB-revert-m-0")' $ hg status --copies --rev 'desc("b-1")' --rev 'desc("mCB-revert-m-0")'