( )⚙ D8596 merge: mark copies in in-memory context when merging

This is an archive of the discontinued Mercurial Phabricator instance.

merge: mark copies in in-memory context when merging
Changes PlannedPublic

Authored by martinvonz on May 29 2020, 11:57 AM.

Details

Reviewers
marmoute
Group Reviewers
hg-reviewers
Summary

Until now, merge.update() didn't mark copies in the
overlayworkingctx (when using that). That's because we copies are
recorded in the wctx (whether that's a workingctx or a
overlayworkingctx) in recordupdates(), which is not called when we
should not udpate the dirstate, which we should not do for in-memory
updates.

We had not noticed this before because rebase calls graftcopies
after. However, I think that using merge.update() with an in-memory
context outside the context of rebase would lose copies.

This patch fixes the problem by first extracting the desired copies
from the action map and then applying the changes outside of
recordupdates() (and removing the copy-marking from that method).

An alternative solution would be to call recordupdates() also in the
in-memory case by modifying it to call wctx.drop() etc (which don't
yet exist), but that would mean that we would add a bunch of no-op
methods on overlayworkingctx, since that already knows which files
are tracked.

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

martinvonz created this revision.May 29 2020, 11:57 AM
martinvonz planned changes to this revision.

This is not meant for review; I'm sending it now in case it'll be useful for @durin42's work on merge-diffs. I also included the rest of the stack to explain why I wrote the code.

marmoute requested changes to this revision.Jun 9 2020, 6:45 AM
marmoute added a subscriber: marmoute.

This is not meant for review; I'm sending it now in case it'll be useful for @durin42's work on merge-diffs. I also included the rest of the stack to explain why I wrote the code.

Can you add a RFC tag to this then ?

This revision now requires changes to proceed.Jun 9 2020, 6:45 AM
martinvonz requested review of this revision.Jun 9 2020, 10:04 AM

This is not meant for review; I'm sending it now in case it'll be useful for @durin42's work on merge-diffs. I also included the rest of the stack to explain why I wrote the code.

Can you add a RFC tag to this then ?

I had marked it "Changes Planned" but I've fixed the issues and the version I uploaded a day or two ago is ready for review .

marmoute added a comment.EditedJun 9 2020, 11:48 AM

This is not meant for review; I'm sending it now in case it'll be useful for @durin42's work on merge-diffs. I also included the rest of the stack to explain why I wrote the code.

Can you add a RFC tag to this then ?

I had marked it "Changes Planned" but I've fixed the issues and the version I uploaded a day or two ago is ready for review .

Okay, (It would help if you reply to your own comment to state this clearly, which you now did)

This is not meant for review; I'm sending it now in case it'll be useful for @durin42's work on merge-diffs. I also included the rest of the stack to explain why I wrote the code.

Can you add a RFC tag to this then ?

I had marked it "Changes Planned" but I've fixed the issues and the version I uploaded a day or two ago is ready for review .

Okay, (It would help if you reply to your own comment to state this clearly, which you now did)

Yep, I definitely should have. Sorry that I didn't think of doing that.

I have a bunch a questions.

mercurial/merge.py
1170

I don't understand this docstring. From what I see of the code, it mostly pull things from an actions dictionnary to put them in a copies dictionnary. That does not seems to involves the dirstates nor any "recording". Am I missing something ?

1891

I don't know what overwrite means and the docstring does not mention it. Can you clarify this and fix the docstring ?

martinvonz updated this revision to Diff 21603.Jun 10 2020, 1:54 PM
martinvonz added inline comments.Jun 10 2020, 1:54 PM
mercurial/merge.py
1170

I don't understand this docstring. From what I see of the code, it mostly pull things from an actions dictionnary to put them in a copies dictionnary. That does not seems to involves the dirstates nor any "recording". Am I missing something ?

I suspect that all you're missing is https://www.mercurial-scm.org/repo/hg/file/61719b9658b1/mercurial/mergestate.py#l758 (or maybe you just didn't infer that this was a bad copy from there) :) Fixed.

1891

It's defined on line 1631 and used a few times thereafter. Would you like a comment in each place it's used? I can add that in a separate patch. I don't know think there's a relevant docstring to update.

marmoute added inline comments.Jun 10 2020, 2:03 PM
mercurial/merge.py
1891

no, phabricator diffing made me think we were in the following function, that takes it as an argument:

def applyupdates(
    repo, actions, wctx, mctx, overwrite, wantfiledata, labels=None
):
marmoute added inline comments.Jun 11 2020, 9:45 AM
mercurial/merge.py
1891

Actually, thinking more about it, I still don't know why we should overlook rename information when overwriting. Can you clarify that point in a comment ?

mercurial/mergestate.py
766

Okay, so why do we have both a applyupdates and recordupdates function ? What are their respective resonsability and why is it split ?

Is it that apply is only responsible for touching the disk content and record to only update internal metadata like the dirstate ? If so, moving the copies recording out of recordupdates seems wrong.

martinvonz added inline comments.Jun 11 2020, 10:35 AM
mercurial/merge.py
1891

Good question! I've added a comment.

mercurial/mergestate.py
766

Yes, that is correct about their roles. See commit message for my reasoning.

marmoute added inline comments.Jun 11 2020, 1:39 PM
mercurial/mergestate.py
766

Okay, then it feel like we are going the in wrong direction and breaking the API contract here. For example, why is only part of record update move to apply update ? Does this impact our ability to execute applyupdates in parallel ? etc.

It would seems much cleaner to call recordupdates on all ctx types, don't it?

martinvonz planned changes to this revision.Jun 11 2020, 1:47 PM
martinvonz added inline comments.
mercurial/mergestate.py
766

Yes, I was also feeling that more and more. We should probably replicate many dirstate methods on workingctx and its subclasses.

What's the conclusion here ? I feel like we decided to move in the opposite direction (and to drop this series). Is that right ?

What's the conclusion here ? I feel like we decided to move in the opposite direction

Yes, I agree that we should make recordupdates() also record copies. I'll update this patch to do that eventually :)

(and to drop this series). Is that right ?

Most if the series would still be unchanged, I think. I could drop this patch and replace it by another patch, but I could also reuse the Phabricator review for the new purpose. Doesn't matter much to me.

Can you mark them as "changes planned" in the mean time to clear up yadda?

Can you mark them as "changes planned" in the mean time to clear up yadda?

There are no actual changes planned and I guess I figured that the base patch being marked as "changes planned" would be sufficient, so sure, I'll update the other ones to that status too.

Can you mark them as "changes planned" in the mean time to clear up yadda?

There are no actual changes planned and I guess I figured that the base patch being marked as "changes planned" would be sufficient, so sure, I'll update the other ones to that status too.

My main goal here is to prevent yadda to be over 50% of content to review that is actually not to review.