Patchwork [2,of,6] extdiff: refactor logic which does diff of patches

login
register
mail settings
Submitter Pulkit Goyal
Date July 14, 2020, 6:39 p.m.
Message ID <2dcd0a72a8f59f9dfd2d.1594751987@workspace>
Download mbox | patch
Permalink /patch/46737/
State Accepted
Headers show

Comments

Pulkit Goyal - July 14, 2020, 6:39 p.m.
# HG changeset patch
# User Pulkit Goyal <7895pulkit@gmail.com>
# Date 1594105959 -19800
#      Tue Jul 07 12:42:39 2020 +0530
# Node ID 2dcd0a72a8f59f9dfd2d2e9112b65b10ed2c87fe
# Parent  be7975344fe67437a5068c6a940c33e5ed234917
# EXP-Topic diff-refactor
extdiff: refactor logic which does diff of patches

The current extdiff code is hard to understand on first look. Took me few hours
to grasp the code. Before adding more things, decided to do some refactoring.

Differential Revision: https://phab.mercurial-scm.org/D8686
Yuya Nishihara - July 15, 2020, 11:36 a.m.
On Wed, 15 Jul 2020 00:09:47 +0530, Pulkit Goyal wrote:
> # HG changeset patch
> # User Pulkit Goyal <7895pulkit@gmail.com>
> # Date 1594105959 -19800
> #      Tue Jul 07 12:42:39 2020 +0530
> # Node ID 2dcd0a72a8f59f9dfd2d2e9112b65b10ed2c87fe
> # Parent  be7975344fe67437a5068c6a940c33e5ed234917
> # EXP-Topic diff-refactor
> extdiff: refactor logic which does diff of patches

> @@ -389,6 +421,7 @@ def dodiff(ui, repo, cmdline, pats, opts
>      subrepos = opts.get(b'subrepos')
>  
>      matcher = scmutil.match(repo[node2], pats, opts)
> +    tmproot = pycompat.mkdtemp(prefix=b'extdiff.')
>  
>      if opts.get(b'patch'):
>          if subrepos:
> @@ -414,82 +447,66 @@ def dodiff(ui, repo, cmdline, pats, opts
>          if not common:
>              return 0
>  
> -    tmproot = pycompat.mkdtemp(prefix=b'extdiff.')

Temporary directory wouldn't be cleaned up on early return/exception.
Pulkit Goyal - July 16, 2020, 9:17 a.m.
On Wed, Jul 15, 2020 at 5:07 PM Yuya Nishihara <yuya@tcha.org> wrote:
>
> On Wed, 15 Jul 2020 00:09:47 +0530, Pulkit Goyal wrote:
> > # HG changeset patch
> > # User Pulkit Goyal <7895pulkit@gmail.com>
> > # Date 1594105959 -19800
> > #      Tue Jul 07 12:42:39 2020 +0530
> > # Node ID 2dcd0a72a8f59f9dfd2d2e9112b65b10ed2c87fe
> > # Parent  be7975344fe67437a5068c6a940c33e5ed234917
> > # EXP-Topic diff-refactor
> > extdiff: refactor logic which does diff of patches
>
> > @@ -389,6 +421,7 @@ def dodiff(ui, repo, cmdline, pats, opts
> >      subrepos = opts.get(b'subrepos')
> >
> >      matcher = scmutil.match(repo[node2], pats, opts)
> > +    tmproot = pycompat.mkdtemp(prefix=b'extdiff.')
> >
> >      if opts.get(b'patch'):
> >          if subrepos:
> > @@ -414,82 +447,66 @@ def dodiff(ui, repo, cmdline, pats, opts
> >          if not common:
> >              return 0
> >
> > -    tmproot = pycompat.mkdtemp(prefix=b'extdiff.')
>
> Temporary directory wouldn't be cleaned up on early return/exception.

Oops, not sure why I even moved this line. Nice eyes. Sending a new version.

Patch

diff --git a/hgext/extdiff.py b/hgext/extdiff.py
--- a/hgext/extdiff.py
+++ b/hgext/extdiff.py
@@ -350,6 +350,38 @@  def _runperfilediff(
                 proc.wait()
 
 
+def diffpatch(ui, repo, node1a, node2, tmproot, matcher, cmdline, do3way):
+    template = b'hg-%h.patch'
+    with formatter.nullformatter(ui, b'extdiff', {}) as fm:
+        cmdutil.export(
+            repo,
+            [repo[node1a].rev(), repo[node2].rev()],
+            fm,
+            fntemplate=repo.vfs.reljoin(tmproot, template),
+            match=matcher,
+        )
+    label1a = cmdutil.makefilename(repo[node1a], template)
+    label2 = cmdutil.makefilename(repo[node2], template)
+    dir1a = repo.vfs.reljoin(tmproot, label1a)
+    dir2 = repo.vfs.reljoin(tmproot, label2)
+    dir1b = None
+    label1b = None
+    cmdline = formatcmdline(
+        cmdline,
+        repo.root,
+        do3way=do3way,
+        parent1=dir1a,
+        plabel1=label1a,
+        parent2=dir1b,
+        plabel2=label1b,
+        child=dir2,
+        clabel=label2,
+    )
+    ui.debug(b'running %r in %s\n' % (pycompat.bytestr(cmdline), tmproot))
+    ui.system(cmdline, cwd=tmproot, blockedtag=b'extdiff')
+    return 1
+
+
 def dodiff(ui, repo, cmdline, pats, opts, guitool=False):
     '''Do the actual diff:
 
@@ -389,6 +421,7 @@  def dodiff(ui, repo, cmdline, pats, opts
     subrepos = opts.get(b'subrepos')
 
     matcher = scmutil.match(repo[node2], pats, opts)
+    tmproot = pycompat.mkdtemp(prefix=b'extdiff.')
 
     if opts.get(b'patch'):
         if subrepos:
@@ -414,82 +447,66 @@  def dodiff(ui, repo, cmdline, pats, opts
         if not common:
             return 0
 
-    tmproot = pycompat.mkdtemp(prefix=b'extdiff.')
     try:
-        if not opts.get(b'patch'):
-            # Always make a copy of node1a (and node1b, if applicable)
-            dir1a_files = mod_a | rem_a | ((mod_b | add_b) - add_a)
-            dir1a = snapshot(ui, repo, dir1a_files, node1a, tmproot, subrepos)[
+        if opts.get(b'patch'):
+            return diffpatch(
+                ui, repo, node1a, node2, tmproot, matcher, cmdline, do3way
+            )
+
+        # Always make a copy of node1a (and node1b, if applicable)
+        dir1a_files = mod_a | rem_a | ((mod_b | add_b) - add_a)
+        dir1a = snapshot(ui, repo, dir1a_files, node1a, tmproot, subrepos)[0]
+        rev1a = b'@%d' % repo[node1a].rev()
+        if do3way:
+            dir1b_files = mod_b | rem_b | ((mod_a | add_a) - add_b)
+            dir1b = snapshot(ui, repo, dir1b_files, node1b, tmproot, subrepos)[
                 0
             ]
-            rev1a = b'@%d' % repo[node1a].rev()
-            if do3way:
-                dir1b_files = mod_b | rem_b | ((mod_a | add_a) - add_b)
-                dir1b = snapshot(
-                    ui, repo, dir1b_files, node1b, tmproot, subrepos
-                )[0]
-                rev1b = b'@%d' % repo[node1b].rev()
-            else:
-                dir1b = None
-                rev1b = b''
-
-            fnsandstat = []
+            rev1b = b'@%d' % repo[node1b].rev()
+        else:
+            dir1b = None
+            rev1b = b''
 
-            # If node2 in not the wc or there is >1 change, copy it
-            dir2root = b''
-            rev2 = b''
-            if node2:
-                dir2 = snapshot(ui, repo, modadd, node2, tmproot, subrepos)[0]
-                rev2 = b'@%d' % repo[node2].rev()
-            elif len(common) > 1:
-                # we only actually need to get the files to copy back to
-                # the working dir in this case (because the other cases
-                # are: diffing 2 revisions or single file -- in which case
-                # the file is already directly passed to the diff tool).
-                dir2, fnsandstat = snapshot(
-                    ui, repo, modadd, None, tmproot, subrepos
-                )
-            else:
-                # This lets the diff tool open the changed file directly
-                dir2 = b''
-                dir2root = repo.root
+        fnsandstat = []
 
-            label1a = rev1a
-            label1b = rev1b
-            label2 = rev2
+        # If node2 in not the wc or there is >1 change, copy it
+        dir2root = b''
+        rev2 = b''
+        if node2:
+            dir2 = snapshot(ui, repo, modadd, node2, tmproot, subrepos)[0]
+            rev2 = b'@%d' % repo[node2].rev()
+        elif len(common) > 1:
+            # we only actually need to get the files to copy back to
+            # the working dir in this case (because the other cases
+            # are: diffing 2 revisions or single file -- in which case
+            # the file is already directly passed to the diff tool).
+            dir2, fnsandstat = snapshot(
+                ui, repo, modadd, None, tmproot, subrepos
+            )
+        else:
+            # This lets the diff tool open the changed file directly
+            dir2 = b''
+            dir2root = repo.root
 
-            # If only one change, diff the files instead of the directories
-            # Handle bogus modifies correctly by checking if the files exist
-            if len(common) == 1:
-                common_file = util.localpath(common.pop())
-                dir1a = os.path.join(tmproot, dir1a, common_file)
-                label1a = common_file + rev1a
-                if not os.path.isfile(dir1a):
-                    dir1a = pycompat.osdevnull
-                if do3way:
-                    dir1b = os.path.join(tmproot, dir1b, common_file)
-                    label1b = common_file + rev1b
-                    if not os.path.isfile(dir1b):
-                        dir1b = pycompat.osdevnull
-                dir2 = os.path.join(dir2root, dir2, common_file)
-                label2 = common_file + rev2
-        else:
-            template = b'hg-%h.patch'
-            with formatter.nullformatter(ui, b'extdiff', {}) as fm:
-                cmdutil.export(
-                    repo,
-                    [repo[node1a].rev(), repo[node2].rev()],
-                    fm,
-                    fntemplate=repo.vfs.reljoin(tmproot, template),
-                    match=matcher,
-                )
-            label1a = cmdutil.makefilename(repo[node1a], template)
-            label2 = cmdutil.makefilename(repo[node2], template)
-            dir1a = repo.vfs.reljoin(tmproot, label1a)
-            dir2 = repo.vfs.reljoin(tmproot, label2)
-            dir1b = None
-            label1b = None
-            fnsandstat = []
+        label1a = rev1a
+        label1b = rev1b
+        label2 = rev2
+
+        # If only one change, diff the files instead of the directories
+        # Handle bogus modifies correctly by checking if the files exist
+        if len(common) == 1:
+            common_file = util.localpath(common.pop())
+            dir1a = os.path.join(tmproot, dir1a, common_file)
+            label1a = common_file + rev1a
+            if not os.path.isfile(dir1a):
+                dir1a = pycompat.osdevnull
+            if do3way:
+                dir1b = os.path.join(tmproot, dir1b, common_file)
+                label1b = common_file + rev1b
+                if not os.path.isfile(dir1b):
+                    dir1b = pycompat.osdevnull
+            dir2 = os.path.join(dir2root, dir2, common_file)
+            label2 = common_file + rev2
 
         if not perfile:
             # Run the external tool on the 2 temp directories or the patches