storage: Use variable-length argument list for Directory.descend()

This provides an API in line with, e.g., os.path.join(), and eliminates
isinstance checks.

10 files changed, 70 insertions, 84 deletions

diff --git a/buildstream/_artifactcache.py b/buildstream/_artifactcache.py
index 9986d689f..330365025 100644
--- a/buildstream/_artifactcache.py
+++ b/buildstream/_artifactcache.py
@@ -692,9 +692,8 @@ class ArtifactCache():
     #     logsdir (CasBasedDirectory): A CasBasedDirectory containing the artifact's logs
     #
     def get_artifact_logs(self, ref):
-        descend = ["logs"]
         cache_id = self.cas.resolve_ref(ref, update_mtime=True)
-        vdir = CasBasedDirectory(self.cas, digest=cache_id).descend(descend)
+        vdir = CasBasedDirectory(self.cas, digest=cache_id).descend('logs')
         return vdir
 
     ################################################
diff --git a/buildstream/element.py b/buildstream/element.py
index 9d1333721..7331a6230 100644
--- a/buildstream/element.py
+++ b/buildstream/element.py
@@ -663,14 +663,14 @@ class Element(Plugin):
 
         with self.timed_activity("Staging {}/{}".format(self.name, self._get_brief_display_key())):
             artifact_vdir, _ = self.__get_artifact_directory()
-            files_vdir = artifact_vdir.descend(['files'])
+            files_vdir = artifact_vdir.descend('files')
 
             # Hard link it into the staging area
             #
             vbasedir = sandbox.get_virtual_directory()
             vstagedir = vbasedir \
                 if path is None \
-                else vbasedir.descend(path.lstrip(os.sep).split(os.sep))
+                else vbasedir.descend(*path.lstrip(os.sep).split(os.sep))
 
             split_filter = self.__split_filter_func(include, exclude, orphans)
 
@@ -1439,7 +1439,7 @@ class Element(Plugin):
 
         # Stage all sources that need to be copied
         sandbox_vroot = sandbox.get_virtual_directory()
-        host_vdirectory = sandbox_vroot.descend(directory.lstrip(os.sep).split(os.sep), create=True)
+        host_vdirectory = sandbox_vroot.descend(*directory.lstrip(os.sep).split(os.sep), create=True)
         self._stage_sources_at(host_vdirectory, mount_workspaces=mount_workspaces, usebuildtree=sandbox._usebuildtree)
 
     # _stage_sources_at():
@@ -1478,7 +1478,7 @@ class Element(Plugin):
             # Check if we have a cached buildtree to use
             elif usebuildtree:
                 artifact_vdir, _ = self.__get_artifact_directory()
-                import_dir = artifact_vdir.descend(['buildtree'])
+                import_dir = artifact_vdir.descend('buildtree')
                 if import_dir.is_empty():
                     detail = "Element type either does not expect a buildtree or it was explictily cached without one."
                     self.warn("WARNING: {} Artifact contains an empty buildtree".format(self.name), detail=detail)
@@ -1663,7 +1663,7 @@ class Element(Plugin):
                     if workspace and self.__staged_sources_directory:
                         sandbox_vroot = sandbox.get_virtual_directory()
                         path_components = self.__staged_sources_directory.lstrip(os.sep).split(os.sep)
-                        sandbox_vpath = sandbox_vroot.descend(path_components)
+                        sandbox_vpath = sandbox_vroot.descend(*path_components)
                         try:
                             sandbox_vpath.import_files(workspace.get_absolute_path())
                         except UtilError as e2:
@@ -1684,7 +1684,7 @@ class Element(Plugin):
             if collect is not None:
                 try:
                     sandbox_vroot = sandbox.get_virtual_directory()
-                    collectvdir = sandbox_vroot.descend(collect.lstrip(os.sep).split(os.sep))
+                    collectvdir = sandbox_vroot.descend(*collect.lstrip(os.sep).split(os.sep))
                 except VirtualDirectoryError:
                     # No collect directory existed
                     collectvdir = None
@@ -1721,7 +1721,7 @@ class Element(Plugin):
                 sandbox_vroot = sandbox.get_virtual_directory()
                 try:
                     sandbox_build_dir = sandbox_vroot.descend(
-                        self.get_variable('build-root').lstrip(os.sep).split(os.sep))
+                        *self.get_variable('build-root').lstrip(os.sep).split(os.sep))
                     buildtreevdir.import_files(sandbox_build_dir)
                 except VirtualDirectoryError:
                     # Directory could not be found. Pre-virtual
@@ -2645,7 +2645,7 @@ class Element(Plugin):
         filter_func = self.__split_filter_func(include=include, exclude=exclude, orphans=orphans)
 
         artifact_vdir, _ = self.__get_artifact_directory()
-        files_vdir = artifact_vdir.descend(['files'])
+        files_vdir = artifact_vdir.descend('files')
 
         element_files = files_vdir.list_relative_paths()
 
diff --git a/buildstream/plugins/elements/compose.py b/buildstream/plugins/elements/compose.py
index 12520ce4c..f45ffd76a 100644
--- a/buildstream/plugins/elements/compose.py
+++ b/buildstream/plugins/elements/compose.py
@@ -156,7 +156,7 @@ class ComposeElement(Element):
         # instead of into a subdir. The element assemble() method should
         # support this in some way.
         #
-        installdir = vbasedir.descend(['buildstream', 'install'], create=True)
+        installdir = vbasedir.descend('buildstream', 'install', create=True)
 
         # We already saved the manifest for created files in the integration phase,
         # now collect the rest of the manifest.
diff --git a/buildstream/plugins/elements/import.py b/buildstream/plugins/elements/import.py
index 3ae979d7a..4d925dec7 100644
--- a/buildstream/plugins/elements/import.py
+++ b/buildstream/plugins/elements/import.py
@@ -75,14 +75,14 @@ class ImportElement(Element):
         self._stage_sources_in_sandbox(sandbox, 'input', mount_workspaces=False)
 
         rootdir = sandbox.get_virtual_directory()
-        inputdir = rootdir.descend(['input'])
-        outputdir = rootdir.descend(['output'], create=True)
+        inputdir = rootdir.descend('input')
+        outputdir = rootdir.descend('output', create=True)
 
         # The directory to grab
-        inputdir = inputdir.descend(self.source.strip(os.sep).split(os.sep))
+        inputdir = inputdir.descend(*self.source.strip(os.sep).split(os.sep))
 
         # The output target directory
-        outputdir = outputdir.descend(self.target.strip(os.sep).split(os.sep), create=True)
+        outputdir = outputdir.descend(*self.target.strip(os.sep).split(os.sep), create=True)
 
         if inputdir.is_empty():
             raise ElementError("{}: No files were found inside directory '{}'"
diff --git a/buildstream/plugins/elements/stack.py b/buildstream/plugins/elements/stack.py
index 138afedf7..b26281fe4 100644
--- a/buildstream/plugins/elements/stack.py
+++ b/buildstream/plugins/elements/stack.py
@@ -63,7 +63,7 @@ class StackElement(Element):
         # the actual artifact data in a subdirectory, then we
         # will be able to store some additional state in the
         # artifact cache, and we can also remove this hack.
-        vrootdir.descend(['output', 'bst'], create=True)
+        vrootdir.descend('output', 'bst', create=True)
 
         # And we're done
         return '/output'
diff --git a/buildstream/sandbox/_sandboxremote.py b/buildstream/sandbox/_sandboxremote.py
index 9ca4738be..348ebca1b 100644
--- a/buildstream/sandbox/_sandboxremote.py
+++ b/buildstream/sandbox/_sandboxremote.py
@@ -317,7 +317,7 @@ class SandboxRemote(Sandbox):
         for mark in self._get_marked_directories():
             directory = mark['directory']
             # Create each marked directory
-            upload_vdir.descend(directory.split(os.path.sep), create=True)
+            upload_vdir.descend(*directory.split(os.path.sep), create=True)
 
         # Generate action_digest first
         input_root_digest = upload_vdir._get_digest()
diff --git a/buildstream/scriptelement.py b/buildstream/scriptelement.py
index 697cd2822..3327f818e 100644
--- a/buildstream/scriptelement.py
+++ b/buildstream/scriptelement.py
@@ -249,7 +249,7 @@ class ScriptElement(Element):
                                              .format(element.name, item['destination']),
                                              silent_nested=True):
                         virtual_dstdir = sandbox.get_virtual_directory()
-                        virtual_dstdir.descend(item['destination'].lstrip(os.sep).split(os.sep), create=True)
+                        virtual_dstdir.descend(*item['destination'].lstrip(os.sep).split(os.sep), create=True)
                         element.stage_dependency_artifacts(sandbox, Scope.RUN, path=item['destination'])
 
             with sandbox.batch(SandboxFlags.NONE):
@@ -269,7 +269,7 @@ class ScriptElement(Element):
                                 dep.integrate(sandbox)
 
         install_root_path_components = self.__install_root.lstrip(os.sep).split(os.sep)
-        sandbox.get_virtual_directory().descend(install_root_path_components, create=True)
+        sandbox.get_virtual_directory().descend(*install_root_path_components, create=True)
 
     def assemble(self, sandbox):
 
diff --git a/buildstream/storage/_casbaseddirectory.py b/buildstream/storage/_casbaseddirectory.py
index 6f4dbae1e..d88b58a1c 100644
--- a/buildstream/storage/_casbaseddirectory.py
+++ b/buildstream/storage/_casbaseddirectory.py
@@ -181,13 +181,12 @@ class CasBasedDirectory(Directory):
 
         self.__invalidate_digest()
 
-    def descend(self, subdirectory_spec, create=False):
+    def descend(self, *paths, create=False):
         """Descend one or more levels of directory hierarchy and return a new
         Directory object for that directory.
 
         Arguments:
-        * subdirectory_spec (list of strings): A list of strings which are all directory
-          names.
+        * *paths (str): A list of strings which are all directory names.
         * create (boolean): If this is true, the directories will be created if
           they don't already exist.
 
@@ -199,38 +198,30 @@ class CasBasedDirectory(Directory):
 
         """
 
-        # It's very common to send a directory name instead of a list and this causes
-        # bizarre errors, so check for it here
-        if not isinstance(subdirectory_spec, list):
-            subdirectory_spec = [subdirectory_spec]
+        current_dir = self
 
-        # Because of the way split works, it's common to get a list which begins with
-        # an empty string. Detect these and remove them.
-        while subdirectory_spec and subdirectory_spec[0] == "":
-            subdirectory_spec.pop(0)
-
-        # Descending into [] returns the same directory.
-        if not subdirectory_spec:
-            return self
+        for path in paths:
+            # Skip empty path segments
+            if not path:
+                continue
 
-        if subdirectory_spec[0] in self.index:
-            entry = self.index[subdirectory_spec[0]]
-            if entry.type == _FileType.DIRECTORY:
-                subdir = entry.get_directory(self)
-                return subdir.descend(subdirectory_spec[1:], create)
-            else:
-                error = "Cannot descend into {}, which is a '{}' in the directory {}"
-                raise VirtualDirectoryError(error.format(subdirectory_spec[0],
-                                                         self.index[subdirectory_spec[0]].type,
-                                                         self))
-        else:
-            if create:
-                newdir = self._add_directory(subdirectory_spec[0])
-                return newdir.descend(subdirectory_spec[1:], create)
+            entry = current_dir.index.get(path)
+            if entry:
+                if entry.type == _FileType.DIRECTORY:
+                    current_dir = entry.get_directory(current_dir)
+                else:
+                    error = "Cannot descend into {}, which is a '{}' in the directory {}"
+                    raise VirtualDirectoryError(error.format(path,
+                                                             current_dir.index[path].type,
+                                                             current_dir))
             else:
-                error = "'{}' not found in {}"
-                raise VirtualDirectoryError(error.format(subdirectory_spec[0], str(self)))
-        return None
+                if create:
+                    current_dir = current_dir._add_directory(path)
+                else:
+                    error = "'{}' not found in {}"
+                    raise VirtualDirectoryError(error.format(path, str(current_dir)))
+
+        return current_dir
 
     def _check_replacement(self, name, path_prefix, fileListResult):
         """ Checks whether 'name' exists, and if so, whether we can overwrite it.
@@ -587,13 +578,13 @@ class CasBasedDirectory(Directory):
         return self.__digest
 
     def _objpath(self, path):
-        subdir = self.descend(path[:-1])
+        subdir = self.descend(*path[:-1])
         entry = subdir.index[path[-1]]
         return self.cas_cache.objpath(entry.digest)
 
     def _exists(self, path):
         try:
-            subdir = self.descend(path[:-1])
+            subdir = self.descend(*path[:-1])
             return path[-1] in subdir.index
         except VirtualDirectoryError:
             return False
diff --git a/buildstream/storage/_filebaseddirectory.py b/buildstream/storage/_filebaseddirectory.py
index 4b0fd917b..742200379 100644
--- a/buildstream/storage/_filebaseddirectory.py
+++ b/buildstream/storage/_filebaseddirectory.py
@@ -46,35 +46,32 @@ class FileBasedDirectory(Directory):
     def __init__(self, external_directory=None):
         self.external_directory = external_directory
 
-    def descend(self, subdirectory_spec, create=False):
+    def descend(self, *paths, create=False):
         """ See superclass Directory for arguments """
-        # It's very common to send a directory name instead of a list and this causes
-        # bizarre errors, so check for it here
-        if not isinstance(subdirectory_spec, list):
-            subdirectory_spec = [subdirectory_spec]
-
-        # Because of the way split works, it's common to get a list which begins with
-        # an empty string. Detect these and remove them.
-        while subdirectory_spec and subdirectory_spec[0] == "":
-            subdirectory_spec.pop(0)
-
-        if not subdirectory_spec:
-            return self
-
-        new_path = os.path.join(self.external_directory, subdirectory_spec[0])
-        try:
-            st = os.lstat(new_path)
-            if not stat.S_ISDIR(st.st_mode):
-                raise VirtualDirectoryError("Cannot descend into '{}': '{}' is not a directory"
-                                            .format(subdirectory_spec[0], new_path))
-        except FileNotFoundError:
-            if create:
-                os.mkdir(new_path)
-            else:
-                raise VirtualDirectoryError("Cannot descend into '{}': '{}' does not exist"
-                                            .format(subdirectory_spec[0], new_path))
 
-        return FileBasedDirectory(new_path).descend(subdirectory_spec[1:], create)
+        current_dir = self
+
+        for path in paths:
+            # Skip empty path segments
+            if not path:
+                continue
+
+            new_path = os.path.join(current_dir.external_directory, path)
+            try:
+                st = os.lstat(new_path)
+                if not stat.S_ISDIR(st.st_mode):
+                    raise VirtualDirectoryError("Cannot descend into '{}': '{}' is not a directory"
+                                                .format(path, new_path))
+            except FileNotFoundError:
+                if create:
+                    os.mkdir(new_path)
+                else:
+                    raise VirtualDirectoryError("Cannot descend into '{}': '{}' does not exist"
+                                                .format(path, new_path))
+
+            current_dir = FileBasedDirectory(new_path)
+
+        return current_dir
 
     def import_files(self, external_pathspec, *,
                      filter_callback=None,
@@ -160,7 +157,7 @@ class FileBasedDirectory(Directory):
                     tf.addfile(tarinfo, f)
             elif tarinfo.isdir():
                 tf.addfile(tarinfo)
-                self.descend(filename.split(os.path.sep)).export_to_tar(tf, arcname, mtime)
+                self.descend(*filename.split(os.path.sep)).export_to_tar(tf, arcname, mtime)
             else:
                 tf.addfile(tarinfo)
 
diff --git a/buildstream/storage/directory.py b/buildstream/storage/directory.py
index 70054f78c..bad818fef 100644
--- a/buildstream/storage/directory.py
+++ b/buildstream/storage/directory.py
@@ -52,13 +52,12 @@ class Directory():
     def __init__(self, external_directory=None):
         raise NotImplementedError()
 
-    def descend(self, subdirectory_spec, create=False):
+    def descend(self, *paths, create=False):
         """Descend one or more levels of directory hierarchy and return a new
         Directory object for that directory.
 
         Args:
-          subdirectory_spec (list of str): A list of strings which are all directory
-            names.
+          *paths (str): A list of strings which are all directory names.
           create (boolean): If this is true, the directories will be created if
             they don't already exist.