This is an archive of the discontinued Mercurial Phabricator instance.

Differential D774

merge: add pathconflict merge state
ClosedPublic

Authored by mbthomas on Sep 22 2017, 5:27 AM.

Details

Reviewers
ryanmce
Group Reviewers
hg-reviewers
Commits
rHG1913162854f2: merge: add pathconflict merge state
rHG2459427e7f57: merge: add pathconflict merge state
Summary

Path conflicts that occur during merges are represented by 'pu' (unresolved)
and 'pr' (resolved) records in the merge state. These are stored on disk
in 'P' records.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

mbthomas created this revision.Sep 22 2017, 5:27 AM
ryanmce requested changes to this revision.Sep 25 2017, 3:07 PM
ryanmce added a subscriber: ryanmce.

Logic looks good to me. Just a couple of inline nits about comments to improve understanding that I think would be useful.

mercurial/merge.py
84–87

I think it's worth mentioning why these need to be separate states (eg, so you can round-trip from resolved path conflict to unresolved path conflict)

88

Thanks for adding this!

364–365

Even though there's not a lot of good comments here, now, could you add some when you modify this?

Something like "if the state includes a path conflict (pu or pr), include a Path (P) record which ...." (I don't know what to write here -- why don't we need to distinguish between pu and pr in the record? A reader should not have to figure this out on their own, ideally.

434

Maybe this should be called addpathconflict?

This revision now requires changes to proceed.Sep 25 2017, 3:07 PM
mbthomas updated this revision to Diff 2210.Oct 1 2017, 5:32 AM
mbthomas marked 2 inline comments as done.Oct 1 2017, 6:38 AM
mbthomas added inline comments.
mercurial/merge.py
434

I named it addpath for symmetry with add. The merge state stores conflicts, so it seems superfluous to have it in the name.

• kiilerix added a subscriber: • kiilerix.Oct 1 2017, 6:49 AM
• kiilerix added inline comments.
mercurial/merge.py
88

Some of this is adding documentation for existing functionality? It could be nice to have this changeset show exactly what it is adding.

mbthomas updated this revision to Diff 2351.Oct 2 2017, 5:15 PM
mbthomas added inline comments.Oct 5 2017, 10:45 AM
mercurial/merge.py
364–365

This could use some improvement in general, so I'll write some comments on how this function works as a follow-up patch.

ryanmce accepted this revision.Oct 5 2017, 10:48 AM

lgtm

mercurial/merge.py
88

I'm not sure it makes sense to split out a one line comment addition into a separate patch -- but will let a more experienced reviewer make the call here.

364–365

that works for me

434

makes sense

Closed by commit rHG2459427e7f57: merge: add pathconflict merge state (authored by mbthomas). · Explain WhyOct 9 2017, 9:25 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/merge.py (22 lines)

Diff 2210

mercurial/merge.py

Currently known records: Currently known records:
L: the node of the "local" part of the merge (hexified version) L: the node of the "local" part of the merge (hexified version)
O: the node of the "other" part of the merge (hexified version) O: the node of the "other" part of the merge (hexified version)
F: a file to be merged entry F: a file to be merged entry
C: a change/delete or delete/change conflict C: a change/delete or delete/change conflict
D: a file that the external merge driver will merge internally D: a file that the external merge driver will merge internally
(experimental) (experimental)
P: a path conflict (file vs directory)
m: the external merge driver defined for this merge plus its run state m: the external merge driver defined for this merge plus its run state
(experimental) (experimental)
f: a (filename, dictionary) tuple of optional values for a given file f: a (filename, dictionary) tuple of optional values for a given file
X: unsupported mandatory record type (used in tests) X: unsupported mandatory record type (used in tests)
x: unsupported advisory record type (used in tests) x: unsupported advisory record type (used in tests)
l: the labels for the parts of the merge. l: the labels for the parts of the merge.
Merge driver run states (experimental): Merge driver run states (experimental):
u: driver-resolved files unmarked -- needs to be run next time we're about u: driver-resolved files unmarked -- needs to be run next time we're about
to resolve or commit to resolve or commit
m: driver-resolved files marked -- only needs to be run before commit m: driver-resolved files marked -- only needs to be run before commit
s: success/skipped -- does not need to be run any more s: success/skipped -- does not need to be run any more
Merge record states (stored in self._state, indexed by filename):
u: unresolved conflict
r: resolved conflict
pu: unresolved path conflict (file conflicts with directory)
pr: resolved path conflict
ryanmceUnsubmitted
Done

I think it's worth mentioning why these need to be separate states (eg, so you can round-trip from resolved path conflict to unresolved path conflict)

ryanmce: I think it's worth mentioning why these need to be separate states (eg, so you can round-trip…
d: driver-resolved conflict
ryanmceUnsubmitted
Done

Thanks for adding this!

ryanmce: Thanks for adding this!
kiilerixUnsubmitted
Not Done

Some of this is adding documentation for existing functionality? It could be nice to have this changeset show exactly what it is adding.

kiilerix: Some of this is adding documentation for existing functionality? It could be nice to have this…
ryanmceUnsubmitted
Not Done

I'm not sure it makes sense to split out a one line comment addition into a separate patch -- but will let a more experienced reviewer make the call here.

ryanmce: I'm not sure it makes sense to split out a one line comment addition into a separate patch…
''' '''
statepathv1 = 'merge/state' statepathv1 = 'merge/state'
statepathv2 = 'merge/state2' statepathv2 = 'merge/state2'
@staticmethod @staticmethod
def clean(repo, node=None, other=None, labels=None): def clean(repo, node=None, other=None, labels=None):
"""Initialize a brand new merge state, removing any existing state on """Initialize a brand new merge state, removing any existing state on
disk.""" disk."""
bits = record.split('\0', 1) bits = record.split('\0', 1)
mdstate = bits[1] mdstate = bits[1]
if len(mdstate) != 1 or mdstate not in 'ums': if len(mdstate) != 1 or mdstate not in 'ums':
# the merge driver should be idempotent, so just rerun it # the merge driver should be idempotent, so just rerun it
mdstate = 'u' mdstate = 'u'
self._readmergedriver = bits[0] self._readmergedriver = bits[0]
self._mdstate = mdstate self._mdstate = mdstate
elif rtype in 'FDC': elif rtype in 'FDCP':
bits = record.split('\0') bits = record.split('\0')
self._state[bits[0]] = bits[1:] self._state[bits[0]] = bits[1:]
elif rtype == 'f': elif rtype == 'f':
filename, rawextras = record.split('\0', 1) filename, rawextras = record.split('\0', 1)
extraparts = rawextras.split('\0') extraparts = rawextras.split('\0')
extras = {} extras = {}
i = 0 i = 0
while i < len(extraparts): while i < len(extraparts):
records.append(('L', hex(self._local))) records.append(('L', hex(self._local)))
records.append(('O', hex(self._other))) records.append(('O', hex(self._other)))
if self.mergedriver: if self.mergedriver:
records.append(('m', '\0'.join([ records.append(('m', '\0'.join([
self.mergedriver, self._mdstate]))) self.mergedriver, self._mdstate])))
for d, v in self._state.iteritems(): for d, v in self._state.iteritems():
if v[0] == 'd': if v[0] == 'd':
records.append(('D', '\0'.join([d] + v))) records.append(('D', '\0'.join([d] + v)))
elif v[0] in ('pu', 'pr'):
records.append(('P', '\0'.join([d] + v)))
ryanmceUnsubmitted
Not Done

Even though there's not a lot of good comments here, now, could you add some when you modify this?

Something like "if the state includes a path conflict (pu or pr), include a Path (P) record which ...." (I don't know what to write here -- why don't we need to distinguish between pu and pr in the record? A reader should not have to figure this out on their own, ideally.

ryanmce: Even though there's not a lot of good comments here, now, could you add some when you modify…
mbthomasAuthorUnsubmitted
Not Done

This could use some improvement in general, so I'll write some comments on how this function works as a follow-up patch.

mbthomas: This could use some improvement in general, so I'll write some comments on how this function…
ryanmceUnsubmitted
Not Done

that works for me

ryanmce: that works for me
# v[1] == local ('cd'), v[6] == other ('dc') -- not supported by # v[1] == local ('cd'), v[6] == other ('dc') -- not supported by
# older versions of Mercurial # older versions of Mercurial
elif v[1] == nullhex or v[6] == nullhex: elif v[1] == nullhex or v[6] == nullhex:
records.append(('C', '\0'.join([d] + v))) records.append(('C', '\0'.join([d] + v)))
else: else:
records.append(('F', '\0'.join([d] + v))) records.append(('F', '\0'.join([d] + v)))
for filename, extras in sorted(self._stateextras.iteritems()): for filename, extras in sorted(self._stateextras.iteritems()):
rawextras = '\0'.join('%s\0%s' % (k, v) for k, v in rawextras = '\0'.join('%s\0%s' % (k, v) for k, v in
self._repo.vfs.write('merge/' + hash, fcl.data()) self._repo.vfs.write('merge/' + hash, fcl.data())
self._state[fd] = ['u', hash, fcl.path(), self._state[fd] = ['u', hash, fcl.path(),
fca.path(), hex(fca.filenode()), fca.path(), hex(fca.filenode()),
fco.path(), hex(fco.filenode()), fco.path(), hex(fco.filenode()),
fcl.flags()] fcl.flags()]
self._stateextras[fd] = { 'ancestorlinknode' : hex(fca.node()) } self._stateextras[fd] = { 'ancestorlinknode' : hex(fca.node()) }
self._dirty = True self._dirty = True
def addpath(self, path, frename, forigin):
ryanmceUnsubmitted
Not Done

Maybe this should be called addpathconflict?

ryanmce: Maybe this should be called `addpathconflict`?
mbthomasAuthorUnsubmitted
Not Done

I named it addpath for symmetry with add. The merge state stores conflicts, so it seems superfluous to have it in the name.

mbthomas: I named it `addpath` for symmetry with `add`. The merge state stores conflicts, so it seems…
ryanmceUnsubmitted
Not Done

makes sense

ryanmce: makes sense
"""add a new conflicting path to the merge state
path: the path that conflicts
frename: the filename the conflicting file was renamed to
forigin: origin of the file ('l' or 'r' for local/remote)
"""
self._state[path] = ['pu', frename, forigin]
self._dirty = True
def __contains__(self, dfile): def __contains__(self, dfile):
return dfile in self._state return dfile in self._state
def __getitem__(self, dfile): def __getitem__(self, dfile):
return self._state[dfile][0] return self._state[dfile][0]
def __iter__(self): def __iter__(self):
return iter(sorted(self._state)) return iter(sorted(self._state))
def files(self): def files(self):
return self._state.keys() return self._state.keys()
def mark(self, dfile, state): def mark(self, dfile, state):
self._state[dfile][0] = state self._state[dfile][0] = state
self._dirty = True self._dirty = True
def mdstate(self): def mdstate(self):
return self._mdstate return self._mdstate
def unresolved(self): def unresolved(self):
"""Obtain the paths of unresolved files.""" """Obtain the paths of unresolved files."""
for f, entry in self._state.iteritems(): for f, entry in self._state.iteritems():
if entry[0] == 'u': if entry[0] in ('u', 'pu'):
yield f yield f
def driverresolved(self): def driverresolved(self):
"""Obtain the paths of driver-resolved files.""" """Obtain the paths of driver-resolved files."""
for f, entry in self._state.items(): for f, entry in self._state.items():
if entry[0] == 'd': if entry[0] == 'd':
yield f yield f