Patchwork D9041: RFC: mergedriver: delete it

login
register
mail settings
Submitter phabricator
Date Sept. 18, 2020, 6:01 a.m.
Message ID <differential-rev-PHID-DREV-7u3dcjpq6qcwfi42tcmv-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/47203/
State Superseded
Headers show

Comments

phabricator - Sept. 18, 2020, 6:01 a.m.
martinvonz created this revision.
Herald added a reviewer: hg-reviewers.
Herald added a subscriber: mercurial-patches.

REVISION SUMMARY
  It's very poorly tested, which makes it very hard to maintain.

REPOSITORY
  rHG Mercurial

BRANCH
  default

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

AFFECTED FILES
  mercurial/commands.py
  mercurial/configitems.py
  mercurial/helptext/internals/mergestate.txt
  mercurial/merge.py
  mercurial/mergestate.py
  mercurial/mergeutil.py
  tests/test-resolve.t

CHANGE DETAILS




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

Patch

diff --git a/tests/test-resolve.t b/tests/test-resolve.t
--- a/tests/test-resolve.t
+++ b/tests/test-resolve.t
@@ -87,64 +87,6 @@ 
   $ cd ..
   $ rmdir nested
 
-don't allow marking or unmarking driver-resolved files
-
-  $ cat > $TESTTMP/markdriver.py << EOF
-  > '''mark and unmark files as driver-resolved'''
-  > from mercurial import (
-  >    mergestate,
-  >    pycompat,
-  >    registrar,
-  >    scmutil,
-  > )
-  > cmdtable = {}
-  > command = registrar.command(cmdtable)
-  > @command(b'markdriver',
-  >   [(b'u', b'unmark', None, b'')],
-  >   b'FILE...')
-  > def markdriver(ui, repo, *pats, **opts):
-  >     wlock = repo.wlock()
-  >     opts = pycompat.byteskwargs(opts)
-  >     try:
-  >         ms = mergestate.mergestate.read(repo)
-  >         m = scmutil.match(repo[None], pats, opts)
-  >         for f in ms:
-  >             if not m(f):
-  >                 continue
-  >             if not opts[b'unmark']:
-  >                 ms.mark(f, b'd')
-  >             else:
-  >                 ms.mark(f, b'u')
-  >         ms.commit()
-  >     finally:
-  >         wlock.release()
-  > EOF
-  $ hg --config extensions.markdriver=$TESTTMP/markdriver.py markdriver file1
-  $ hg resolve --list
-  D file1
-  U file2
-  $ hg resolve --mark file1
-  not marking file1 as it is driver-resolved
-this should not print out file1
-  $ hg resolve --mark --all
-  (no more unresolved files -- run "hg resolve --all" to conclude)
-  $ hg resolve --mark 'glob:file*'
-  (no more unresolved files -- run "hg resolve --all" to conclude)
-  $ hg resolve --list
-  D file1
-  R file2
-  $ hg resolve --unmark file1
-  not unmarking file1 as it is driver-resolved
-  (no more unresolved files -- run "hg resolve --all" to conclude)
-  $ hg resolve --unmark --all
-  $ hg resolve --list
-  D file1
-  U file2
-  $ hg --config extensions.markdriver=$TESTTMP/markdriver.py markdriver --unmark file1
-  $ hg resolve --list
-  U file1
-  U file2
-
 resolve the failure
 
   $ echo resolved > file1
diff --git a/mercurial/mergeutil.py b/mercurial/mergeutil.py
--- a/mercurial/mergeutil.py
+++ b/mercurial/mergeutil.py
@@ -17,8 +17,3 @@ 
         raise error.Abort(
             _(b"unresolved merge conflicts (see 'hg help resolve')")
         )
-    if ms.mdstate() != b's' or list(ms.driverresolved()):
-        raise error.Abort(
-            _(b'driver-resolved merge conflicts'),
-            hint=_(b'run "hg resolve --all" to resolve'),
-        )
diff --git a/mercurial/mergestate.py b/mercurial/mergestate.py
--- a/mercurial/mergestate.py
+++ b/mercurial/mergestate.py
@@ -48,8 +48,6 @@ 
 RECORD_OTHER = b'O'
 # record merge labels
 RECORD_LABELS = b'l'
-# store info about merge driver used and it's state
-RECORD_MERGE_DRIVER_STATE = b'm'
 
 #####
 # record extra information about files, with one entry containing info about one
@@ -65,7 +63,6 @@ 
 #####
 RECORD_MERGED = b'F'
 RECORD_CHANGEDELETE_CONFLICT = b'C'
-RECORD_MERGE_DRIVER_MERGE = b'D'
 # the path was dir on one side of merge and file on another
 RECORD_PATH_CONFLICT = b'P'
 
@@ -77,7 +74,6 @@ 
 MERGE_RECORD_RESOLVED = b'r'
 MERGE_RECORD_UNRESOLVED_PATH = b'pu'
 MERGE_RECORD_RESOLVED_PATH = b'pr'
-MERGE_RECORD_DRIVER_RESOLVED = b'd'
 # represents that the file was automatically merged in favor
 # of other version. This info is used on commit.
 # This is now deprecated and commit related information is now
@@ -91,18 +87,16 @@ 
 RECORD_OVERRIDE = b't'
 
 #####
-# possible states which a merge driver can have. These are stored inside a
-# RECORD_MERGE_DRIVER_STATE entry
-#####
-MERGE_DRIVER_STATE_UNMARKED = b'u'
-MERGE_DRIVER_STATE_MARKED = b'm'
-MERGE_DRIVER_STATE_SUCCESS = b's'
-
-#####
 # legacy records which are no longer used but kept to prevent breaking BC
 #####
 # This record was release in 5.4 and usage was removed in 5.5
 LEGACY_RECORD_RESOLVED_OTHER = b'R'
+# This record was release in 3.7 and usage was removed in 5.6
+LEGACY_RECORD_DRIVER_RESOLVED = b'd'
+# This record was release in 3.7 and usage was removed in 5.6
+LEGACY_MERGE_DRIVER_STATE = b'm'
+# This record was release in 3.7 and usage was removed in 5.6
+LEGACY_MERGE_DRIVER_MERGE = b'D'
 
 
 ACTION_FORGET = b'f'
@@ -147,26 +141,15 @@ 
     O: the node of the "other" part of the merge (hexified version)
     F: a file to be merged entry
     C: a change/delete or delete/change conflict
-    D: a file that the external merge driver will merge internally
-       (experimental)
     P: a path conflict (file vs directory)
-    m: the external merge driver defined for this merge plus its run state
-       (experimental)
     f: a (filename, dictionary) tuple of optional values for a given file
     l: the labels for the parts of the merge.
 
-    Merge driver run states (experimental):
-    u: driver-resolved files unmarked -- needs to be run next time we're about
-       to resolve or commit
-    m: driver-resolved files marked -- only needs to be run before commit
-    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
-    d: driver-resolved conflict
 
     The resolve command transitions between 'u' and 'r' for conflicts and
     'pu' and 'pr' for path conflicts.
@@ -182,8 +165,6 @@ 
         self._local = None
         self._other = None
         self._labels = None
-        self._readmergedriver = None
-        self._mdstate = MERGE_DRIVER_STATE_UNMARKED
         # contains a mapping of form:
         # {filename : (merge_return_value, action_to_be_performed}
         # these are results of re-running merge process
@@ -199,32 +180,6 @@ 
         self._local = node
         self._other = other
         self._labels = labels
-        if self.mergedriver:
-            self._mdstate = MERGE_DRIVER_STATE_SUCCESS
-
-    @util.propertycache
-    def mergedriver(self):
-        # protect against the following:
-        # - A configures a malicious merge driver in their hgrc, then
-        #   pauses the merge
-        # - A edits their hgrc to remove references to the merge driver
-        # - A gives a copy of their entire repo, including .hg, to B
-        # - B inspects .hgrc and finds it to be clean
-        # - B then continues the merge and the malicious merge driver
-        #  gets invoked
-        configmergedriver = self._repo.ui.config(
-            b'experimental', b'mergedriver'
-        )
-        if (
-            self._readmergedriver is not None
-            and self._readmergedriver != configmergedriver
-        ):
-            raise error.ConfigError(
-                _(b"merge driver changed since merge started"),
-                hint=_(b"revert merge driver change or abort merge"),
-            )
-
-        return configmergedriver
 
     @util.propertycache
     def local(self):
@@ -267,25 +222,13 @@ 
         records = []
         records.append((RECORD_LOCAL, hex(self._local)))
         records.append((RECORD_OTHER, hex(self._other)))
-        if self.mergedriver:
-            records.append(
-                (
-                    RECORD_MERGE_DRIVER_STATE,
-                    b'\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
         # the type of state that is stored, and capital-letter records are used
         # to prevent older versions of Mercurial that do not support the feature
         # from loading them.
         for filename, v in pycompat.iteritems(self._state):
-            if v[0] == MERGE_RECORD_DRIVER_RESOLVED:
-                # Driver-resolved merge. These are stored in 'D' records.
-                records.append(
-                    (RECORD_MERGE_DRIVER_MERGE, b'\0'.join([filename] + v))
-                )
-            elif v[0] in (
+            if v[0] in (
                 MERGE_RECORD_UNRESOLVED_PATH,
                 MERGE_RECORD_RESOLVED_PATH,
             ):
@@ -388,9 +331,6 @@ 
         self._state[dfile][0] = state
         self._dirty = True
 
-    def mdstate(self):
-        return self._mdstate
-
     def unresolved(self):
         """Obtain the paths of unresolved files."""
 
@@ -401,13 +341,6 @@ 
             ):
                 yield f
 
-    def driverresolved(self):
-        """Obtain the paths of driver-resolved files."""
-
-        for f, entry in self._state.items():
-            if entry[0] == MERGE_RECORD_DRIVER_RESOLVED:
-                yield f
-
     def extras(self, filename):
         return self._stateextras[filename]
 
@@ -416,7 +349,10 @@ 
         Returns whether the merge was completed and the return value of merge
         obtained from filemerge._filemerge().
         """
-        if self[dfile] in (MERGE_RECORD_RESOLVED, MERGE_RECORD_DRIVER_RESOLVED):
+        if self[dfile] in (
+            MERGE_RECORD_RESOLVED,
+            LEGACY_RECORD_DRIVER_RESOLVED,
+        ):
             return True, 0
         stateentry = self._state[dfile]
         state, localkey, lfile, afile, anode, ofile, onode, flags = stateentry
@@ -548,24 +484,6 @@ 
                 actions[action].append((f, None, b"merge result"))
         return actions
 
-    def queueremove(self, f):
-        """queues a file to be removed from the dirstate
-
-        Meant for use by custom merge drivers."""
-        self._results[f] = 0, ACTION_REMOVE
-
-    def queueadd(self, f):
-        """queues a file to be added to the dirstate
-
-        Meant for use by custom merge drivers."""
-        self._results[f] = 0, ACTION_ADD
-
-    def queueget(self, f):
-        """queues a file to be marked modified in the dirstate
-
-        Meant for use by custom merge drivers."""
-        self._results[f] = 0, ACTION_GET
-
 
 class mergestate(_mergestate_base):
 
@@ -593,7 +511,6 @@ 
         This function process "record" entry produced by the de-serialization
         of on disk file.
         """
-        self._mdstate = MERGE_DRIVER_STATE_SUCCESS
         unsupported = set()
         records = self._readrecords()
         for rtype, record in records:
@@ -601,24 +518,13 @@ 
                 self._local = bin(record)
             elif rtype == RECORD_OTHER:
                 self._other = bin(record)
-            elif rtype == RECORD_MERGE_DRIVER_STATE:
-                bits = record.split(b'\0', 1)
-                mdstate = bits[1]
-                if len(mdstate) != 1 or mdstate not in (
-                    MERGE_DRIVER_STATE_UNMARKED,
-                    MERGE_DRIVER_STATE_MARKED,
-                    MERGE_DRIVER_STATE_SUCCESS,
-                ):
-                    # the merge driver should be idempotent, so just rerun it
-                    mdstate = MERGE_DRIVER_STATE_UNMARKED
-
-                self._readmergedriver = bits[0]
-                self._mdstate = mdstate
+            elif rtype == LEGACY_MERGE_DRIVER_STATE:
+                pass
             elif rtype in (
                 RECORD_MERGED,
                 RECORD_CHANGEDELETE_CONFLICT,
                 RECORD_PATH_CONFLICT,
-                RECORD_MERGE_DRIVER_MERGE,
+                LEGACY_MERGE_DRIVER_MERGE,
                 LEGACY_RECORD_RESOLVED_OTHER,
             ):
                 bits = record.split(b'\0')
@@ -812,17 +718,6 @@ 
     def _restore_backup(self, fctx, localkey, flags):
         fctx.write(self._backups[localkey], flags)
 
-    @util.propertycache
-    def mergedriver(self):
-        configmergedriver = self._repo.ui.config(
-            b'experimental', b'mergedriver'
-        )
-        if configmergedriver:
-            raise error.InMemoryMergeConflictsError(
-                b"in-memory merge does not support mergedriver"
-            )
-        return None
-
     def commit(self):
         pass
 
diff --git a/mercurial/merge.py b/mercurial/merge.py
--- a/mercurial/merge.py
+++ b/mercurial/merge.py
@@ -355,20 +355,6 @@ 
         lastfull = f
 
 
-def driverpreprocess(repo, ms, wctx, labels=None):
-    """run the preprocess step of the merge driver, if any
-
-    This is currently not implemented -- it's an extension point."""
-    return True
-
-
-def driverconclude(repo, ms, wctx, labels=None):
-    """run the conclude step of the merge driver, if any
-
-    This is currently not implemented -- it's an extension point."""
-    return True
-
-
 def _filesindirs(repo, manifest, dirs):
     """
     Generator that yields pairs of all the files in the manifest that are found
@@ -1605,27 +1591,6 @@ 
             mergestatemod.ACTION_DIR_RENAME_MOVE_LOCAL,
         )
     )
-    # the ordering is important here -- ms.mergedriver will raise if the merge
-    # driver has changed, and we want to be able to bypass it when overwrite is
-    # True
-    usemergedriver = not overwrite and mergeactions and ms.mergedriver
-
-    if usemergedriver:
-        ms.commit()
-        proceed = driverpreprocess(repo, ms, wctx, labels=labels)
-        # the driver might leave some files unresolved
-        unresolvedf = set(ms.unresolved())
-        if not proceed:
-            # XXX setting unresolved to at least 1 is a hack to make sure we
-            # error out
-            return updateresult(
-                updated, merged, removed, max(len(unresolvedf), 1)
-            )
-        newactions = []
-        for f, args, msg in mergeactions:
-            if f in unresolvedf:
-                newactions.append((f, args, msg))
-        mergeactions = newactions
 
     try:
         # premerge
@@ -1655,18 +1620,6 @@ 
 
     unresolved = ms.unresolvedcount()
 
-    if (
-        usemergedriver
-        and not unresolved
-        and ms.mdstate() != mergestatemod.MERGE_DRIVER_STATE_SUCCESS
-    ):
-        if not driverconclude(repo, ms, wctx, labels=labels):
-            # XXX setting unresolved to at least 1 is a hack to make sure we
-            # error out
-            unresolved = max(unresolved, 1)
-
-        ms.commit()
-
     msupdated, msmerged, msremoved = ms.counts()
     updated += msupdated
     merged += msmerged
@@ -1674,9 +1627,6 @@ 
 
     extraactions = ms.actions()
     if extraactions:
-        mfiles = {
-            a[0] for a in mresult.getactions((mergestatemod.ACTION_MERGE,))
-        }
         for k, acts in pycompat.iteritems(extraactions):
             for a in acts:
                 mresult.addfile(a[0], k, *a[1:])
@@ -1684,27 +1634,6 @@ 
                 # no filedata until mergestate is updated to provide it
                 for a in acts:
                     getfiledata[a[0]] = None
-            # Remove these files from actions[ACTION_MERGE] as well. This is
-            # important because in recordupdates, files in actions[ACTION_MERGE]
-            # are processed after files in other actions, and the merge driver
-            # might add files to those actions via extraactions above. This can
-            # lead to a file being recorded twice, with poor results. This is
-            # especially problematic for actions[ACTION_REMOVE] (currently only
-            # possible with the merge driver in the initial merge process;
-            # interrupted merges don't go through this flow).
-            #
-            # The real fix here is to have indexes by both file and action so
-            # that when the action for a file is changed it is automatically
-            # reflected in the other action lists. But that involves a more
-            # complex data structure, so this will do for now.
-            #
-            # We don't need to do the same operation for 'dc' and 'cd' because
-            # those lists aren't consulted again.
-            mfiles.difference_update(a[0] for a in acts)
-
-        for a in list(mresult.getactions((mergestatemod.ACTION_MERGE,))):
-            if a[0] not in mfiles:
-                mresult.removefile(a[0])
 
     progress.complete()
     assert len(getfiledata) == (
diff --git a/mercurial/helptext/internals/mergestate.txt b/mercurial/helptext/internals/mergestate.txt
--- a/mercurial/helptext/internals/mergestate.txt
+++ b/mercurial/helptext/internals/mergestate.txt
@@ -37,30 +37,18 @@ 
 | * O: the node of the "other" part of the merge (hexified version)
 | * F: a file to be merged entry
 | * C: a change/delete or delete/change conflict
-| * D: a file that the external merge driver will merge internally
-|      (experimental)
 | * P: a path conflict (file vs directory)
-| * m: the external merge driver defined for this merge plus its run state
-|      (experimental)
 | * f: a (filename, dictionary) tuple of optional values for a given file
 | * X: unsupported mandatory record type (used in tests)
 | * x: unsupported advisory record type (used in tests)
 | * l: the labels for the parts of the merge.
 
-Merge driver run states (experimental):
-
-| * u: driver-resolved files unmarked -- needs to be run next time we're
-|      about to resolve or commit
-| * m: driver-resolved files marked -- only needs to be run before commit
-| * s: success/skipped -- does not need to be run any more
-
 Merge record states (indexed by filename):
 
 | * u: unresolved conflict
 | * r: resolved conflict
 | * pu: unresolved path conflict (file conflicts with directory)
 | * pr: resolved path conflict
-| * d: driver-resolved conflict
 
 The resolve command transitions between 'u' and 'r' for conflicts and
 'pu' and 'pr' for path conflicts.
diff --git a/mercurial/configitems.py b/mercurial/configitems.py
--- a/mercurial/configitems.py
+++ b/mercurial/configitems.py
@@ -635,9 +635,6 @@ 
 coreconfigitem(
     b'experimental', b'httppostargs', default=False,
 )
-coreconfigitem(
-    b'experimental', b'mergedriver', default=None,
-)
 coreconfigitem(b'experimental', b'nointerrupt', default=False)
 coreconfigitem(b'experimental', b'nointerrupt-interactiveonly', default=True)
 
diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -5986,10 +5986,6 @@ 
                 b'resolve.resolved',
                 b'R',
             ),
-            mergestatemod.MERGE_RECORD_DRIVER_RESOLVED: (
-                b'resolve.driverresolved',
-                b'D',
-            ),
         }
 
         for f in ms:
@@ -6014,21 +6010,9 @@ 
             )
 
         wctx = repo[None]
-
-        if (
-            ms.mergedriver
-            and ms.mdstate() == mergestatemod.MERGE_DRIVER_STATE_UNMARKED
-        ):
-            proceed = mergemod.driverpreprocess(repo, ms, wctx)
-            ms.commit()
-            # allow mark and unmark to go through
-            if not mark and not unmark and not proceed:
-                return 1
-
         m = scmutil.match(wctx, pats, opts)
         ret = 0
         didwork = False
-        runconclude = False
 
         tocomplete = []
         hasconflictmarkers = []
@@ -6043,26 +6027,6 @@ 
 
             didwork = True
 
-            # don't let driver-resolved files be marked, and run the conclude
-            # step if asked to resolve
-            if ms[f] == mergestatemod.MERGE_RECORD_DRIVER_RESOLVED:
-                exact = m.exact(f)
-                if mark:
-                    if exact:
-                        ui.warn(
-                            _(b'not marking %s as it is driver-resolved\n')
-                            % uipathfn(f)
-                        )
-                elif unmark:
-                    if exact:
-                        ui.warn(
-                            _(b'not unmarking %s as it is driver-resolved\n')
-                            % uipathfn(f)
-                        )
-                else:
-                    runconclude = True
-                continue
-
             # path conflicts must be resolved manually
             if ms[f] in (
                 mergestatemod.MERGE_RECORD_UNRESOLVED_PATH,
@@ -6184,32 +6148,12 @@ 
             ui.warn(_(b"arguments do not match paths that need resolving\n"))
             if hint:
                 ui.warn(hint)
-        elif ms.mergedriver and ms.mdstate() != b's':
-            # run conclude step when either a driver-resolved file is requested
-            # or there are no driver-resolved files
-            # we can't use 'ret' to determine whether any files are unresolved
-            # because we might not have tried to resolve some
-            if (runconclude or not list(ms.driverresolved())) and not list(
-                ms.unresolved()
-            ):
-                proceed = mergemod.driverconclude(repo, ms, wctx)
-                ms.commit()
-                if not proceed:
-                    return 1
 
     # Nudge users into finishing an unfinished operation
     unresolvedf = list(ms.unresolved())
-    driverresolvedf = list(ms.driverresolved())
-    if not unresolvedf and not driverresolvedf:
+    if not unresolvedf:
         ui.status(_(b'(no more unresolved files)\n'))
         cmdutil.checkafterresolved(repo)
-    elif not unresolvedf:
-        ui.status(
-            _(
-                b'(no more unresolved files -- '
-                b'run "hg resolve --all" to conclude)\n'
-            )
-        )
 
     return ret