Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: make getChildEntry recursively traverse the path #301

Merged
merged 2 commits into from
Dec 8, 2023
Merged

fix: make getChildEntry recursively traverse the path #301

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c5083aa
fix: make getChildEntry recursively traverse the path
whitequark Dec 7, 2023
af64e63
fix: don't assign `entry = entry;`
whitequark Dec 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions packages/preview2-shim/lib/browser/filesystem.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,15 +37,18 @@ function getChildEntry (parentEntry, subpath, openFlags) {
let segmentIdx;
do {
if (!entry || !entry.dir) throw 'not-directory';
segmentIdx = subpath.indexOf('/');
segmentIdx = subpath.indexOf('/', segmentIdx);
Copy link

@bakkot bakkot Dec 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drive by: this will search starting at the previous segmentIdx, but that value was an offset into subpath as it was before the later subpath = subpath.substring(segmentIdx + 1) line. Probably you don't want to have both that line and this one?

Consider running this code on longDirName/short/x - the second time through the loop it will start searching the string "short/x" at index 11 (which is past the end of the string), and so miss the / between "short" and "x".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For posterity, here's the traverse function I eventually ended up writing, which I think is a lot more correct:

  traverse(path, flags = { create: false, remove: false }) {
    let entry = this;
    let separatorAt = -1;
    do {
      if (entry instanceof File)
        throw 'not-directory';
      const files = entry.files;
      separatorAt = path.indexOf('/');
      const segment = separatorAt === -1 ? path : path.substring(0, separatorAt);
      if (separatorAt === -1 && flags.remove)
        delete files[segment];
      else if (segment === '' || segment === '.')
        /* disregard */;
      else if (segment === '..')
        /* hack to make scandir() work */;
      else if (Object.hasOwn(files, segment))
        entry = files[segment];
      else if (flags.create === 'directory' || flags.create === 'file' && separatorAt !== -1)
        entry = files[segment] = new Directory({});
      else if (flags.create === 'file')
        entry = files[segment] = new File(new Uint8Array());
      else
        throw 'no-entry';
      path = path.substring(separatorAt + 1);
    } while (separatorAt !== -1);
    return entry;
  }

It's not quite compatible with this specific shim but I think the algorithm is sound.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for noticing this one @bakkot, the browser implementation is considered experimental and is kinda on the verge of either deprecation or heavy refactoring depending on which way we go here. I do like the idea of refining this implementation into something more like what was discussed in #303 (comment).

For now, it's kinda still placeholder code and lots of code paths are entirely untested, a decision will be made on the deprecation / refactoring soon though so it won't be staying in this state.

For now I posted a follow-up in #309. Any review there would be very welcome. Always fun to have the opportunity to use a bodiless conditional...

const segment = segmentIdx === -1 ? subpath : subpath.slice(0, segmentIdx);
if (segment === '.' || segment === '') return entry;
if (segment === '..') throw 'no-entry';
if (!entry.dir[segment] && openFlags.create)
if (segment === '.' || segment === '')
/* continue traversing */;
else if (segment === '..')
throw 'no-entry';
else if (!entry.dir[segment] && openFlags.create)
entry = entry.dir[segment] = openFlags.directory ? { dir: {} } : { source: new Uint8Array([]) };
else
entry = entry.dir[segment];
} while (segmentIdx !== -1)
subpath = subpath.substring(segmentIdx + 1);
} while (segmentIdx !== -1);
if (!entry) throw 'no-entry';
return entry;
}
Expand Down