Catch exception in relative path #5427

orgads · 2018-01-10T06:51:35Z

F:/Ruby/lib/ruby/2.4.0/pathname.rb:520:in `relative_path_from': different prefix: "D:/" and "C:/Directory" (ArgumentError)
	from path_util.rb:19:in `relative_path'
	from config.rb:363:in `path_relative_to_config'
	from cop/cop.rb:209:in `block in file_name_matches_any?'
	from cop/cop.rb:204:in `any?'
	from cop/cop.rb:204:in `file_name_matches_any?'
	from cop/cop.rb:184:in `relevant_file?'
	from cop/cop.rb:189:in `excluded_file?'
	from cop/commissioner.rb:71:in `block in remove_irrelevant_cops'
	from cop/commissioner.rb:71:in `reject!'
	from cop/commissioner.rb:71:in `remove_irrelevant_cops'
	from cop/commissioner.rb:55:in `investigate'
	from cop/team.rb:114:in `investigate'
	from cop/team.rb:102:in `offenses'
	from cop/team.rb:44:in `inspect_file'
	from runner.rb:258:in `inspect_file'
	from script.rb:16:in `_inspect_code'
	from script.rb:38:in `parse'
	from script.rb:57:in `main'
	from script.rb:61:in `<main>'```

jonas054 · 2018-01-10T08:27:28Z

Please add an example in spec/rubocop/path_util_spec.rb that fails before your change.

bbatsov · 2018-01-10T17:37:56Z

And a changelog entry. :-)

orgads · 2018-01-10T20:42:50Z

Done, I hope. :)

garettarrowood · 2018-01-11T00:39:54Z

CHANGELOG.md

@@ -26,6 +26,7 @@
 * [#5357](https://github.com/bbatsov/rubocop/issues/5357): Fix `Lint/InterpolationCheck` false positives on escaped interpolations. ([@pocke][])
 * [#5409](https://github.com/bbatsov/rubocop/issues/5409): Fix multiline indent for `Style/SymbolArray` and `Style/WordArray` autocorrect. ([@flyerhzm][])
 * [#5393](https://github.com/bbatsov/rubocop/issues/5393): Fix `Rails/Delegate`'s false positive with a method call with arguments. ([@pocke][])
+* Fix exception when executing from a different drive on Windows. ([@orgads][])


This is causing your CI failures. You'll have to add your github handle at the bottom of the file as well.

pocke · 2018-01-12T13:59:22Z

lib/rubocop/path_util.rb

-      path_name.relative_path_from(Pathname.new(base_dir)).to_s
+      begin
+        path_name.relative_path_from(Pathname.new(base_dir)).to_s
+      rescue ArgumentError


It rescues unexpected errors. For example, Pathname#relative_path_from raises an error when relative path and absolute path are mixed. It will conceals RuboCop's bug.

require 'pathname' Pathname('foo/').relative_path_from(Pathname('/bar'))

$ ruby test.rb /home/pocke/.rbenv/versions/trunk/lib/ruby/2.6.0/pathname.rb:522:in `relative_path_from': different prefix: "" and "/bar" (ArgumentError) from test.rb:3:in `<main>'

So, I think it should check start of the path with regexp or something expressly.

File.expand_path converts the path to absolute form

True, but it is one of the examples that raises ArgumentError. ArgumentErorr is raised by many reasons other than the example, and the reasons are bugs in almost cases. So I think it should not rescue ArgumentError.

I'm not sure I understand.
From a quick look at the code, ArgumentError is raised in exactly 2 cases:

Different prefix (which is what this patch covers). Given that both paths are absolute, this should only be triggered on Windows with different drives.

If the base directory (the argument to relative_path_from) contains .., but File.expand_path removes all these as well, so this shouldn't happen.
Can you demonstrate a real bad case?

Currently, probably it raises ArgumentError only if the paths on different drives. But I think rescuing has big maintenance cost.
If we use rescue ArgumentError, we should be careful that ArgumentError is not raised for other reasons in the feature. For example, we cannot notice that we mistake arity.
So I think the rescuing has big maintenance cost. We can reduce the cost if it checks drive name with regular expressions.

I guess we can implement the regexp easily, but I'm not sure the difficulty because I don't use Windows. So if it's difficult, please tell me the problem. maybe we should use rescuing if implementaion of the regexp checking is hard.

It is difficult, because Pathname also detects other things like UNC names (\\server\share against \\otherserver\othershare). I doubt you want to duplicate all this logic from Pathname, when it provides you with an exception just for that.

pocke · 2018-01-12T14:04:02Z

spec/rubocop/path_util_spec.rb

+    it 'works for different drives' do
+      expect(described_class.relative_path('D:/foo/bar', 'C:/foo'))
+        .to eq('D:/foo/bar')
+    end


I guess the test should run on Windows only.
relative_path methods uses File.expand_path, and FIle.expand_path('D:/foo/bar') returns unexpected value in Linux.

$ pwd /tmp/tmp.tVst8PbbvU $ ruby -e 'p File.expand_path("D:/foo/bar")' "/tmp/tmp.tVst8PbbvU/D:/foo/bar"

So the test case makes no sense in Linux.

It doesn't hurt. So on linux the test case wouldn't have failed even without the fix.

Do you still prefer to limit the test to Windows only?

Do you still prefer to limit the test to Windows only?

Yes, because it doesn't check anything in Linux. So I think it is unnecessary in Linux.

And can you add a test case that checks to work for same drives? For example: relative_path('D:/foo/bar', 'D:/foo')

koic · 2018-01-15T17:08:29Z

CHANGELOG.md

@@ -31,6 +31,7 @@
 * [#5287](https://github.com/bbatsov/rubocop/issues/5287): Do not register an offense in `Style/SafeNavigation` if there is an unsafe method used in a method chain. ([@rrosenblum][])
 * [#5401](https://github.com/bbatsov/rubocop/issues/5401): Fix `Style/RedundantReturn` to trigger when begin-end, rescue, and ensure blocks present. ([@asherkach][])
 * [#5426](https://github.com/bbatsov/rubocop/issues/5426): Make `Rails/InverseOf` accept `inverse_of: nil` to opt-out. ([@wata727][])
+* Fix exception when executing from a different drive on Windows. ([@orgads][])


Please add a link to the source URL just like other entries 📝

-* Fix exception when executing from a different drive on Windows. ([@orgads][]) +* [#5247](https://github.com/bbatsov/rubocop/pull/5427): Fix exception when executing from a different drive on Windows. ([@orgads][])

F:/Ruby/lib/ruby/2.4.0/pathname.rb:520:in `relative_path_from': different prefix: "D:/" and "C:/Directory" (ArgumentError) from path_util.rb:19:in `relative_path' from config.rb:363:in `path_relative_to_config' from cop/cop.rb:209:in `block in file_name_matches_any?' from cop/cop.rb:204:in `any?' from cop/cop.rb:204:in `file_name_matches_any?' from cop/cop.rb:184:in `relevant_file?' from cop/cop.rb:189:in `excluded_file?' from cop/commissioner.rb:71:in `block in remove_irrelevant_cops' from cop/commissioner.rb:71:in `reject!' from cop/commissioner.rb:71:in `remove_irrelevant_cops' from cop/commissioner.rb:55:in `investigate' from cop/team.rb:114:in `investigate' from cop/team.rb:102:in `offenses' from cop/team.rb:44:in `inspect_file' from runner.rb:258:in `inspect_file' from script.rb:16:in `_inspect_code' from script.rb:38:in `parse' from script.rb:57:in `main' from script.rb:61:in `<main>'

pocke

Thank you!🚢

bbatsov · 2018-01-20T00:57:10Z

👍

orgads force-pushed the relative-path branch from 64b4413 to 1ade240 Compare January 10, 2018 20:42

garettarrowood reviewed Jan 11, 2018

View reviewed changes

orgads force-pushed the relative-path branch 3 times, most recently from 6e0f131 to 6d9f199 Compare January 11, 2018 19:51

pocke reviewed Jan 12, 2018

View reviewed changes

orgads force-pushed the relative-path branch from 6d9f199 to a19b114 Compare January 13, 2018 18:44

koic reviewed Jan 15, 2018

View reviewed changes

orgads force-pushed the relative-path branch from a19b114 to 51090ea Compare January 15, 2018 17:56

pocke approved these changes Jan 18, 2018

View reviewed changes

bbatsov merged commit 3c428c5 into rubocop:master Jan 20, 2018

orgads deleted the relative-path branch January 21, 2018 10:23

dependencies bot mentioned this pull request Mar 5, 2018

Update rubocop in / from 0.52.1 to 0.53.0 chauncey-garrett/dotfiles#103

Open

dependabot-preview bot mentioned this pull request Mar 19, 2018

Bump rubocop from 0.52.1 to 0.53.0 tnir/gitlabhq#95

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Catch exception in relative path #5427

Catch exception in relative path #5427

orgads commented Jan 10, 2018

jonas054 commented Jan 10, 2018

bbatsov commented Jan 10, 2018

orgads commented Jan 10, 2018

garettarrowood Jan 11, 2018

pocke Jan 12, 2018

orgads Jan 13, 2018

pocke Jan 15, 2018

orgads Jan 15, 2018

pocke Jan 16, 2018

orgads Jan 16, 2018

pocke Jan 12, 2018

orgads Jan 13, 2018

pocke Jan 15, 2018

koic Jan 15, 2018 •

edited

Loading

pocke left a comment

bbatsov commented Jan 20, 2018

Catch exception in relative path #5427

Catch exception in relative path #5427

Conversation

orgads commented Jan 10, 2018

jonas054 commented Jan 10, 2018

bbatsov commented Jan 10, 2018

orgads commented Jan 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koic Jan 15, 2018 • edited Loading

Choose a reason for hiding this comment

pocke left a comment

Choose a reason for hiding this comment

bbatsov commented Jan 20, 2018

koic Jan 15, 2018 •

edited

Loading