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

Bugfix - Job Fails Silently When Path Contains Spaces #2472

Merged
merged 2 commits into from
Sep 6, 2019

Conversation

Rican7
Copy link
Contributor

@Rican7 Rican7 commented Sep 4, 2019

Identification

I noticed that certain commands, such as all of the GoTest variants (GoTest, GoTestCompile, GoTestFunc, etc) weren't working for only some of my projects. Interestingly, when they failed, they failed silently, with no logs, messages, or errors.

After trying these commands in other projects, some in module mode, some in my GOPATH, etc, I realized that the consistent issue was for paths that had certain characters, such as spaces.

I narrowed the issue down to an over-eager escaping of a file-path in the go#job#Options function:

\ 'jobdir': fnameescape(expand("%:p:h")),

... which was causing the early return due to the condition that intends to squelch error messages for otherwise "virtual" files:

vim-go/autoload/go/job.vim

Lines 286 to 287 in c6273c5

if has_key(l:options, 'cwd') && !isdirectory(l:options.cwd)
return

I then noticed that not only was the escaping eager, but it was inconsistent with the later attempt to grab the same path, if one wasn't provided:

let l:filedir = expand("%:p:h")

Fix

This PR fixes the over-eager escaping of the file-path of the job directory in go#job#Options that caused paths with certain characters and spaces to fail (silently) to be able to execute a job.

I also made sure to properly escape the path later on, where it was necessary, as its potentially used in a raw command later on that would require this level of escaping.

`go#job#Options` that caused paths with certain characters and spaces to
fail (silently) to be able to execute a job
@bhcleek
Copy link
Collaborator

bhcleek commented Sep 4, 2019

Thank you for the fix! I'll take a close look and probably add some tests before merging. Do you mind if I push commits that add tests to your branch?

@Rican7
Copy link
Contributor Author

Rican7 commented Sep 4, 2019

Thank you for the fix!

Wow! You're quick! No problem!

I've actually been plagued by this issue for months, and just couldn't figure out what the problem was, and finally decided to spend the time to look into it. 😅

I'll take a close look and probably add some tests before merging. Do you mind if I push commits that add tests to your branch?

That would be great! I wouldn't mind at all!

I'm sorry that I didn't include tests myself. I usually never open a PR without including tests, but I couldn't think of a good way to actually test running in a path with spaces.

(Also, I've never actually written a test in VimL 😆... I'm still new to the plugin contribution world.)

@Rican7
Copy link
Contributor Author

Rican7 commented Sep 4, 2019

I appreciate your hard-work on this project!

Thanks again!

image

@bhcleek bhcleek merged commit 856f042 into fatih:master Sep 6, 2019
bhcleek added a commit that referenced this pull request Sep 6, 2019
@Rican7
Copy link
Contributor Author

Rican7 commented Sep 6, 2019

Thanks again, @bhcleek!! 🙌

@Rican7 Rican7 deleted the bugfix/job-cwd-spaces branch September 6, 2019 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants