Patchwork [remotenames-ext] namespace: never return None for nodes query

login
register
mail settings
Submitter Durham Goode
Date June 6, 2017, 6:33 p.m.
Message ID <2e45bcb8483880d79f37.1496773998@dev111.prn1.facebook.com>
Download mbox | patch
Permalink /patch/21224/
State Accepted
Headers show

Comments

Durham Goode - June 6, 2017, 6:33 p.m.
# HG changeset patch
# User Durham Goode <durham@fb.com>
# Date 1496773892 25200
#      Tue Jun 06 11:31:32 2017 -0700
# Node ID 2e45bcb8483880d79f370ab0e703e72d900d450c
# Parent  55a7fc62fc84a0d9e6996c69f35493d0fcd6bd41
namespace: never return None for nodes query

Previously we would return None if the given name didn't have any nodes
associated with it. This causes problems in a number of places that assume the
return value is always a list. For instance, core calls sorted on the result
without checking if it's None, and the remotenames code itself tries to access
the nodes via [0] without checking if there were any actual nodes.

In remotenames, this can occur if a name exists that has an non-existent node.
If remotenames.resolvenodes is False (for performance reasons) then we will see
the name, but the node list will resolve to no nodes. This patch avoids
returning None and makes the 0 index access check if there are any nodes first.
Sean Farley - June 7, 2017, 11:41 p.m.
Durham Goode <durham@fb.com> writes:

> # HG changeset patch
> # User Durham Goode <durham@fb.com>
> # Date 1496773892 25200
> #      Tue Jun 06 11:31:32 2017 -0700
> # Node ID 2e45bcb8483880d79f370ab0e703e72d900d450c
> # Parent  55a7fc62fc84a0d9e6996c69f35493d0fcd6bd41
> namespace: never return None for nodes query
>
> Previously we would return None if the given name didn't have any nodes
> associated with it. This causes problems in a number of places that assume the
> return value is always a list. For instance, core calls sorted on the result
> without checking if it's None, and the remotenames code itself tries to access
> the nodes via [0] without checking if there were any actual nodes.
>
> In remotenames, this can occur if a name exists that has an non-existent node.
> If remotenames.resolvenodes is False (for performance reasons) then we will see
> the name, but the node list will resolve to no nodes. This patch avoids
> returning None and makes the 0 index access check if there are any nodes first.

Ah, makes sense; thanks for the patch! Queued

Patch

diff --git a/remotenames.py b/remotenames.py
--- a/remotenames.py
+++ b/remotenames.py
@@ -506,7 +506,7 @@  def reposetup(ui, repo):
             colorname='remotebookmark',
             listnames=lambda repo: repo._remotenames.mark2nodes().keys(),
             namemap=lambda repo, name:
-                repo._remotenames.mark2nodes().get(name, None),
+                repo._remotenames.mark2nodes().get(name, []),
             nodemap=lambda repo, node:
                 repo._remotenames.node2marks().get(node, []))
         repo.names.addnamespace(remotebookmarkns)
@@ -522,7 +522,7 @@  def reposetup(ui, repo):
                 listnames = lambda repo:
                     repo._remotenames.hoist2nodes(hoist).keys(),
                 namemap = lambda repo, name:
-                    repo._remotenames.hoist2nodes(hoist).get(name, None),
+                    repo._remotenames.hoist2nodes(hoist).get(name, []),
                 nodemap = lambda repo, node:
                     repo._remotenames.node2hoists(hoist).get(node, []))
             repo.names.addnamespace(hoistednamens)
@@ -535,7 +535,7 @@  def reposetup(ui, repo):
             colorname='remotebranch',
             listnames = lambda repo: repo._remotenames.branch2nodes().keys(),
             namemap = lambda repo, name:
-                repo._remotenames.branch2nodes().get(name, None),
+                repo._remotenames.branch2nodes().get(name, []),
             nodemap = lambda repo, node:
                 repo._remotenames.node2branch().get(node, []))
         repo.names.addnamespace(remotebranchns)
@@ -1255,7 +1255,11 @@  def displayremotebookmarks(ui, repo, opt
     useformatted = repo.ui.formatted()
 
     for name in sorted(ns.listnames(repo)):
-        node = ns.nodes(repo, name)[0]
+        nodes = ns.nodes(repo, name)
+        if not nodes:
+            continue
+
+        node = nodes[0]
         ctx = repo[node]
         fm.startitem()
 
diff --git a/tests/test-remotenames.t b/tests/test-remotenames.t
--- a/tests/test-remotenames.t
+++ b/tests/test-remotenames.t
@@ -159,6 +159,12 @@  make sure we can list remote bookmarks w
   alpha/stable                   2:95cb4ab9fe1d
   alpha/default                  1:7c3bad9141dc
 
+Verify missing node doesnt break remotenames
+
+  $ echo "18f8e0f8ba54270bf158734c781327581cf43634 bookmarks beta/foo" >> .hg/remotenames
+  $ hg book --remote --config remotenames.resolvenodes=False
+     beta/babar                3:78f83396d79e
+
 make sure bogus revisions in .hg/remotenames do not break hg
   $ echo deadbeefdeadbeefdeadbeefdeadbeefdeadbeef default/default >> \
   > .hg/remotenames