Merge pull request #8455 from reitermarkus/document-svn

Refactor and document `Svn`.

Library/Homebrew/dependency_collector.rb

@@ -60,7 +60,7 @@ class DependencyCollector
   end
 
   def subversion_dep_if_needed(tags)
-    return if Utils.svn_available?
+    return if Utils::Svn.available?
 
     Dependency.new("subversion", tags)
   end

Library/Homebrew/dev-cmd/audit.rb

@@ -1088,9 +1088,9 @@ module Homebrew
           problem "The URL #{url} is not a valid git URL" unless Utils::Git.remote_exists? url
         elsif strategy <= SubversionDownloadStrategy
           next unless DevelopmentTools.subversion_handles_most_https_certificates?
-          next unless Utils.svn_available?
+          next unless Utils::Svn.available?
 
-          problem "The URL #{url} is not a valid svn URL" unless Utils.svn_remote_exists? url
+          problem "The URL #{url} is not a valid svn URL" unless Utils::Svn.remote_exists? url
         end
       end
     end

Library/Homebrew/download_strategy.rb

@@ -523,7 +523,7 @@ class SubversionDownloadStrategy < VCSDownloadStrategy
   end
 
   def source_modified_time
-    time = if Version.create(Utils.svn_version) >= Version.create("1.9")
+    time = if Version.create(Utils::Svn.version) >= Version.create("1.9")
       out, = system_command("svn", args: ["info", "--show-item", "last-changed-date"], chdir: cached_location)
       out
     else

Library/Homebrew/test/utils/svn_spec.rb

@@ -2,49 +2,48 @@
 
 require "utils/svn"
 
-describe Utils do
-  describe "#self.svn_available?" do
-    before do
-      described_class.clear_svn_version_cache
-    end
+describe Utils::Svn do
+  before do
+    described_class.clear_version_cache
+  end
 
+  describe "::available?" do
     it "returns svn version if svn available" do
       if quiet_system "#{HOMEBREW_SHIMS_PATH}/scm/svn", "--version"
-        expect(described_class).to be_svn_available
+        expect(described_class).to be_available
       else
-        expect(described_class).not_to be_svn_available
+        expect(described_class).not_to be_available
       end
     end
   end
 
-  describe "#self.svn_version" do
-    before do
-      described_class.clear_svn_version_cache
-    end
-
-    it "returns nil when svn is not available" do
-      allow(described_class).to receive(:svn_available?).and_return(false)
-      expect(described_class.svn_version).to eq(nil)
+  describe "::version" do
+    it "returns svn version if svn available" do
+      if quiet_system "#{HOMEBREW_SHIMS_PATH}/scm/svn", "--version"
+        expect(described_class.version).to match(/^\d+\.\d+\.\d+$/)
+      else
+        expect(described_class.version).to be_nil
+      end
     end
 
     it "returns version of svn when svn is available", :needs_svn do
-      expect(described_class.svn_version).not_to be_nil
+      expect(described_class.version).not_to be_nil
     end
   end
 
-  describe "#self.svn_remote_exists?" do
+  describe "::remote_exists?" do
     it "returns true when svn is not available" do
-      allow(described_class).to receive(:svn_available?).and_return(false)
-      expect(described_class).to be_svn_remote_exists("blah")
+      allow(described_class).to receive(:available?).and_return(false)
+      expect(described_class).to be_remote_exists("blah")
     end
 
     context "when svn is available" do
       before do
-        allow(described_class).to receive(:svn_available?).and_return(true)
+        allow(described_class).to receive(:available?).and_return(true)
       end
 
       it "returns false when remote does not exist" do
-        expect(described_class).not_to be_svn_remote_exists(HOMEBREW_CACHE/"install")
+        expect(described_class).not_to be_remote_exists(HOMEBREW_CACHE/"install")
       end
 
       it "returns true when remote exists", :needs_network, :needs_svn do
@@ -54,7 +53,7 @@ describe Utils do
                  "https://github.com/Homebrew/install"
         end
 
-        expect(described_class).to be_svn_remote_exists(HOMEBREW_CACHE/"install")
+        expect(described_class).to be_remote_exists(HOMEBREW_CACHE/"install")
       end
     end
   end

Library/Homebrew/utils/svn.rb

@@ -1,32 +1,36 @@
 # frozen_string_literal: true
 
+require "system_command"
+
 module Utils
-  def self.clear_svn_version_cache
-    remove_instance_variable(:@svn_available) if defined?(@svn_available)
-    remove_instance_variable(:@svn_version) if defined?(@svn_version)
-  end
+  # Helper functions for querying SVN information.
+  #
+  # @api private
+  module Svn
+    module_function
 
-  def self.svn_available?
-    return @svn_available if defined?(@svn_available)
+    def available?
+      version.present?
+    end
 
-    @svn_available = quiet_system HOMEBREW_SHIMS_PATH/"scm/svn", "--version"
-  end
+    def version
+      return @version if defined?(@version)
 
-  def self.svn_version
-    return unless svn_available?
-    return @svn_version if defined?(@svn_version)
+      stdout, _, status = system_command(HOMEBREW_SHIMS_PATH/"scm/svn", args: ["--version"], print_stderr: false)
+      @version = status.success? ? stdout.chomp[/svn, version (\d+(?:\.\d+)*)/, 1] : nil
+    end
 
-    @svn_version = Utils.popen_read(
-      HOMEBREW_SHIMS_PATH/"scm/svn", "--version"
-    ).chomp[/svn, version (\d+(?:\.\d+)*)/, 1]
-  end
+    def remote_exists?(url)
+      return true unless available?
 
-  def self.svn_remote_exists?(url)
-    return true unless svn_available?
+      # OK to unconditionally trust here because we're just checking if
+      # a URL exists.
+      quiet_system "svn", "ls", url, "--depth", "empty",
+                   "--non-interactive", "--trust-server-cert"
+    end
 
-    # OK to unconditionally trust here because we're just checking if
-    # a URL exists.
-    quiet_system "svn", "ls", url, "--depth", "empty",
-                 "--non-interactive", "--trust-server-cert"
+    def clear_version_cache
+      remove_instance_variable(:@version) if defined?(@version)
+    end
   end
 end