Fix regression, auto readlink on symlinks again #1830
Conversation
| @@ -75,10 +81,11 @@ export function getUploadZipSpecification( | |||
| - file3.txt | |||
| */ | |||
| for (let file of filesToZip) { | |||
| if (!fs.existsSync(file)) { | |||
| const stats = fs.lstatSync(file, {throwIfNoEntry: false}) | |||
There was a problem hiding this comment.
mildly important part: we switched from statSync to lstatSync
similar to
statunlesspathrefers to a symbolic link, in which case the link itself is stat-ed, not the file that it refers to
we'll use these stats to check isSymbolicLink when we add the file to the zip, and resolve the target's contents.
| throw new Error(`File ${file} does not exist`) | ||
| } | ||
| if (!fs.statSync(file).isDirectory()) { | ||
| if (!stats.isDirectory()) { |
There was a problem hiding this comment.
Can you have a symbolic link to a directory?
Do we need a normal stat to check this?
There was a problem hiding this comment.
Can you have a symbolic link to a directory?
Yes you can!
Do we need a normal stat to check this?
I don't think so. As long as we know it's a directory that's all that matters. We add the symlink's path as the destinationPath below will a sourcePath of null, so when we create the directory in the zip, it's just a normal directory.
Directory symlinks don't need to be explicitly followed since our globber will do that for us when traversing the files. So as long as we know that it's a directory (symlink or not), that's all that matters.
| * Information about the file | ||
| * https://nodejs.org/api/fs.html#class-fsstats | ||
| */ | ||
| stats: fs.Stats |
There was a problem hiding this comment.
Since this is in the src/internal directory, does that mean this interface won't be used by consumers of this package?
Would it make sense to export the minimal information we need rather than the entire fs.Stats interface?
There was a problem hiding this comment.
The reason why I added the fs.Stats interface is if we eventually want to add file permissions, see this PR:
The stats and mode can eventually be passed to the archiver library. I didn't want to do that yet to avoid additional changes.
With the changes introduced in:
Which was meant to resolve:
We introduced a new bug:
We had a slight regression with symlinks. The typical behavior is that they are auto-resolved and the contents of the zip are added at the link's location. With the lazy-stream wrapper being used, it did not resolve the symlinks.
This PR switches our
statto anlstat, and we can check for a symbolic link, then resolve it, when we add the file to the zip archive.