Skip to content
This repository has been archived by the owner on Mar 3, 2023. It is now read-only.

Make sure path is not undefined (#20871) #22685

Merged
merged 5 commits into from
Aug 31, 2021
Merged

Make sure path is not undefined (#20871) #22685

merged 5 commits into from
Aug 31, 2021

Conversation

bennyborn
Copy link
Contributor

@bennyborn bennyborn commented Jul 2, 2021

Identify the Bug

This fixes a bug where upating the rights (CHMOD) of files mounted on a SMB share causes the path watcher to create an invalid path (see #20871)

Description of the Change

While I do not understand the exact origins how this bug comes together, I still managed to provide a very obvious fix for something that should be a string rather than undefined.

Alternate Designs

none

Possible Drawbacks

none

Verification Process

Working with files from a SMB share (Windows host) on a daily basis it is as simple as having open a SMB mounted directory and updating the rights of the folder via chmod -R 775.

Release Notes

Fixed an error that occurs when working with files from a SMB share

@bennyborn bennyborn changed the title Make sure path is not undefined (fix #20871) Make sure path is not undefined (#20871) Jul 2, 2021
@icecream17
Copy link
Contributor

icecream17 commented Jul 2, 2021

@bennyborn Unforunately you can't use the nullish coalescing operator (because stuff is outdated)
image

Also add spaces around the operators to pass the linter check, which counts as a test:
image

src/path-watcher.js Outdated Show resolved Hide resolved
src/path-watcher.js Outdated Show resolved Hide resolved
Co-authored-by: steven nguyen <nguyeste008@students.garlandisd.net>
@bennyborn
Copy link
Contributor Author

@icecream17 Thanks. Haven't looked into the checks since this morning.

Copy link
Contributor

@sadick254 sadick254 left a comment

Choose a reason for hiding this comment

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

Can we please have some specs on this PR?

@bennyborn
Copy link
Contributor Author

Can we please have some specs on this PR?

I'm not quite sure what you mean by that. What do you need?

@icecream17
Copy link
Contributor

icecream17 commented Jul 2, 2021

spec = test
(I don't know much about creating tests though - but I'd say that https://github.com/atom/atom/blob/master/spec/path-watcher-spec.js would be the file to change)

@bennyborn
Copy link
Contributor Author

I don't know much about creating tests though - but I'd say that https://github.com/atom/atom/blob/master/spec/path-watcher-spec.js would be the file to change

That makes two of us. I could try if I had any clue how the original problem occured in the first place but as stated in my first post I can only provide a fix, not an explanation :\

@DeeDeeG
Copy link
Contributor

DeeDeeG commented Jul 14, 2021

A test that sets up event.oldFile and event.newFile to be undefined (as input) and makes sure that payload.oldPath and payload.path (as output) are each the empty string '' rather than undefined should do it?

Edit: Seems kind of hard to test that level of internal detail in the spec file, because that level of hidden implementation detail stuff isn't exported from src/patchwatcher.js

@sadick254 sadick254 merged commit 53805cd into atom:master Aug 31, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants