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

Handle staged changes with special characters in the filename #1667

Merged
merged 3 commits into from Aug 24, 2018

Conversation

Projects
None yet
3 participants
@annthurium
Contributor

annthurium commented Aug 24, 2018

Description of the Change

when a user clicks on a changed file in the StagingView, the github package creates a uri to pass to atom.workspace.open() so the user can open the file and view its contents.

However, we were not properly encoding all components of this URI. Which is only a problem if there are special characters in the filename that need to be escaped. In this case, an invalid URI would be generated, Atom would look for a file that doesn't exist, and show an error state thinking that the entire repository had only been a dream.

This PR addresses this issue by URI encoding the relative path to the file.

Applicable Issues

#1610

annthurium added some commits Aug 24, 2018

URI encode relative path to filepatch
This prevents us from borking the path for files that, for example, 
contain question marks.
@@ -96,6 +96,18 @@ describe('FilePatchController', function() {
getFilePatchForPath = sinon.stub(repository, 'getFilePatchForPath');
});
describe('buildURI', function() {

This comment has been minimized.

@annthurium

annthurium Aug 24, 2018

Contributor

@smashwilson do you think it would be worth it to also add tests at the react component layer? IE, clicking on staged changes and verifying that the filepath is correct? Or is this sufficient?

@annthurium

annthurium Aug 24, 2018

Contributor

@smashwilson do you think it would be worth it to also add tests at the react component layer? IE, clicking on staged changes and verifying that the filepath is correct? Or is this sufficient?

This comment has been minimized.

@smashwilson

smashwilson Aug 24, 2018

Member

This should be fine. I'd say that in this case, good test coverage of the buildURI() method with wacky URIs + good test coverage of launching pane items = good test coverage of launching pane items with wacky URIs. In other words, adding an integration-ish test for this particular loophole wouldn't really catch anything that that wouldn't.

:shipit:

@smashwilson

smashwilson Aug 24, 2018

Member

This should be fine. I'd say that in this case, good test coverage of the buildURI() method with wacky URIs + good test coverage of launching pane items = good test coverage of launching pane items with wacky URIs. In other words, adding an integration-ish test for this particular loophole wouldn't really catch anything that that wouldn't.

:shipit:

This comment has been minimized.

@annthurium

annthurium Aug 24, 2018

Contributor

bitmoji

thank you!

@annthurium

annthurium Aug 24, 2018

Contributor

bitmoji

thank you!

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 24, 2018

Coverage Status

Coverage decreased (-0.07%) to 80.15% when pulling 755bacb on tt-18-aug-question-mark-bug into 71c1781 on master.

coveralls commented Aug 24, 2018

Coverage Status

Coverage decreased (-0.07%) to 80.15% when pulling 755bacb on tt-18-aug-question-mark-bug into 71c1781 on master.

@annthurium annthurium merged commit 7f89ab7 into master Aug 24, 2018

4 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.07%) to 80.15%
Details

@annthurium annthurium deleted the tt-18-aug-question-mark-bug branch Aug 24, 2018

@smashwilson smashwilson referenced this pull request Oct 5, 2018

Open

Render file patches with a decorated Editor #1512

88 of 92 tasks complete

smashwilson added a commit that referenced this pull request Oct 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment