Patchwork extdiff: correctly handle deleted subrepositories (issue3153)

login
register
mail settings
Submitter Andrew Zwicky
Date Dec. 2, 2015, 5:15 a.m.
Message ID <c09ee2f731d9f92fb759.1449033304@localhost.localdomain>
Download mbox | patch
Permalink /patch/11746/
State Accepted
Headers show

Comments

Andrew Zwicky - Dec. 2, 2015, 5:15 a.m.
# HG changeset patch
# User Andrew Zwicky <andrew.zwicky@gmail.com>
# Date 1447800172 21600
#      Tue Nov 17 16:42:52 2015 -0600
# Node ID c09ee2f731d9f92fb75900b3a7becd9a5ee45124
# Parent  61fbf5dc12b23e7a2a30cf04ebd9f096c42a1f61
extdiff: correctly handle deleted subrepositories (issue3153)

Previously, when extdiff was called on two changesets where
a subrepository had been removed, an unexpected KeyError would
be raised.

Now, the missing subrepository will be ignored.  This behavior
mirrors the behavior in diffordiffstat from cmdutil.py line
~1138-1153.  The KeyError is caught and the revision is
set to None.

try/catch of LookupError around matchmod.narrowmatcher and
sub.status is removed, as LookupError is not raised anywhere
within those methods or deeper calls.
Augie Fackler - Dec. 2, 2015, 4:12 p.m.
On Tue, Dec 01, 2015 at 11:15:04PM -0600, Andrew Zwicky wrote:
> # HG changeset patch
> # User Andrew Zwicky <andrew.zwicky@gmail.com>
> # Date 1447800172 21600
> #      Tue Nov 17 16:42:52 2015 -0600
> # Node ID c09ee2f731d9f92fb75900b3a7becd9a5ee45124
> # Parent  61fbf5dc12b23e7a2a30cf04ebd9f096c42a1f61
> extdiff: correctly handle deleted subrepositories (issue3153)

Queued, thanks. Congratulations on your first hg patch!


>
> Previously, when extdiff was called on two changesets where
> a subrepository had been removed, an unexpected KeyError would
> be raised.
>
> Now, the missing subrepository will be ignored.  This behavior
> mirrors the behavior in diffordiffstat from cmdutil.py line
> ~1138-1153.  The KeyError is caught and the revision is
> set to None.
>
> try/catch of LookupError around matchmod.narrowmatcher and
> sub.status is removed, as LookupError is not raised anywhere
> within those methods or deeper calls.
>
> diff -r 61fbf5dc12b2 -r c09ee2f731d9 mercurial/context.py
> --- a/mercurial/context.py	Tue Nov 24 21:41:12 2015 +0000
> +++ b/mercurial/context.py	Tue Nov 17 16:42:52 2015 -0600
> @@ -336,17 +336,19 @@
>
>          if listsubrepos:
>              for subpath, sub in scmutil.itersubrepos(ctx1, ctx2):
> -                rev2 = ctx2.subrev(subpath)
>                  try:
> -                    submatch = matchmod.narrowmatcher(subpath, match)
> -                    s = sub.status(rev2, match=submatch, ignored=listignored,
> -                                   clean=listclean, unknown=listunknown,
> -                                   listsubrepos=True)
> -                    for rfiles, sfiles in zip(r, s):
> -                        rfiles.extend("%s/%s" % (subpath, f) for f in sfiles)
> -                except error.LookupError:
> -                    self._repo.ui.status(_("skipping missing "
> -                                           "subrepository: %s\n") % subpath)
> +                    rev2 = ctx2.subrev(subpath)
> +                except KeyError:
> +                    # A subrepo that existed in node1 was deleted between
> +                    # node1 and node2 (inclusive). Thus, ctx2's substate
> +                    # won't contain that subpath. The best we can do ignore it.
> +                    rev2 = None
> +                submatch = matchmod.narrowmatcher(subpath, match)
> +                s = sub.status(rev2, match=submatch, ignored=listignored,
> +                               clean=listclean, unknown=listunknown,
> +                               listsubrepos=True)
> +                for rfiles, sfiles in zip(r, s):
> +                    rfiles.extend("%s/%s" % (subpath, f) for f in sfiles)
>
>          for l in r:
>              l.sort()
> diff -r 61fbf5dc12b2 -r c09ee2f731d9 tests/test-extdiff.t
> --- a/tests/test-extdiff.t	Tue Nov 24 21:41:12 2015 +0000
> +++ b/tests/test-extdiff.t	Tue Nov 17 16:42:52 2015 -0600
> @@ -126,6 +126,25 @@
>    diff-like tools yield a non-zero exit code
>  #endif
>
> +issue3153: ensure using extdiff with removed subrepos doesn't crash:
> +
> +  $ hg init suba
> +  $ cd suba
> +  $ echo suba > suba
> +  $ hg add
> +  adding suba
> +  $ hg ci -m "adding suba file"
> +  $ cd ..
> +  $ echo suba=suba > .hgsub
> +  $ hg add
> +  adding .hgsub
> +  $ hg ci -Sm "adding subrepo"
> +  $ echo > .hgsub
> +  $ hg ci -m "removing subrepo"
> +  $ hg falabala -r 4 -r 5 -S
> +  diffing a.398e36faf9c6 a.5ab95fb166c4
> +  [1]
> +
>  issue4463: usage of command line configuration without additional quoting
>
>    $ cat <<EOF >> $HGRCPATH
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel

Patch

diff -r 61fbf5dc12b2 -r c09ee2f731d9 mercurial/context.py
--- a/mercurial/context.py	Tue Nov 24 21:41:12 2015 +0000
+++ b/mercurial/context.py	Tue Nov 17 16:42:52 2015 -0600
@@ -336,17 +336,19 @@ 
 
         if listsubrepos:
             for subpath, sub in scmutil.itersubrepos(ctx1, ctx2):
-                rev2 = ctx2.subrev(subpath)
                 try:
-                    submatch = matchmod.narrowmatcher(subpath, match)
-                    s = sub.status(rev2, match=submatch, ignored=listignored,
-                                   clean=listclean, unknown=listunknown,
-                                   listsubrepos=True)
-                    for rfiles, sfiles in zip(r, s):
-                        rfiles.extend("%s/%s" % (subpath, f) for f in sfiles)
-                except error.LookupError:
-                    self._repo.ui.status(_("skipping missing "
-                                           "subrepository: %s\n") % subpath)
+                    rev2 = ctx2.subrev(subpath)
+                except KeyError:
+                    # A subrepo that existed in node1 was deleted between
+                    # node1 and node2 (inclusive). Thus, ctx2's substate
+                    # won't contain that subpath. The best we can do ignore it.
+                    rev2 = None
+                submatch = matchmod.narrowmatcher(subpath, match)
+                s = sub.status(rev2, match=submatch, ignored=listignored,
+                               clean=listclean, unknown=listunknown,
+                               listsubrepos=True)
+                for rfiles, sfiles in zip(r, s):
+                    rfiles.extend("%s/%s" % (subpath, f) for f in sfiles)
 
         for l in r:
             l.sort()
diff -r 61fbf5dc12b2 -r c09ee2f731d9 tests/test-extdiff.t
--- a/tests/test-extdiff.t	Tue Nov 24 21:41:12 2015 +0000
+++ b/tests/test-extdiff.t	Tue Nov 17 16:42:52 2015 -0600
@@ -126,6 +126,25 @@ 
   diff-like tools yield a non-zero exit code
 #endif
 
+issue3153: ensure using extdiff with removed subrepos doesn't crash:
+
+  $ hg init suba
+  $ cd suba
+  $ echo suba > suba
+  $ hg add
+  adding suba
+  $ hg ci -m "adding suba file"
+  $ cd ..
+  $ echo suba=suba > .hgsub
+  $ hg add
+  adding .hgsub
+  $ hg ci -Sm "adding subrepo"
+  $ echo > .hgsub
+  $ hg ci -m "removing subrepo"
+  $ hg falabala -r 4 -r 5 -S
+  diffing a.398e36faf9c6 a.5ab95fb166c4
+  [1]
+
 issue4463: usage of command line configuration without additional quoting
 
   $ cat <<EOF >> $HGRCPATH