fix: cd and ls don't always work against vfs

Fixes #6997

Author: starpit

plugins/plugin-bash-like/fs/src/lib/cd.ts

@@ -20,15 +20,11 @@
  *
  */
 
-import Debug from 'debug'
-import { join } from 'path'
-
 import { cwd, inBrowser, Arguments, Registrar, i18n, expandHomeDir } from '@kui-shell/core'
 import { localFilepath } from './usage-helpers'
-import { findMount } from '../vfs'
+import { absolute, findMount } from '../vfs'
 
 const strings = i18n('plugin-bash-like')
-const debug = Debug('plugins/bash-like/cmds/general')
 
 const usage = {
   cd: {
@@ -52,28 +48,28 @@ const cd = async (args: Arguments) => {
 
   const dir = !dirAsProvided ? expandHomeDir('~') : dirAsProvided === '-' ? process.env.OLDPWD : dirAsProvided
 
-  const mount = findMount(dir)
+  const mount = findMount(dir, undefined, true)
   try {
-    const resolveDir =
-      mount.isLocal || dirAsProvided === '-' ? dir : process.env.VIRTUAL_CWD ? join(process.env.VIRTUAL_CWD, dir) : dir
-    debug('cd dir', resolveDir)
-    const stat = await mount.fstat(args, resolveDir)
+    const { isDirectory, fullpath } = !mount
+      ? { isDirectory: true, fullpath: absolute(dir) }
+      : await mount.fstat(args, absolute(dir))
+    const isLocal = mount && mount.isLocal
 
-    if (stat.isDirectory) {
+    if (isDirectory) {
       if (process.env.OLDPWD === undefined) {
         process.env.OLDPWD = ''
       }
 
       const OLDPWD = cwd() // remember it for when we're done
-      const newDir = expandHomeDir(stat.fullpath)
+      const newDir = expandHomeDir(fullpath)
 
-      if (mount.isLocal && !inBrowser()) {
+      if (isLocal && !inBrowser()) {
         process.chdir(newDir)
       }
 
       process.env.OLDPWD = OLDPWD
       process.env.PWD = newDir
-      if (mount.isLocal) {
+      if (isLocal) {
         delete process.env.VIRTUAL_CWD
       } else {
         process.env.VIRTUAL_CWD = newDir

plugins/plugin-bash-like/fs/src/vfs/delegates.ts

@@ -16,7 +16,7 @@
 
 import { basename } from 'path'
 import { Arguments, CodedError, Table, flatten } from '@kui-shell/core'
-import { DirEntry, VFS, findMount, multiFindMount, findMatchingMounts } from '.'
+import { DirEntry, VFS, absolute, findMount, multiFindMount, findMatchingMounts } from '.'
 
 /** '/a/b/c' -> 3 */
 function countSlashes(path: string) {
@@ -95,24 +95,46 @@ async function lsMounts(path: string): Promise<DirEntry[]> {
  *
  */
 export async function ls(...parameters: Parameters<VFS['ls']>): Promise<DirEntry[]> {
-  const mounts = multiFindMount(parameters[1], true)
-  if (mounts.length === 0) {
-    const err: CodedError = new Error(`VFS not mounted: ${parameters[1]}`)
+  const filepaths = parameters[1].length === 0 ? [process.env.PWD] : parameters[1].map(absolute)
+
+  // the first maintains the mapping from input to mountContent: DirEntry[][]
+  // the second flattens this down to a DirEntry[]
+  const mountContentPerInput = await Promise.all(filepaths.map(lsMounts))
+  const mountContent = flatten(mountContentPerInput)
+
+  const mounts = multiFindMount(filepaths, true)
+  if (mounts.length === 0 && mountContent.length === 0) {
+    const err: CodedError = new Error(`VFS not mounted: ${filepaths}`)
     err.code = 404
     throw err
   }
 
-  const [mountContent, vfsContent] = await Promise.all([
-    Promise.all((parameters[1].length === 0 ? [process.env.PWD] : parameters[1]).map(lsMounts)).then(flatten),
-    Promise.all(mounts.map(({ filepaths, mount }) => mount.ls(parameters[0], filepaths))).then(flatten)
-  ])
+  const vfsContent = (
+    await Promise.all(
+      mounts.map(async ({ filepaths, mount }) => {
+        try {
+          return await mount.ls(parameters[0], filepaths)
+        } catch (err) {
+          if (err.code !== 404) {
+            console.error(err)
+            throw err
+          }
+        }
+      })
+    ).then(flatten)
+  ).filter(_ => _)
 
-  if (mountContent.length > 0) {
+  if (mountContent.length === 0) {
+    if (vfsContent.length === 0) {
+      // TODO: if no matches, we need to check whether the filepaths are
+      // all directories (and no -d); only then is an empty response valid.
+      // Otherwise, we need to report a 404.
+    }
+    return vfsContent
+  } else {
     return mountContent.concat(vfsContent).sort((a, b) => {
       return a.name.localeCompare(b.name)
     })
-  } else {
-    return vfsContent
   }
 }
 

plugins/plugin-bash-like/fs/src/vfs/index.ts

@@ -182,7 +182,7 @@ export async function mount(vfs: VFS | VFSProducingFunction) {
 }
 
 /** @return the absolute path to `filepath` */
-function absolute(filepath: string): string {
+export function absolute(filepath: string): string {
   return isAbsolute(filepath) ? filepath : join(cwd(), filepath)
 }
 
@@ -198,17 +198,75 @@ export function findMatchingMounts(filepath: string, checkClient = false): VFS[]
   return mounts
 }
 
-/** Lookup compiatible mount */
-export function findMount(filepath: string, checkClient = false): VFS {
+/**
+ * Lookup compiatible mount. Returns one of:
+ * - a VFS mount, if it encloses the given filepath
+ *
+ * - true if `allowInner` and there exists a mount s.t. filepath
+ *     encloses it (i.e. filepath is a parent of some mount)
+ *
+ * - otherwise, the local VFS mount
+ */
+export function findMount(filepath: string, checkClient = false, allowInner = false): VFS {
   const isClient = inBrowser()
   filepath = absolute(filepath)
-  const mount =
-    _currentMounts.find(
-      mount => filepath.startsWith(mount.mountPath) && (!checkClient || !isClient || mount.isVirtual)
-    ) || _currentMounts.find(mount => mount.isLocal) // local fallback; see https://github.com/IBM/kui/issues/5898
 
-  debug(`findMount ${filepath}->${mount.mountPath}`)
-  return mount
+  // filepath: /kuifake   Possibilities limited to [/kuifake]
+  // mounts: ['/kuifake/fake1', '/tmpo', '/kuifake/fake2', '/kui', '/']
+  // first loop should find nothing
+  // if allowInner, second loop should return true; done!
+
+  // filepath: /kuifake/fake1  Possibilities limited to [/kuifake, /kuifake/fake1]
+  // mounts: ['/kuifake/fake1', '/tmpo', '/kuifake/fake2', '/kui', '/']
+  // first loop should find /kuifake/fake1; done!
+
+  // filepath: /kuifake/fake1/E1/f1  Possibilities limited to [/kuifake, /kuifake/fake1, /kuifake/fake1/E1, /kuifake/fake1/E1/f1]
+  // mounts: ['/kuifake/fake1', '/tmpo', '/kuifake/fake2', '/kui', '/']
+  // first loop should find /kuifake/fake1; done!
+
+  // filepath: /a/b/c/d
+  // mounts: ['/kuifake/fake1', '/tmpo', '/kuifake/fake2', '/kui', '/']
+  // first loop should find '/'; done!
+
+  // This loop checks if any mount **encloses** the given filepath (hence startsWith())
+  // Important: search from longest to shortest!! e.g. we don't want
+  // /aaa/b/c to match a mount /a when we also have a mount /aaa/b
+  const splitPath = filepath.split(/\//)
+  const possibilities: string[] = []
+  for (let idx = 1; idx < splitPath.length; idx++) {
+    if (splitPath[idx]) {
+      possibilities.push('/' + splitPath.slice(1, idx + 1).join('/'))
+    }
+  }
+
+  let foundMount: VFS
+  for (let idx = 0; idx < _currentMounts.length; idx++) {
+    const mount = _currentMounts[idx]
+    const { mountPath } = mount
+
+    if (possibilities.includes(mountPath) && (!checkClient || !isClient || mount.isVirtual)) {
+      foundMount = mount
+      break
+    }
+  }
+
+  if (!foundMount) {
+    if (allowInner) {
+      // ok, then look for a mount where the given filepath encloses the mount
+      for (let idx = 0; idx < _currentMounts.length; idx++) {
+        const mount = _currentMounts[idx]
+        if (mount.mountPath.startsWith(filepath) && (!checkClient || !isClient || mount.isVirtual)) {
+          return
+        }
+      }
+    }
+
+    // local fallback; see https://github.com/IBM/kui/issues/5898
+    foundMount = _currentMounts.find(mount => mount.isLocal)
+  }
+
+  debug(`findMount ${filepath}->${foundMount.mountPath}`)
+  return foundMount
 }
 
 /** Lookup compatible mounts */

plugins/plugin-client-test/src/test/api2/vfs.ts

@@ -75,6 +75,22 @@ describe('ls versus vfs', function(this: Common.ISuite) {
     CLI.command('ls -l', this.app)
       .then(ReplExpect.okWith('kuifake'))
       .catch(Common.oops(this, true)))
+  it('should ls kuifake from / and show fake1', () =>
+    CLI.command('ls -l kuifake', this.app)
+      .then(ReplExpect.okWith('fake1'))
+      .catch(Common.oops(this, true)))
+  it('should ls kuifake from / and show fake2', () =>
+    CLI.command('ls -l kuifake', this.app)
+      .then(ReplExpect.okWith('fake2'))
+      .catch(Common.oops(this, true)))
+  it('should ls kuifake/ from / and show fake1', () =>
+    CLI.command('ls -l kuifake/', this.app)
+      .then(ReplExpect.okWith('fake1'))
+      .catch(Common.oops(this, true)))
+  it('should ls kuifake/ from / and show fake2', () =>
+    CLI.command('ls -l kuifake/', this.app)
+      .then(ReplExpect.okWith('fake2'))
+      .catch(Common.oops(this, true)))
 
   // now try cd /tmpo and make sure ls works against PWD
   it('should cd /tmpo', () =>
@@ -89,4 +105,46 @@ describe('ls versus vfs', function(this: Common.ISuite) {
     CLI.command('ls -l', this.app)
       .then(ReplExpect.okWith('D2'))
       .catch(Common.oops(this, true)))
+
+  // now try cd /kuifake and make sure ls works against PWD
+  it('should cd /kuifake', () =>
+    CLI.command('cd /kuifake', this.app)
+      .then(ReplExpect.okWithString('/kuifake'))
+      .catch(Common.oops(this, true)))
+  it('should ls and show fake1', () =>
+    CLI.command('ls -l', this.app)
+      .then(ReplExpect.okWith('fake1'))
+      .catch(Common.oops(this, true)))
+  it('should ls and show fake2', () =>
+    CLI.command('ls -l', this.app)
+      .then(ReplExpect.okWith('fake2'))
+      .catch(Common.oops(this, true)))
+
+  // now try cd fake1 (relative cd!)
+  it('should cd fake1', () =>
+    CLI.command('cd fake1', this.app)
+      .then(ReplExpect.okWithString('fake1'))
+      .catch(Common.oops(this, true)))
+  it('should ls show E1', () =>
+    CLI.command('ls -l E1', this.app)
+      .then(ReplExpect.ok)
+      .catch(Common.oops(this, true)))
+  it('should ls show E2', () =>
+    CLI.command('ls -l E2', this.app)
+      .then(ReplExpect.ok)
+      .catch(Common.oops(this, true)))
+
+  // now try cd ../fake2 (relative cd!)
+  it('should cd ../fake2', () =>
+    CLI.command('cd ../fake2', this.app)
+      .then(ReplExpect.okWithString('fake2'))
+      .catch(Common.oops(this, true)))
+  it('should ls show F1', () =>
+    CLI.command('ls -l F1', this.app)
+      .then(ReplExpect.ok)
+      .catch(Common.oops(this, true)))
+  it('should ls show F2', () =>
+    CLI.command('ls -l F2', this.app)
+      .then(ReplExpect.ok)
+      .catch(Common.oops(this, true)))
 })

plugins/plugin-core-support/src/notebooks/vfs/index.ts

@@ -30,6 +30,7 @@ interface Tutorial {
 
 interface BaseEntry {
   mountPath: string
+  isDirectory?: boolean
 }
 
 type Directory = BaseEntry
@@ -74,24 +75,32 @@ export class NotebookVFS implements VFS {
   }
 
   /** Looks in the trie for any matches for the given filepath, handling the "contents of directory" case */
-  private find(filepath: string): Entry[] {
+  private find(filepath: string, dashD = false, exact = false): Entry[] {
     const dirPattern = this.dirPattern(filepath)
-
-    return this.trie
+    const flexMatches = this.trie
       .get(filepath.replace(/\*.*$/, ''))
-      .filter(_ => micromatch.isMatch(_.mountPath, filepath) || dirPattern.test(_.mountPath))
+      .filter(_ =>
+        exact ? _.mountPath === filepath : micromatch.isMatch(_.mountPath, filepath) || dirPattern.test(_.mountPath)
+      )
+    if (dashD) {
+      return flexMatches.filter(_ => _.isDirectory)
+    } else if (exact) {
+      return flexMatches
+    } else {
+      return flexMatches.filter(_ => _.mountPath !== filepath)
+    }
   }
 
   /** Looks in the trie for a single precise match */
   private findExact(filepath: string, withData: boolean): FStat {
-    const flexMatches = this.find(filepath) // all glob and/or directory matches
-    const possibleMatches = flexMatches.filter(_ => _.mountPath === filepath)
+    const possibleMatches = this.find(filepath, false, true)
 
     if (possibleMatches.length > 1) {
       const msg = 'Multiple matches'
       console.error(msg, possibleMatches)
       throw new Error(msg)
     } else if (possibleMatches.length === 0) {
+      const flexMatches = this.find(filepath, false, false)
       if (filepath === this.mountPath || flexMatches.find(_ => _.mountPath.startsWith(filepath))) {
         // then this is either a match against the mount position or an interior directory
         return {
@@ -146,10 +155,10 @@ export class NotebookVFS implements VFS {
     })
   }
 
-  public async ls(_, filepaths: string[]) {
+  public async ls({ parsedOptions }: Parameters<VFS['ls']>[0], filepaths: string[]) {
     return flatten(
       filepaths
-        .map(filepath => ({ filepath, entries: this.find(filepath) }))
+        .map(filepath => ({ filepath, entries: this.find(filepath, parsedOptions.d) }))
         .filter(_ => _.entries.length > 0)
         .map(_ => this.enumerate(_))
     )
@@ -219,7 +228,7 @@ export class NotebookVFS implements VFS {
   /** Create a directory/bucket */
   public async mkdir(opts: Pick<Arguments, 'argvNoOptions'>): Promise<void> {
     const mountPath = opts.argvNoOptions[opts.argvNoOptions.indexOf('mkdir') + 1]
-    this.trie.map(mountPath, { mountPath })
+    this.trie.map(mountPath, { mountPath, isDirectory: true })
   }
 
   /** Remove a directory/bucket */