Fix chdir issue for LSP
#2171
Fix chdir issue for LSP
#2171
Conversation
andyw8
force-pushed
the
andyw8/fix-chdir-issue
branch
from
e6f0fd3 to
5c7fcb7
Compare
andyw8
force-pushed
the
andyw8/fix-chdir-issue
branch
from
e9323ad to
1dfeacf
Compare
| @@ -45,7 +46,8 @@ def lockfile_changed? | |||
|
|
|||
| sig { returns(String) } | |||
| def fetch_lockfile_diff | |||
| @lockfile_diff = File.exist?("Gemfile.lock") ? %x(git diff Gemfile.lock).strip : "" | |||
| @lockfile_diff = | |||
| File.exist?("#{project_path}/Gemfile.lock") ? %x(cd #{project_path} && git diff Gemfile.lock).strip : "" | |||
There was a problem hiding this comment.
(we should probbably use File.join instead of interpolation)
andyw8
force-pushed
the
andyw8/fix-chdir-issue
branch
from
014d49b to
d1116c7
Compare
| @@ -45,7 +46,8 @@ def lockfile_changed? | |||
|
|
|||
| sig { returns(String) } | |||
| def fetch_lockfile_diff | |||
| @lockfile_diff = File.exist?("Gemfile.lock") ? %x(git diff Gemfile.lock).strip : "" | |||
| @lockfile_diff = | |||
| File.exist?("#{project_path}/Gemfile.lock") ? execute_in_project_path("git diff Gemfile.lock").strip : "" | |||
There was a problem hiding this comment.
This should be using Bundler methods to find the lockfile.
Bundler.with_original_env { Bundler.default_lockfile }
andyw8
force-pushed
the
andyw8/fix-chdir-issue
branch
from
f6052b2 to
e04572f
Compare
andyw8
force-pushed
the
andyw8/fix-chdir-issue
branch
from
169bf19 to
cb81ea5
Compare
andyw8
force-pushed
the
andyw8/fix-chdir-issue
branch
from
cb81ea5 to
29cf4ef
Compare
| sig { returns(T::Boolean) } | ||
| def git_repo? | ||
| _, status = Open3.capture2e("git rev-parse --is-inside-work-tree") | ||
| T.must(status.success?) | ||
| !!system("git rev-parse --is-inside-work-tree", chdir: project_path) |
There was a problem hiding this comment.
I get why we are doing a !! here, but it is totally fine for a ? method to return T.nilable(T::Boolean), which would remove the need for this operation.
Predicate methods in Ruby return truthy or falsy values, not booleans.
| @@ -82,35 +87,46 @@ def execute_tapioca_gem_command(gems) | |||
|
|
|||
| sig { params(gems: T::Array[String]).void } | |||
| def remove_rbis(gems) | |||
| FileUtils.rm_f(Dir.glob("sorbet/rbi/gems/{#{gems.join(",")}}@*.rbi")) | |||
| FileUtils.rm_f(Dir.glob("sorbet/rbi/gems/{#{gems.join(",")}}@*.rbi", base: project_path)) | |||
There was a problem hiding this comment.
I don't think this is correct. From the docs:
If optional keyword argument
baseis given, its value specifies the base directory. Each pattern string specifies entries relative to the base directory; the default is'.'. The base directory is not prepended to the entry names in the result:Dir.glob(pattern, base: 'lib').take(5) # => ["abbrev.gemspec", "abbrev.rb", "base64.gemspec", "base64.rb", "benchmark.gemspec"]
You will need to then File.join all the glob results with the project_path before passing them to FileUtils.rm_f.
In any case, for readability, it would be great to pull out the glob result into a local variable.
| untracked_files = execute_in_project_path( | ||
| "git", | ||
| "ls-files", | ||
| "--others", | ||
| "--exclude-standard", | ||
| "sorbet/rbi/gems/", | ||
| ).lines.map(&:strip) | ||
| deleted_files = execute_in_project_path("git", "ls-files", "--deleted", "sorbet/rbi/gems/").lines.map(&:strip) |
There was a problem hiding this comment.
It feels like this is missing a git_ls_gem_rbis abstraction:
| untracked_files = execute_in_project_path( | |
| "git", | |
| "ls-files", | |
| "--others", | |
| "--exclude-standard", | |
| "sorbet/rbi/gems/", | |
| ).lines.map(&:strip) | |
| deleted_files = execute_in_project_path("git", "ls-files", "--deleted", "sorbet/rbi/gems/").lines.map(&:strip) | |
| untracked_files = git_ls_gem_rbis("--others", "--exclude-standard") | |
| deleted_files = git_ls_gem_rbis("--deleted") |
|
|
||
| delete_files(untracked_files, "Deleted untracked RBIs") | ||
| restore_files(deleted_files, "Restored deleted RBIs") | ||
| end | ||
|
|
||
| sig { params(files: T::Array[String], message: String).void } | ||
| def delete_files(files, message) | ||
| files.each { |file| File.delete(file) } | ||
| FileUtils.rm(files.map { |file| File.join(project_path, file) }) |
There was a problem hiding this comment.
Again, we are doing this operation in two places, remove_rbis and here in this method. We seem to be missing a common abstraction that will take relative file paths and remove them after mapping them to absolute paths.
| log_message("#{message}: #{files.join(", ")}") unless files.empty? | ||
| end | ||
|
|
||
| sig { params(files: T::Array[String], message: String).void } | ||
| def restore_files(files, message) | ||
| files.each { |file| %x(git checkout -- #{file}) } | ||
| files.each { |file| execute_in_project_path("git", "checkout", "--", file) } |
There was a problem hiding this comment.
It would be great to not need to execute multiple commands. If we join all the filenames, it could be too long, but we can stream them into the stdin of the execution, and use the --paths-from-file=- flag of git checkout to do all of this in one operation.
Something like:
| files.each { |file| execute_in_project_path("git", "checkout", "--", file) } | |
| execute_in_project_path("git", "checkout", "--pathspec-from-file=-", stdin: files.join("\n")) |
| def execute_in_project_path(*parts) | ||
| stdout_and_stderr, _status = T.unsafe(Open3).capture2e(*parts, chdir: project_path) | ||
| stdout_and_stderr | ||
| end |
There was a problem hiding this comment.
As part of my suggestion above:
| def execute_in_project_path(*parts) | |
| stdout_and_stderr, _status = T.unsafe(Open3).capture2e(*parts, chdir: project_path) | |
| stdout_and_stderr | |
| end | |
| def execute_in_project_path(*parts, stdin: nil) | |
| options = {} | |
| options[:chdir] = project_path | |
| options[:stdin_data] = stdin if stdin | |
| stdout_and_stderr, _status = T.unsafe(Open3).capture2e(*parts, options) | |
| stdout_and_stderr | |
| end |
Motivation
After #2081, we began so to see an
Error loading add-ons: RuboCop: conflicting chdir during another chdir block.Implementation
We don't want to run
chdirsince it may impact other things Ruby LSP is doing, since the path will be wrong.Tests
Updated.
@alexcrocha has verified on Core.