fix: Site.copy doing nothing for trailing slashes

[not-my-profile]

Previously

site.copy("static/");

had no effect whatsoever because of the trailing slash.

This commit fixes this bug to recognize the trailing slash and remove it. The trailing slash is still significant since:

site.copy("foo/");

will now only copy a directory "foo" but not a file named "foo" (as you would expect).

Fixes #425.

[oscarotero]

Hi! There's indeed a bug here!

Your fix seems to be okay, but I think the real issue is in the normalizePath function that should ensure that a path doesn't end with a slash (unless the path is exactly "/").

This is the way Lume handles internally all paths: always use posix separator (even on Windows), always starts with / and, here is the bug, never should end with / (except the path "/").

Would you like to make these changes?

[not-my-profile]

Hi!

I did consider adding the trailing slash trimming to normalizePath, however decided against it since I think it would be counter-intuitive if site.copy("foo/") also copied a file named foo and not just a directory named foo. That is modelling the behavior after the well-known Linux behavior (ENOTDIR), e.g:

• cp foo/ bar will fail if foo isn't a directory with cp: cannot stat 'foo/': Not a directory.
• fs.copyFileSync('foo/', 'bar') will fail if foo isn't a directory with Error: ENOTDIR: not a directory, copyfile 'foo/' -> 'bar'.

Hence I introduced the dirOnly to enable the distinction between foo/ and foo. Only that the Site.copy function currently doesn't report any error/warning if the specified file doesn't exist, which I'd however consider to be part of a larger question about error detection, for which I just opened the issue #427.

If we had a mechanism to report non-fatal errors as suggested in #427, we could make site.copy("foo/") report such an error if foo isn't a directory, and thus be consistent with the vast majority of other IO tools / functions. What do you think about this idea?

[oscarotero]

I see but I think this distinction is no needed because AFAIK, is not possible to have a file and a folder with the same name in the same directory. I think Lume should be able to handle these small details and resolve the correct path without forcing to users to specify it in a specific way. For example, without the normalizePath function, the user would be forced to start all paths with slash, site.copy("foo") wouldn't work, but site.copy("/foo") would.

[not-my-profile]

You're right the distinction isn't strictly needed. I just think it would be weird for site.copy("foo/") to also copy a file named foo if it happens to exist since it looks like you have explicitly indicated the source to be a directory and the common practice for that mismatch scenario is to fail. Note that specifying a trailing slash for directories would still be optional, this PR doesn't force you to use trailing slashes for directories ("foo" would still apply to both files and directories).

[oscarotero]

Okay, good point. Had not thought on that.
Thank!