This is an archive of the discontinued Mercurial Phabricator instance.

Differential D8596

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 added a subscriber: durin42.May 29 2020, 12:00 PM
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.
In D8596#128319, @martinvonz wrote:

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
In D8596#128704, @marmoute wrote:
In D8596#128319, @martinvonz wrote:

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
In D8596#128750, @martinvonz wrote:
In D8596#128704, @marmoute wrote:
In D8596#128319, @martinvonz wrote:

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)

martinvonz added a comment.Jun 9 2020, 12:06 PM
In D8596#128758, @marmoute wrote:
In D8596#128750, @martinvonz wrote:
In D8596#128704, @marmoute wrote:
In D8596#128319, @martinvonz wrote:

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.

marmoute added a comment.Jun 10 2020, 1:40 PM

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.

marmoute added a comment.Jul 15 2020, 12:16 PM

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

martinvonz added a comment.Jul 15 2020, 12:20 PM
In D8596#130671, @marmoute wrote:

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.

marmoute added a comment.Jul 16 2020, 10:34 AM

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

martinvonz added a comment.Jul 16 2020, 11:23 AM
In D8596#130831, @marmoute wrote:

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.

marmoute added a comment.Jul 16 2020, 11:24 AM
In D8596#130836, @martinvonz wrote:
In D8596#130831, @marmoute wrote:

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.

Revision Contents
Changeset List

PathPackages
Mmercurial/merge.py (49 lines)
Mmercurial/mergestate.py (8 lines)

Diff 21607

mercurial/merge.py

mergestatemod.ACTION_KEEP, mergestatemod.ACTION_KEEP,
mergestatemod.ACTION_PATH_CONFLICT, mergestatemod.ACTION_PATH_CONFLICT,
mergestatemod.ACTION_PATH_CONFLICT_RESOLVE, mergestatemod.ACTION_PATH_CONFLICT_RESOLVE,
mergestatemod.ACTION_GET_OTHER_AND_STORE, mergestatemod.ACTION_GET_OTHER_AND_STORE,
) )
} }
def _extract_copies(actions, branchmerge):
"""extract a copy dict (dst->src) from a dict of merge actions"""
marmouteUnsubmitted
Not Done

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 ?

marmoute: I don't understand this docstring. From what I see of the code, it mostly pull things from an…
martinvonzAuthorUnsubmitted
Done

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.

martinvonz: > I don't understand this docstring. From what I see of the code, it mostly pull things from an…
copies = {}
# resolve path conflicts
for f, args, msg in actions.get(
mergestatemod.ACTION_PATH_CONFLICT_RESOLVE, []
):
(f0, origf0) = args
copies[f] = origf0
# merge
for f, args, msg in actions.get(mergestatemod.ACTION_MERGE, []):
f1, f2, fa, move, anc = args
if branchmerge:
if f1 != f2: # copy/rename
if f1 != f:
copies[f] = f1
else:
copies[f] = f2
# directory rename, move local
for f, args, msg in actions.get(
mergestatemod.ACTION_DIR_RENAME_MOVE_LOCAL, []
):
f0, flag = args
if branchmerge:
copies[f] = f0
# directory rename, get
for f, args, msg in actions.get(
mergestatemod.ACTION_LOCAL_DIR_RENAME_GET, []
):
f0, flag = args
if branchmerge:
copies[f] = f0
return copies
def applyupdates( def applyupdates(
repo, actions, wctx, mctx, overwrite, wantfiledata, labels=None repo, actions, wctx, mctx, overwrite, wantfiledata, labels=None
): ):
"""apply the merge action list to the working directory """apply the merge action list to the working directory
wctx is the working copy context wctx is the working copy context
mctx is the context to be merged into the working copy mctx is the context to be merged into the working copy
_( _(
b'(warning: large working directory being used without ' b'(warning: large working directory being used without '
b'fsmonitor enabled; enable fsmonitor to improve performance; ' b'fsmonitor enabled; enable fsmonitor to improve performance; '
b'see "hg help -e fsmonitor")\n' b'see "hg help -e fsmonitor")\n'
) )
) )
wantfiledata = updatedirstate and not branchmerge wantfiledata = updatedirstate and not branchmerge
copies = {}
# We don't want to record copies when doing `hg update -C` (which is
marmouteUnsubmitted
Not Done

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

marmoute: I don't know what `overwrite` means and the docstring does not mention it. Can you clarify this…
martinvonzAuthorUnsubmitted
Done

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.

martinvonz: It's defined on line 1631 and used a few times thereafter. Would you like a comment in each…
marmouteUnsubmitted
Not Done

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: no, phabricator diffing made me think we were in the following function, that takes it as an…
marmouteUnsubmitted
Not Done

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 ?

marmoute: Actually, thinking more about it, I still don't know why we should overlook rename information…
martinvonzAuthorUnsubmitted
Done

Good question! I've added a comment.

martinvonz: Good question! I've added a comment.
# what `overwrite` is about)
if not overwrite:
copies = _extract_copies(actions, branchmerge)
stats, getfiledata = applyupdates( stats, getfiledata = applyupdates(
repo, actions, wc, p2, overwrite, wantfiledata, labels=labels repo, actions, wc, p2, overwrite, wantfiledata, labels=labels
) )
if updatedirstate: if updatedirstate:
with repo.dirstate.parentchange(): with repo.dirstate.parentchange():
repo.setparents(fp1, fp2) repo.setparents(fp1, fp2)
wc = repo[None]
mergestatemod.recordupdates( mergestatemod.recordupdates(
repo, actions, branchmerge, getfiledata repo, actions, branchmerge, getfiledata
) )
# update completed, clear state # update completed, clear state
util.unlink(repo.vfs.join(b'updatestate')) util.unlink(repo.vfs.join(b'updatestate'))
if not branchmerge: if not branchmerge:
repo.dirstate.setbranch(p2.branch()) repo.dirstate.setbranch(p2.branch())
for dst, src in copies.items():
wc[dst].markcopied(src)
# If we're updating to a location, clean up any stale temporary includes # If we're updating to a location, clean up any stale temporary includes
# (ex: this happens during hg rebase --abort). # (ex: this happens during hg rebase --abort).
if not branchmerge: if not branchmerge:
sparse.prunetemporaryincludes(repo) sparse.prunetemporaryincludes(repo)
if updatedirstate: if updatedirstate:
repo.hook( repo.hook(
b'update', parent1=xp1, parent2=xp2, error=stats.unresolvedcount b'update', parent1=xp1, parent2=xp2, error=stats.unresolvedcount

mercurial/mergestate.py

"""record merge actions to the dirstate""" """record merge actions to the dirstate"""
# remove (must come first) # remove (must come first)
for f, args, msg in actions.get(ACTION_REMOVE, []): for f, args, msg in actions.get(ACTION_REMOVE, []):
if branchmerge: if branchmerge:
repo.dirstate.remove(f) repo.dirstate.remove(f)
else: else:
repo.dirstate.drop(f) repo.dirstate.drop(f)
# forget (must come first) # forget (must come first)
marmouteUnsubmitted
Not Done

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.

marmoute: Okay, so why do we have both a `applyupdates` and `recordupdates` function ? What are their…
martinvonzAuthorUnsubmitted
Done

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

martinvonz: Yes, that is correct about their roles. See commit message for my reasoning.
marmouteUnsubmitted
Not Done

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?

marmoute: Okay, then it feel like we are going the in wrong direction and breaking the API contract here.
martinvonzAuthorUnsubmitted
Done

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

martinvonz: Yes, I was also feeling that more and more. We should probably replicate many dirstate methods…
for f, args, msg in actions.get(ACTION_FORGET, []): for f, args, msg in actions.get(ACTION_FORGET, []):
repo.dirstate.drop(f) repo.dirstate.drop(f)
# resolve path conflicts # resolve path conflicts
for f, args, msg in actions.get(ACTION_PATH_CONFLICT_RESOLVE, []): for f, args, msg in actions.get(ACTION_PATH_CONFLICT_RESOLVE, []):
(f0, origf0) = args (f0, origf0) = args
repo.dirstate.add(f) repo.dirstate.add(f)
repo.dirstate.copy(origf0, f)
if f0 == origf0: if f0 == origf0:
repo.dirstate.remove(f0) repo.dirstate.remove(f0)
else: else:
repo.dirstate.drop(f0) repo.dirstate.drop(f0)
# re-add # re-add
for f, args, msg in actions.get(ACTION_ADD, []): for f, args, msg in actions.get(ACTION_ADD, []):
repo.dirstate.add(f) repo.dirstate.add(f)
f1, f2, fa, move, anc = args f1, f2, fa, move, anc = args
if branchmerge: if branchmerge:
# We've done a branch merge, mark this file as merged # We've done a branch merge, mark this file as merged
# so that we properly record the merger later # so that we properly record the merger later
repo.dirstate.merge(f) repo.dirstate.merge(f)
if f1 != f2: # copy/rename if f1 != f2: # copy/rename
if move: if move:
repo.dirstate.remove(f1) repo.dirstate.remove(f1)
if f1 != f:
repo.dirstate.copy(f1, f)
else:
repo.dirstate.copy(f2, f)
else: else:
# We've update-merged a locally modified file, so # We've update-merged a locally modified file, so
# we set the dirstate to emulate a normal checkout # we set the dirstate to emulate a normal checkout
# of that file some time in the past. Thus our # of that file some time in the past. Thus our
# merge will appear as a normal local file # merge will appear as a normal local file
# modification. # modification.
if f2 == f: # file not locally copied/moved if f2 == f: # file not locally copied/moved
repo.dirstate.normallookup(f) repo.dirstate.normallookup(f)
if move: if move:
repo.dirstate.drop(f1) repo.dirstate.drop(f1)
# directory rename, move local # directory rename, move local
for f, args, msg in actions.get(ACTION_DIR_RENAME_MOVE_LOCAL, []): for f, args, msg in actions.get(ACTION_DIR_RENAME_MOVE_LOCAL, []):
f0, flag = args f0, flag = args
if branchmerge: if branchmerge:
repo.dirstate.add(f) repo.dirstate.add(f)
repo.dirstate.remove(f0) repo.dirstate.remove(f0)
repo.dirstate.copy(f0, f)
else: else:
repo.dirstate.normal(f) repo.dirstate.normal(f)
repo.dirstate.drop(f0) repo.dirstate.drop(f0)
# directory rename, get # directory rename, get
for f, args, msg in actions.get(ACTION_LOCAL_DIR_RENAME_GET, []): for f, args, msg in actions.get(ACTION_LOCAL_DIR_RENAME_GET, []):
f0, flag = args
if branchmerge: if branchmerge:
repo.dirstate.add(f) repo.dirstate.add(f)
repo.dirstate.copy(f0, f)
else: else:
repo.dirstate.normal(f) repo.dirstate.normal(f)