Page MenuHomePhabricator

salvaged: properly deal with salvaged file during copy tracing
ClosedPublic

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

Details

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.Wed, Sep 30, 9:11 AM
marmoute updated this revision to Diff 22997.Fri, Oct 2, 5:09 AM
Alphare accepted this revision.Fri, Oct 2, 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.Fri, Oct 2, 12:26 PM
tests/test-copies-chain-merge.t
968

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

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.

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.

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.

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.

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.

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

This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.