This is an archive of the discontinued Mercurial Phabricator instance.

Differential D8563

localrepo: get mergestate via context
AbandonedPublic

Authored by durin42 on May 18 2020, 6:08 PM.

Details

Reviewers
marmoute
Group Reviewers
hg-reviewers

Diff Detail

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

Event Timeline

durin42 created this revision.May 18 2020, 6:08 PM
martinvonz added a subscriber: martinvonz.May 19 2020, 2:30 AM

It looks like the latter two hunks in this file will need to be able to read the merge state from an overlayworkingctx. Maybe that's coming in a later patch, but I think it would be easier to follow if this code was changed only then (very much related to my comment on an earlier patch).

durin42 added a comment.May 19 2020, 3:18 PM

Yeah, I'm happy to reorder patches, this is still a bit "work-in-progress unfortunately.

durin42 updated this revision to Diff 21509.May 28 2020, 4:26 PM
marmoute added a subscriber: marmoute.Jun 8 2020, 1:57 PM

@martinvonz do you mean that these patch break normal behavior if taken without other patch ?

martinvonz added a comment.Jun 8 2020, 2:27 PM
In D8563#128618, @marmoute wrote:

@martinvonz do you mean that these patch break normal behavior if taken without other patch ?

No, but I think those two places should be able to accept an overlayworkingctx, so it might be better to leave them unchanged for now and to switch them directly to use wctx.mergestate() once we have a wctx passed in here.

marmoute added a comment.Jun 9 2020, 4:41 AM

Is your concern about the creation of new "independant" wctc (by each call of repo[None]) instead of reusing the same wctx for the full operation ?

martinvonz added a comment.Jun 9 2020, 12:46 PM
In D8563#128630, @marmoute wrote:

Is your concern about the creation of new "independant" wctc (by each call of repo[None]) instead of reusing the same wctx for the full operation ?

Yes. I guess I'd rather have the pattern wctx.mergestate() indicate that it works with either a workingctx or a overlayworkingctx, while direct access to mergestate.read() indicates that we care about the actual working copy.

marmoute added a comment.Jun 10 2020, 4:55 AM
In D8563#128768, @martinvonz wrote:
In D8563#128630, @marmoute wrote:

Is your concern about the creation of new "independant" wctc (by each call of repo[None]) instead of reusing the same wctx for the full operation ?

Yes. I guess I'd rather have the pattern wctx.mergestate() indicate that it works with either a workingctx or a overlayworkingctx, while direct access to mergestate.read() indicates that we care about the actual working copy.

Is there any reason to distinct the two ? From afar, it would be better for the mergestate module to become an implementation details and every access to happen throught the context for consistency.

Do you have concrêt example where distincting the two seems better ?

durin42 added a comment.Jun 11 2020, 12:05 PM
In D8563#128829, @marmoute wrote:
In D8563#128768, @martinvonz wrote:
In D8563#128630, @marmoute wrote:

Is your concern about the creation of new "independant" wctc (by each call of repo[None]) instead of reusing the same wctx for the full operation ?

Yes. I guess I'd rather have the pattern wctx.mergestate() indicate that it works with either a workingctx or a overlayworkingctx, while direct access to mergestate.read() indicates that we care about the actual working copy.

Is there any reason to distinct the two ? From afar, it would be better for the mergestate module to become an implementation details and every access to happen throught the context for consistency.
Do you have concrêt example where distincting the two seems better ?

Should we have a brief email thread or IRC discussion about this? We seem to be conflating workingctx and overlayworkingctx here, and I'm not sure we're all on the same page.

durin42 updated this revision to Diff 21609.Jun 11 2020, 12:07 PM
marmoute added a comment.Jun 11 2020, 12:25 PM

I can probably do a VC tomorrow (US morning time) if that help

durin42 added a comment.Jun 11 2020, 1:06 PM

Okay, now localrepo correctly uses the same context across whole operations. This almost certainly avoids some bugs, given that committing a memctx was (surprise!) reading the mergestate from disk.

marmoute requested changes to this revision.Jul 31 2020, 12:02 PM

According to IRC discussion, this series is not ready for review, so moving it out of yadda.

This revision now requires changes to proceed.Jul 31 2020, 12:02 PM
durin42 updated this revision to Diff 23026.Oct 2 2020, 2:45 PM
durin42 planned changes to this revision.Oct 2 2020, 2:47 PM
durin42 abandoned this revision.Jan 19 2021, 1:50 PM

Revision Contents
Changeset List

Diff 21619

mercurial/localrepo.py

b'revisions %d and %d\n' b'revisions %d and %d\n'
) )
% parents % parents
) )
else: else:
ui.status( ui.status(
_(b'working directory now based on revision %d\n') % parents _(b'working directory now based on revision %d\n') % parents
) )
mergestatemod.mergestate.clean(self, self[b'.'].node()) self[None].mergestate(clean=True)
# TODO: if we know which new heads may result from this rollback, pass # TODO: if we know which new heads may result from this rollback, pass
# them to destroy(), which will prevent the branchhead cache from being # them to destroy(), which will prevent the branchhead cache from being
# invalidated. # invalidated.
self.destroyed() self.destroyed()
return 0 return 0
def _buildcacheupdater(self, newtransaction): def _buildcacheupdater(self, newtransaction):
return l return l
def currentwlock(self): def currentwlock(self):
"""Returns the wlock if it's held, or None if it's not.""" """Returns the wlock if it's held, or None if it's not."""
return self._currentlock(self._wlockref) return self._currentlock(self._wlockref)
def _filecommit( def _filecommit(
self, self,
ctx,
fctx, fctx,
manifest1, manifest1,
manifest2, manifest2,
linkrev, linkrev,
tr, tr,
changelist, changelist,
includecopymeta, includecopymeta,
): ):
# is one parent an ancestor of the other? # is one parent an ancestor of the other?
fparentancestors = flog.commonancestorsheads(fparent1, fparent2) fparentancestors = flog.commonancestorsheads(fparent1, fparent2)
if fparent1 in fparentancestors: if fparent1 in fparentancestors:
fparent1, fparent2 = fparent2, nullid fparent1, fparent2 = fparent2, nullid
elif fparent2 in fparentancestors: elif fparent2 in fparentancestors:
fparent2 = nullid fparent2 = nullid
elif not fparentancestors: elif not fparentancestors:
# TODO: this whole if-else might be simplified much more # TODO: this whole if-else might be simplified much more
ms = mergestatemod.mergestate.read(self) ms = ctx.mergestate()
if ( if (
fname in ms fname in ms
and ms[fname] == mergestatemod.MERGE_RECORD_MERGED_OTHER and ms[fname] == mergestatemod.MERGE_RECORD_MERGED_OTHER
): ):
fparent1, fparent2 = fparent2, nullid fparent1, fparent2 = fparent2, nullid
# is the file changed? # is the file changed?
text = fctx.data() text = fctx.data()
# make sure all explicit patterns are matched # make sure all explicit patterns are matched
if not force: if not force:
self.checkcommitpatterns(wctx, match, status, fail) self.checkcommitpatterns(wctx, match, status, fail)
cctx = context.workingcommitctx( cctx = context.workingcommitctx(
self, status, text, user, date, extra self, status, text, user, date, extra
) )
ms = mergestatemod.mergestate.read(self) ms = wctx.mergestate()
mergeutil.checkunresolved(ms) mergeutil.checkunresolved(ms)
# internal config: ui.allowemptycommit # internal config: ui.allowemptycommit
allowemptycommit = ( allowemptycommit = (
wctx.branch() != wctx.p1().branch() wctx.branch() != wctx.p1().branch()
or extra.get(b'close') or extra.get(b'close')
or merge or merge
or cctx.files() or cctx.files()
self.ui.note(uipathfn(f) + b"\n") self.ui.note(uipathfn(f) + b"\n")
try: try:
fctx = ctx[f] fctx = ctx[f]
if fctx is None: if fctx is None:
removed.append(f) removed.append(f)
else: else:
added.append(f) added.append(f)
m[f] = self._filecommit( m[f] = self._filecommit(
ctx,
fctx, fctx,
m1, m1,
m2, m2,
linkrev, linkrev,
trp, trp,
changed, changed,
writefilecopymeta, writefilecopymeta,
) )

tests/test-annotate.t

and (2) the extension to allow filelog merging between the revision and (2) the extension to allow filelog merging between the revision
and its ancestor by overriding "repo._filecommit". and its ancestor by overriding "repo._filecommit".
$ cat > ../legacyrepo.py <<EOF $ cat > ../legacyrepo.py <<EOF
> from __future__ import absolute_import > from __future__ import absolute_import
> from mercurial import error, node > from mercurial import error, node
> def reposetup(ui, repo): > def reposetup(ui, repo):
> class legacyrepo(repo.__class__): > class legacyrepo(repo.__class__):
> def _filecommit(self, fctx, manifest1, manifest2, > def _filecommit(self, ctx, fctx, manifest1, manifest2,
> linkrev, tr, changelist, includecopymeta): > linkrev, tr, changelist, includecopymeta):
> fname = fctx.path() > fname = fctx.path()
> text = fctx.data() > text = fctx.data()
> flog = self.file(fname) > flog = self.file(fname)
> fparent1 = manifest1.get(fname, node.nullid) > fparent1 = manifest1.get(fname, node.nullid)
> fparent2 = manifest2.get(fname, node.nullid) > fparent2 = manifest2.get(fname, node.nullid)
> meta = {} > meta = {}
> copy = fctx.copysource() > copy = fctx.copysource()

tests/test-fastannotate-hg.t

"hg debugsetparents" to merge without ancestor check by "hg merge", "hg debugsetparents" to merge without ancestor check by "hg merge",
and (2) the extension to allow filelog merging between the revision and (2) the extension to allow filelog merging between the revision
and its ancestor by overriding "repo._filecommit". and its ancestor by overriding "repo._filecommit".
$ cat > ../legacyrepo.py <<EOF $ cat > ../legacyrepo.py <<EOF
> from mercurial import error, node > from mercurial import error, node
> def reposetup(ui, repo): > def reposetup(ui, repo):
> class legacyrepo(repo.__class__): > class legacyrepo(repo.__class__):
> def _filecommit(self, fctx, manifest1, manifest2, > def _filecommit(self, ctx, fctx, manifest1, manifest2,
> linkrev, tr, changelist, includecopymeta): > linkrev, tr, changelist, includecopymeta):
> fname = fctx.path() > fname = fctx.path()
> text = fctx.data() > text = fctx.data()
> flog = self.file(fname) > flog = self.file(fname)
> fparent1 = manifest1.get(fname, node.nullid) > fparent1 = manifest1.get(fname, node.nullid)
> fparent2 = manifest2.get(fname, node.nullid) > fparent2 = manifest2.get(fname, node.nullid)
> meta = {} > meta = {}
> copy = fctx.renamed() > copy = fctx.renamed()