Patchwork D2698: merge: use constants for merge state record types

login
register
mail settings
Submitter phabricator
Date March 6, 2018, 12:15 a.m.
Message ID <differential-rev-PHID-DREV-uylkwamyinhpwutxwuhw-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/29050/
State Superseded
Headers show

Comments

phabricator - March 6, 2018, 12:15 a.m.
indygreg created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  merge.py is using multiple discrete sets of 1 and 2 letter constants
  to define types and behavior. To the uninitiated, the code is very
  difficult to reason about. I didn't even realize there were multiple
  sets of constants in play initially!
  
  We begin our sanity injection with merge state records. The record
  types (which are serialized to disk) are now defined in RECORD_*
  constants.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2698

AFFECTED FILES
  mercurial/merge.py

CHANGE DETAILS




To: indygreg, #hg-reviewers
Cc: mercurial-devel
phabricator - March 22, 2018, 12:35 a.m.
martinvonz added inline comments.

INLINE COMMENTS

> merge.py:50
>  
> +# Merge state record types. See ``mergestate`` docs for more.
> +RECORD_LOCAL = b'L'

Perhaps we should move that documentation to here so we don't have two lists of the same values that can end up getting out of sync? That would also make it easier to review this patch because the descriptions would be right next to the names you picked.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2698

To: indygreg, #hg-reviewers
Cc: martinvonz, mercurial-devel

Patch

diff --git a/mercurial/merge.py b/mercurial/merge.py
--- a/mercurial/merge.py
+++ b/mercurial/merge.py
@@ -47,6 +47,20 @@ 
     bits = bits[:-2] + bits[-1:]
     return '\0'.join(bits)
 
+# Merge state record types. See ``mergestate`` docs for more.
+RECORD_LOCAL = b'L'
+RECORD_OTHER = b'O'
+RECORD_MERGED = b'F'
+RECORD_CHANGEDELETE_CONFLICT = b'C'
+RECORD_MERGE_DRIVER_MERGE = b'D'
+RECORD_PATH_CONFLICT = b'P'
+RECORD_MERGE_DRIVER_STATE = b'm'
+RECORD_FILE_VALUES = b'f'
+RECORD_LABELS = b'l'
+RECORD_OVERRIDE = b't'
+RECORD_UNSUPPORTED_MANDATORY = b'X'
+RECORD_UNSUPPORTED_ADVISORY = b'x'
+
 class mergestate(object):
     '''track 3-way merge state of individual files
 
@@ -158,23 +172,24 @@ 
         unsupported = set()
         records = self._readrecords()
         for rtype, record in records:
-            if rtype == 'L':
+            if rtype == RECORD_LOCAL:
                 self._local = bin(record)
-            elif rtype == 'O':
+            elif rtype == RECORD_OTHER:
                 self._other = bin(record)
-            elif rtype == 'm':
+            elif rtype == RECORD_MERGE_DRIVER_STATE:
                 bits = record.split('\0', 1)
                 mdstate = bits[1]
                 if len(mdstate) != 1 or mdstate not in 'ums':
                     # the merge driver should be idempotent, so just rerun it
                     mdstate = 'u'
 
                 self._readmergedriver = bits[0]
                 self._mdstate = mdstate
-            elif rtype in 'FDCP':
+            elif rtype in (RECORD_MERGED, RECORD_CHANGEDELETE_CONFLICT,
+                           RECORD_PATH_CONFLICT, RECORD_MERGE_DRIVER_MERGE):
                 bits = record.split('\0')
                 self._state[bits[0]] = bits[1:]
-            elif rtype == 'f':
+            elif rtype == RECORD_FILE_VALUES:
                 filename, rawextras = record.split('\0', 1)
                 extraparts = rawextras.split('\0')
                 extras = {}
@@ -184,7 +199,7 @@ 
                     i += 2
 
                 self._stateextras[filename] = extras
-            elif rtype == 'l':
+            elif rtype == RECORD_LABELS:
                 labels = record.split('\0', 2)
                 self._labels = [l for l in labels if len(l) > 0]
             elif not rtype.islower():
@@ -218,25 +233,25 @@ 
             # we have to infer the "other" changeset of the merge
             # we cannot do better than that with v1 of the format
             mctx = self._repo[None].parents()[-1]
-            v1records.append(('O', mctx.hex()))
+            v1records.append((RECORD_OTHER, mctx.hex()))
             # add place holder "other" file node information
             # nobody is using it yet so we do no need to fetch the data
             # if mctx was wrong `mctx[bits[-2]]` may fails.
             for idx, r in enumerate(v1records):
-                if r[0] == 'F':
+                if r[0] == RECORD_MERGED:
                     bits = r[1].split('\0')
                     bits.insert(-2, '')
                     v1records[idx] = (r[0], '\0'.join(bits))
             return v1records
 
     def _v1v2match(self, v1records, v2records):
         oldv2 = set() # old format version of v2 record
         for rec in v2records:
-            if rec[0] == 'L':
+            if rec[0] == RECORD_LOCAL:
                 oldv2.add(rec)
-            elif rec[0] == 'F':
+            elif rec[0] == RECORD_MERGED:
                 # drop the onode data (not contained in v1)
-                oldv2.add(('F', _droponode(rec[1])))
+                oldv2.add((RECORD_MERGED, _droponode(rec[1])))
         for rec in v1records:
             if rec not in oldv2:
                 return False
@@ -256,9 +271,9 @@ 
             f = self._repo.vfs(self.statepathv1)
             for i, l in enumerate(f):
                 if i == 0:
-                    records.append(('L', l[:-1]))
+                    records.append((RECORD_LOCAL, l[:-1]))
                 else:
-                    records.append(('F', l[:-1]))
+                    records.append((RECORD_MERGED, l[:-1]))
             f.close()
         except IOError as err:
             if err.errno != errno.ENOENT:
@@ -296,7 +311,7 @@ 
                 off += 4
                 record = data[off:(off + length)]
                 off += length
-                if rtype == 't':
+                if rtype == RECORD_OVERRIDE:
                     rtype, record = record[0:1], record[1:]
                 records.append((rtype, record))
             f.close()
@@ -359,10 +374,10 @@ 
 
     def _makerecords(self):
         records = []
-        records.append(('L', hex(self._local)))
-        records.append(('O', hex(self._other)))
+        records.append((RECORD_LOCAL, hex(self._local)))
+        records.append((RECORD_OTHER, hex(self._other)))
         if self.mergedriver:
-            records.append(('m', '\0'.join([
+            records.append((RECORD_MERGE_DRIVER_STATE, '\0'.join([
                 self.mergedriver, self._mdstate])))
         # Write out state items. In all cases, the value of the state map entry
         # is written as the contents of the record. The record type depends on
@@ -372,27 +387,32 @@ 
         for filename, v in self._state.iteritems():
             if v[0] == 'd':
                 # Driver-resolved merge. These are stored in 'D' records.
-                records.append(('D', '\0'.join([filename] + v)))
+                records.append((RECORD_MERGE_DRIVER_MERGE,
+                                '\0'.join([filename] + v)))
             elif v[0] in ('pu', 'pr'):
                 # Path conflicts. These are stored in 'P' records.  The current
                 # resolution state ('pu' or 'pr') is stored within the record.
-                records.append(('P', '\0'.join([filename] + v)))
+                records.append((RECORD_PATH_CONFLICT,
+                                '\0'.join([filename] + v)))
             elif v[1] == nullhex or v[6] == nullhex:
                 # Change/Delete or Delete/Change conflicts. These are stored in
                 # 'C' records. v[1] is the local file, and is nullhex when the
                 # file is deleted locally ('dc'). v[6] is the remote file, and
                 # is nullhex when the file is deleted remotely ('cd').
-                records.append(('C', '\0'.join([filename] + v)))
+                records.append((RECORD_CHANGEDELETE_CONFLICT,
+                                '\0'.join([filename] + v)))
             else:
                 # Normal files.  These are stored in 'F' records.
-                records.append(('F', '\0'.join([filename] + v)))
+                records.append((RECORD_MERGED,
+                                '\0'.join([filename] + v)))
         for filename, extras in sorted(self._stateextras.iteritems()):
             rawextras = '\0'.join('%s\0%s' % (k, v) for k, v in
                                   extras.iteritems())
-            records.append(('f', '%s\0%s' % (filename, rawextras)))
+            records.append((RECORD_FILE_VALUES,
+                            '%s\0%s' % (filename, rawextras)))
         if self._labels is not None:
             labels = '\0'.join(self._labels)
-            records.append(('l', labels))
+            records.append((RECORD_LABELS, labels))
         return records
 
     def _writerecords(self, records):
@@ -405,24 +425,24 @@ 
         f = self._repo.vfs(self.statepathv1, 'wb')
         irecords = iter(records)
         lrecords = next(irecords)
-        assert lrecords[0] == 'L'
+        assert lrecords[0] == RECORD_LOCAL
         f.write(hex(self._local) + '\n')
         for rtype, data in irecords:
-            if rtype == 'F':
+            if rtype == RECORD_MERGED:
                 f.write('%s\n' % _droponode(data))
         f.close()
 
     def _writerecordsv2(self, records):
         """Write current state on disk in a version 2 file
 
         See the docstring for _readrecordsv2 for why we use 't'."""
         # these are the records that all version 2 clients can read
-        whitelist = 'LOF'
+        allowlist = (RECORD_LOCAL, RECORD_OTHER, RECORD_MERGED)
         f = self._repo.vfs(self.statepathv2, 'wb')
         for key, data in records:
             assert len(key) == 1
-            if key not in whitelist:
-                key, data = 't', '%s%s' % (key, data)
+            if key not in allowlist:
+                key, data = RECORD_OVERRIDE, '%s%s' % (key, data)
             format = '>sI%is' % len(data)
             f.write(_pack(format, key, len(data), data))
         f.close()