From patchwork Mon May 19 15:58:02 2014
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Subject: [3,of,7] revert: add some inline comments
From: Pierre-Yves David <pierre-yves.david@ens-lyon.org>
X-Patchwork-Id: 4797
Message-Id: <2343dafe2e301dce5549.1400515082@marginatus.alto.octopoid.net>
To: mercurial-devel@selenic.com
Cc: pierre-yves.david@ens-lyon.org
Date: Mon, 19 May 2014 08:58:02 -0700

# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@fb.com>
# Date 1400023782 25200
#      Tue May 13 16:29:42 2014 -0700
# Node ID 2343dafe2e301dce55499c4c64a909f415f3bc17
# Parent  4e99a4c765f591a01ed1ef190823344d2025e144
revert: add some inline comments

I spend some time understanding how this part of the revset code is working. I'm
adding some comments to help the code readability. I expect most of them to
disappear in a coming refactoring. But the refactoring should be easier to
follow with the comment.

diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
--- a/mercurial/cmdutil.py
+++ b/mercurial/cmdutil.py
@@ -2247,22 +2247,26 @@ def revert(ui, repo, ctx, parents, *pats
 
     # need all matching names in dirstate and manifest of target rev,
     # so have to walk both. do not print errors if files exist in one
     # but not other.
 
+    # `names` is a mapping for all elements in working copy and target revision
+    # The mapping is in the form:
+    #   <asb path in repo> -> (<path from CWD>, <exactly specified by matcher?>)
     names = {}
 
     wlock = repo.wlock()
     try:
-        # walk dirstate.
+        ## filling of the `names` mapping
+        # walk dirstate to fill `names`
 
         m = scmutil.match(repo[None], pats, opts)
         m.bad = lambda x, y: False
         for abs in repo.walk(m):
             names[abs] = m.rel(abs), m.exact(abs)
 
-        # walk target manifest.
+        # walk target manifest to fill `names`
 
         def badfn(path, msg):
             if path in names:
                 return
             if path in ctx.substate:
@@ -2279,27 +2283,33 @@ def revert(ui, repo, ctx, parents, *pats
             if abs not in names:
                 names[abs] = m.rel(abs), m.exact(abs)
 
         # get the list of subrepos that must be reverted
         targetsubs = sorted(s for s in ctx.substate if m(s))
+
+        # Find status of all file in `names`. (Against working directory parent)
         m = scmutil.matchfiles(repo, names)
         changes = repo.status(match=m)[:4]
         modified, added, removed, deleted = map(set, changes)
 
-        # if f is a rename, also revert the source
+        # if f is a rename, update `names` to also revert the source
         cwd = repo.getcwd()
         for f in added:
             src = repo.dirstate.copied(f)
             if src and src not in names and repo.dirstate[src] == 'r':
                 removed.add(src)
                 names[src] = (repo.pathto(src, cwd), True)
 
+        ## computation of the action to performs on `names` content.
+
         def removeforget(abs):
             if repo.dirstate[abs] == 'a':
                 return _('forgetting %s\n')
             return _('removing %s\n')
 
+        # action to be actually performed by revert
+        # (<list of file>, message>) tuple
         revert = ([], _('reverting %s\n'))
         add = ([], _('adding %s\n'))
         remove = ([], removeforget)
         undelete = ([], _('undeleting %s\n'))
 
@@ -2315,11 +2325,13 @@ def revert(ui, repo, ctx, parents, *pats
             (removed,  undelete, None,   True,  False),
             (deleted,  revert,   remove, False, False),
             )
 
         for abs, (rel, exact) in sorted(names.items()):
+            # hash on file in target manifest (or None if missing from target)
             mfentry = mf.get(abs)
+            # target file to be touch on disk (relative to cwd)
             target = repo.wjoin(abs)
             def handle(xlist, dobackup):
                 xlist[0].append(abs)
                 if (dobackup and not opts.get('no_backup') and
                     os.path.lexists(target) and
@@ -2332,31 +2344,39 @@ def revert(ui, repo, ctx, parents, *pats
                 if ui.verbose or not exact:
                     msg = xlist[1]
                     if not isinstance(msg, basestring):
                         msg = msg(abs)
                     ui.status(msg % rel)
+            # search the entry in the dispatch table.
+            # if the file is in any of this sets, it was touched in the working
+            # directory parent and we are sure it needs to be reverted.
             for table, hitlist, misslist, backuphit, backupmiss in disptable:
                 if abs not in table:
                     continue
                 # file has changed in dirstate
                 if mfentry:
                     handle(hitlist, backuphit)
                 elif misslist is not None:
                     handle(misslist, backupmiss)
                 break
             else:
+                # Not touched in current dirstate.
+
+                # file is unknown in parent, restore older version or ignore.
                 if abs not in repo.dirstate:
                     if mfentry:
                         handle(add, True)
                     elif exact:
                         ui.warn(_('file not managed: %s\n') % rel)
                     continue
-                # file has not changed in dirstate
+
+                # parent is target, no changes mean no changes
                 if node == parent:
                     if exact:
                         ui.warn(_('no changes needed to %s\n') % rel)
                     continue
+                # no change in dirstate but parent and target may differ
                 if pmf is None:
                     # only need parent manifest in this unlikely case,
                     # so do not read by default
                     pmf = repo[parent].manifest()
                 if abs in pmf and mfentry: