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.

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

Fix path separator used by normalizeTarEntry() under Windows #8

Merged
merged 1 commit into from Apr 16, 2019

Conversation

Projects
None yet
3 participants
@pdcastro
Copy link
Contributor

commented Apr 15, 2019

Connects-to: balena-io/balena-cli#1133
Change-type: patch
Signed-off-by: Paulo Castro paulo@balena.io

@pdcastro pdcastro self-assigned this Apr 15, 2019

@pdcastro pdcastro requested review from zvin, CameronDiver, hedss and dfunckt Apr 15, 2019

@zvin

zvin approved these changes Apr 15, 2019

@pdcastro pdcastro force-pushed the fix-normalize-path-separator branch from 41fac9e to 9481089 Apr 15, 2019

@pdcastro

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

I've just force-pushed a complete replacement of the previous code, because I when I actually tested it on Windows, it failed. @zvin and others, apologies for having placed the original PR for review before having tested it on Windows (luckily it was a very small change...). What is now up for review has been tested on Windows and MacOS. Thanks!

@pdcastro pdcastro requested a review from zvin Apr 15, 2019

@pdcastro pdcastro force-pushed the fix-normalize-path-separator branch from 9481089 to 5e0d2fd Apr 15, 2019

@pdcastro

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

OK, one more change. The tests were passing on Windows and MacOS, yes, but then I thought of additional test cases which were failing on all platforms:

input → expected output
'' → ''
'.' → '.'
'..' → '..'
'/' → '.'
'/.' → '.'
'/..' → '.'
'./' → '.'
'../' → '..'
'/./' → '.'
'/../' → '.'

(At least some of those were failing.) Also I have added removal of any trailing slashes. The purpose of normalization is to provide a single representation for multiple ways of referring to the same entity, so for example directories 'foo/bar/' and 'foo/bar' are now both normalized as 'foo/bar' (the tar entry header has a specific field for the file type, to distinguish between files and folders, so a trailing slash is not required).

I think the trailing slash removal, however, could be seen as a breaking change (?), and for this reason I have set the change-type to 'major'.

@hedss

hedss approved these changes Apr 16, 2019

}
let normalized = normalize(name);
// remove any leading or trailing slashes
if (normalized.startsWith('/')) {

This comment has been minimized.

Copy link
@hedss

hedss Apr 16, 2019

I don't know how much other code in here would benefit from lodash, but certainly return _.trim(normalized, '/') would save the next 7 lines.

This comment has been minimized.

Copy link
@pdcastro

pdcastro Apr 16, 2019

Author Contributor

Nice! There was also a global noop function that I have now replaced with _.noop. (In the past I thought that noop alone wasn't enough reason to add lodash as a dependency, but _.trim tipped the balance for me.) :-)

* Borrowed from:
* https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Regular_Expressions
*/
function escapeRE(s: string) {

This comment has been minimized.

Copy link
@hedss

hedss Apr 16, 2019

I guess lodash would be useful then, as there's a _.escapeRegExp() method that does exactly this.

This comment has been minimized.

Copy link
@pdcastro

pdcastro Apr 16, 2019

Author Contributor

Well, after @zvin's comment, it turns out I don't need a regular expression at all...

*/
function normalizeNonPosix(p: string): string {
return path.normalize(p).replace(pathSepRE, '/');
}

This comment has been minimized.

Copy link
@zvin

zvin Apr 16, 2019

You can use path.posix.normalize here.

This comment has been minimized.

Copy link
@pdcastro

pdcastro Apr 16, 2019

Author Contributor

What?!? Where did that come from?! I had just saved several lines of code using lodash following @hedss comment, now I can delete a couple more functions too. What next? I guess if I just wait another half an hour, someone else will add a comment: "oh, you're using tar-utils? you don't need that, just import genie and you're done!" or maybe: "oh, all of that to fix the CLI under Windows? you don't need the CLI, just say 'Siri, push the app to balena' and it's all sorted".
LOL :-)

This comment has been minimized.

Copy link
@zvin

zvin Apr 16, 2019

Unfortunately, we're not using python https://xkcd.com/353/ :)

@zvin zvin self-requested a review Apr 16, 2019

Fix path separator used by normalizeTarEntry() under Windows.
normalizeTarEntry() now also removes trailing slashes, reason why
the change-type is major.

Connects-to: balena-io/balena-cli#1133
Change-type: major
Signed-off-by: Paulo Castro <paulo@balena.io>

@pdcastro pdcastro force-pushed the fix-normalize-path-separator branch from 5e0d2fd to 7aaa906 Apr 16, 2019

stream.removeListener('end', onEnd || noop);
stream.removeListener('data', onData || _.noop);
stream.removeListener('error', onError || _.noop);
stream.removeListener('end', onEnd || _.noop);

This comment has been minimized.

Copy link
@pdcastro

pdcastro Apr 16, 2019

Author Contributor

Now that lodash was added as a dependency, might as well use its _.noop.

@@ -124,7 +150,7 @@ describe('multicastStream', function() {
const nStreams = 3;
for (let i = 0; i < nStreams; ++i) {
toStreams.push(
new MockWritable({ highWaterMark: hwm / (i + 1) }, i * 10),
new MockWritable({ highWaterMark: Math.ceil(hwm / (i + 1)) }, i * 10),

This comment has been minimized.

Copy link
@pdcastro

pdcastro Apr 16, 2019

Author Contributor

This change prevents a non-integer highWaterMark value like 16384 / 3 = 5461.3. Oddly the test was passing under MacOS and Linux, but failing under Windows.

@zvin

zvin approved these changes Apr 16, 2019

@pdcastro pdcastro merged commit 460c2a7 into master Apr 16, 2019

12 checks passed

ResinCI/check-commits/commits Commits checks passed
Details
ResinCI/check-files/license License check passed
Details
ResinCI/check-files/readme Readme check passed
Details
ResinCI/check-versionist/versionist Automatic versioning can be performed
Details
ResinCI/npm/linux-alpine-x86_64-node10 Done!
Details
ResinCI/npm/linux-alpine-x86_64-node6 Done!
Details
ResinCI/npm/linux-alpine-x86_64-node8 Done!
Details
ResinCI/npm/store-npm
Details
ResinCI/store-github/store-github Will successfully increase version on master
Details
ci/circleci: node-10 Your tests passed on CircleCI!
Details
ci/circleci: node-6 Your tests passed on CircleCI!
Details
ci/circleci: node-8 Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.