[rubygems/rubygems] Modify bundle doctor to not report issue when files aren't writable:

- ### Problem

  Running `bundle doctor` warn about files that aren't writable.
  This makes the output of `bundle doctor` very verbose for something
  I believe isn't really an issue.

  ### Context

  Rubygems keeps the files original permission at the time the gem
  is packaged.
  Many gem maintainers have decided that the permissions of the files
  in their bundled would be 0444, this includes amongst others:
  minitest, selenium, brakeman...

  Any git gems that had a 0444 permissions at some point in its git
  history would also be reported (as bundle doctor look in the
  `cache/bundler/git/<gem>/object` path).

  While it completely make sense to report when files aren't readable,
  maybe it's worth questioning the usefulness of reporting files
  that can't be written and what problem this causes to the user
  (if any).

  ### Solution

  Removed the check for unwritable file.

  ### Side note

  I also tweaked the "No issues ..." message logic as it was doing
  the opposite (reporting an issue when there is none and vice versa).
  This wasn't caught in tests because as a stub on `Bundler.ui.info`
  was missing.

ruby/rubygems@9a426b9495

Author: Edouard-chin

lib/bundler/cli/doctor.rb

@@ -99,7 +99,7 @@ def run
           end
         end.sort.each {|m| message += m }
         raise ProductionError, message
-      elsif !permissions_valid
+      elsif permissions_valid
         Bundler.ui.info "No issues found with the installed bundle"
       end
     end
@@ -108,21 +108,21 @@ def run
 
     def check_home_permissions
       require "find"
-      files_not_readable_or_writable = []
-      files_not_rw_and_owned_by_different_user = []
-      files_not_owned_by_current_user_but_still_rw = []
+      files_not_readable = []
+      files_not_readable_and_owned_by_different_user = []
+      files_not_owned_by_current_user_but_still_readable = []
       broken_symlinks = []
       Find.find(Bundler.bundle_path.to_s).each do |f|
         if !File.exist?(f)
           broken_symlinks << f
-        elsif !File.writable?(f) || !File.readable?(f)
+        elsif !File.readable?(f)
           if File.stat(f).uid != Process.uid
-            files_not_rw_and_owned_by_different_user << f
+            files_not_readable_and_owned_by_different_user << f
           else
-            files_not_readable_or_writable << f
+            files_not_readable << f
           end
         elsif File.stat(f).uid != Process.uid
-          files_not_owned_by_current_user_but_still_rw << f
+          files_not_owned_by_current_user_but_still_readable << f
         end
       end
 
@@ -134,23 +134,23 @@ def check_home_permissions
         ok = false
       end
 
-      if files_not_owned_by_current_user_but_still_rw.any?
+      if files_not_owned_by_current_user_but_still_readable.any?
         Bundler.ui.warn "Files exist in the Bundler home that are owned by another " \
-          "user, but are still readable/writable. These files are:\n - #{files_not_owned_by_current_user_but_still_rw.join("\n - ")}"
+          "user, but are still readable. These files are:\n - #{files_not_owned_by_current_user_but_still_readable.join("\n - ")}"
 
         ok = false
       end
 
-      if files_not_rw_and_owned_by_different_user.any?
+      if files_not_readable_and_owned_by_different_user.any?
         Bundler.ui.warn "Files exist in the Bundler home that are owned by another " \
-          "user, and are not readable/writable. These files are:\n - #{files_not_rw_and_owned_by_different_user.join("\n - ")}"
+          "user, and are not readable. These files are:\n - #{files_not_readable_and_owned_by_different_user.join("\n - ")}"
 
         ok = false
       end
 
-      if files_not_readable_or_writable.any?
+      if files_not_readable.any?
         Bundler.ui.warn "Files exist in the Bundler home that are not " \
-          "readable/writable by the current user. These files are:\n - #{files_not_readable_or_writable.join("\n - ")}"
+          "readable by the current user. These files are:\n - #{files_not_readable.join("\n - ")}"
 
         ok = false
       end

spec/bundler/commands/doctor_spec.rb

@@ -14,7 +14,7 @@
 
     @stdout = StringIO.new
 
-    [:error, :warn].each do |method|
+    [:error, :warn, :info].each do |method|
       allow(Bundler.ui).to receive(method).and_wrap_original do |m, message|
         m.call message
         @stdout.puts message
@@ -45,20 +45,20 @@
       allow(File).to receive(:writable?).with(File.expand_path("..", Gem.default_dir))
     end
 
-    it "exits with no message if the installed gem has no C extensions" do
+    it "exits with a success message if the installed gem has no C extensions" do
       doctor = Bundler::CLI::Doctor.new({})
       allow(doctor).to receive(:lookup_with_fiddle).and_return(false)
       expect { doctor.run }.not_to raise_error
-      expect(@stdout.string).to be_empty
+      expect(@stdout.string).to include("No issues")
     end
 
-    it "exits with no message if the installed gem's C extension dylib breakage is fine" do
+    it "exits with a success message if the installed gem's C extension dylib breakage is fine" do
       doctor = Bundler::CLI::Doctor.new({})
       expect(doctor).to receive(:bundles_for_gem).exactly(2).times.and_return ["/path/to/myrack/myrack.bundle"]
       expect(doctor).to receive(:dylibs).exactly(2).times.and_return ["/usr/lib/libSystem.dylib"]
       allow(doctor).to receive(:lookup_with_fiddle).with("/usr/lib/libSystem.dylib").and_return(false)
       expect { doctor.run }.not_to raise_error
-      expect(@stdout.string).to be_empty
+      expect(@stdout.string).to include("No issues")
     end
 
     it "exits with a message if one of the linked libraries is missing" do
@@ -105,19 +105,32 @@
       allow(File).to receive(:stat).with(@unwritable_file) { @stat }
     end
 
-    it "exits with an error if home contains files that are not readable/writable" do
+    it "exits with an error if home contains files that are not readable" do
       doctor = Bundler::CLI::Doctor.new({})
       allow(doctor).to receive(:lookup_with_fiddle).and_return(false)
       allow(@stat).to receive(:uid) { Process.uid }
       allow(File).to receive(:writable?).with(@unwritable_file) { false }
       allow(File).to receive(:readable?).with(@unwritable_file) { false }
       expect { doctor.run }.not_to raise_error
       expect(@stdout.string).to include(
-        "Files exist in the Bundler home that are not readable/writable by the current user. These files are:\n - #{@unwritable_file}"
+        "Files exist in the Bundler home that are not readable by the current user. These files are:\n - #{@unwritable_file}"
       )
       expect(@stdout.string).not_to include("No issues")
     end
 
+    it "exits without an error if home contains files that are not writable" do
+      doctor = Bundler::CLI::Doctor.new({})
+      allow(doctor).to receive(:lookup_with_fiddle).and_return(false)
+      allow(@stat).to receive(:uid) { Process.uid }
+      allow(File).to receive(:writable?).with(@unwritable_file) { false }
+      allow(File).to receive(:readable?).with(@unwritable_file) { true }
+      expect { doctor.run }.not_to raise_error
+      expect(@stdout.string).not_to include(
+        "Files exist in the Bundler home that are not readable by the current user. These files are:\n - #{@unwritable_file}"
+      )
+      expect(@stdout.string).to include("No issues")
+    end
+
     context "when home contains files that are not owned by the current process", :permissions do
       before(:each) do
         allow(@stat).to receive(:uid) { 0o0000 }
@@ -130,7 +143,7 @@
         allow(File).to receive(:readable?).with(@unwritable_file) { false }
         expect { doctor.run }.not_to raise_error
         expect(@stdout.string).to include(
-          "Files exist in the Bundler home that are owned by another user, and are not readable/writable. These files are:\n - #{@unwritable_file}"
+          "Files exist in the Bundler home that are owned by another user, and are not readable. These files are:\n - #{@unwritable_file}"
         )
         expect(@stdout.string).not_to include("No issues")
       end
@@ -142,7 +155,7 @@
         allow(File).to receive(:readable?).with(@unwritable_file) { true }
         expect { doctor.run }.not_to raise_error
         expect(@stdout.string).to include(
-          "Files exist in the Bundler home that are owned by another user, but are still readable/writable. These files are:\n - #{@unwritable_file}"
+          "Files exist in the Bundler home that are owned by another user, but are still readable. These files are:\n - #{@unwritable_file}"
         )
         expect(@stdout.string).not_to include("No issues")
       end