Auto merge of #4811 - NickLaMuro:dots_for_compact_index_logger, r=segiddins

Fix random string printing inconsistencies and formatting

Add dot output for CompactIndex fetcher
---------------------------------------

The `Bundler::Fetcher::CompactIndex`'s logging was missing the "dot logging" when not in debug mode, which is an assumed behavior based on the code in `lib/bundler/source/rubygems.rb`:

```ruby
api_fetchers.each do |f|
  Bundler.ui.info "Fetching gem metadata from #{f.uri}", Bundler.ui.debug?
  idx.use f.specs_with_retry(dependency_names, self)
  Bundler.ui.info "" unless Bundler.ui.debug? # new line now that the dots are over
end
```

While this isn't critical for `bundler` function properly by any stretch of the imagination, it provides a small bit of user feedback that requests are still continuing to be processed and `bundler` hasn't stalled.  It also maintains logging consistency between the different fetcher models.

Add newlines for `Bundler::Retry`
---------------------------------

This is very pedantic, but it makes it so that retry warning messages that used to look like this:

```
Fetching gem metadata from https://rubygems.org/....Retrying fetcher due to error (2/4): Error::OMGWatHappened LOL, no idea
.....
```

Now will look like this:

```
Fetching gem metadata from https://rubygems.org/....
Retrying fetcher due to error (2/4): Error::OMGWatHappened LOL, no idea.....
```

I think this reads a bit better and puts context to the "dots" so they now match up with the original attempt and the retries

Remove unneeded if statement
----------------------------

Ran across this while failing at writing tests for the above (for longer than I want to admit in public), but basically the `if` statement to determine whether or not to print "name" in the `Bundler.ui.warn` was not needed, as it never could be reached because of a short circuiting return statement prior to the print

```ruby
return true unless name
```

Have no idea how to test it anymore than we already are, so instead of embarrassing myself further, I moved on.

6 files changed, 77 insertions, 14 deletions

diff --git a/lib/bundler/fetcher/base.rb b/lib/bundler/fetcher/base.rb
index 6714ea2cd7..271729a534 100644
--- a/lib/bundler/fetcher/base.rb
+++ b/lib/bundler/fetcher/base.rb
@@ -36,6 +36,16 @@ module Bundler
       def api_fetcher?
         false
       end
+
+    private
+
+      def log_specs(debug_msg)
+        if Bundler.ui.debug?
+          Bundler.ui.debug debug_msg
+        else
+          Bundler.ui.info ".", false
+        end
+      end
     end
   end
 end
diff --git a/lib/bundler/fetcher/compact_index.rb b/lib/bundler/fetcher/compact_index.rb
index c0e1b3b4cf..e6f936c2fe 100644
--- a/lib/bundler/fetcher/compact_index.rb
+++ b/lib/bundler/fetcher/compact_index.rb
@@ -36,7 +36,7 @@ module Bundler
         remaining_gems = gem_names.dup
 
         until remaining_gems.empty?
-          Bundler.ui.debug "Looking up gems #{remaining_gems.inspect}"
+          log_specs "Looking up gems #{remaining_gems.inspect}"
 
           deps = compact_index_client.dependencies(remaining_gems)
           next_gems = deps.map {|d| d[3].map(&:first).flatten(1) }.flatten(1).uniq
diff --git a/lib/bundler/fetcher/dependency.rb b/lib/bundler/fetcher/dependency.rb
index a145837a88..1cd5f9a213 100644
--- a/lib/bundler/fetcher/dependency.rb
+++ b/lib/bundler/fetcher/dependency.rb
@@ -23,7 +23,7 @@ module Bundler
       def specs(gem_names, full_dependency_list = [], last_spec_list = [])
         query_list = gem_names.uniq - full_dependency_list
 
-        log_specs(query_list)
+        log_specs "Query List: #{query_list.inspect}"
 
         return last_spec_list if query_list.empty?
 
@@ -76,17 +76,6 @@ module Bundler
         uri.query = "gems=#{CGI.escape(gem_names.join(","))}" if gem_names.any?
         uri
       end
-
-    private
-
-      def log_specs(query_list)
-        # only display the message on the first run
-        if Bundler.ui.debug?
-          Bundler.ui.debug "Query List: #{query_list.inspect}"
-        else
-          Bundler.ui.info ".", false
-        end
-      end
     end
   end
 end
diff --git a/lib/bundler/retry.rb b/lib/bundler/retry.rb
index bf4d6ab7f7..a7a72feed5 100644
--- a/lib/bundler/retry.rb
+++ b/lib/bundler/retry.rb
@@ -45,7 +45,8 @@ module Bundler
       @failed = true
       raise e if last_attempt? || @exceptions.any? {|k| e.is_a?(k) }
       return true unless name
-      Bundler.ui.warn "Retrying#{" #{name}" if name} due to error (#{current_run.next}/#{total_runs}): #{e.class} #{e.message}"
+      Bundler.ui.info "" unless Bundler.ui.debug? # Add new line incase dots preceded this
+      Bundler.ui.warn "Retrying #{name} due to error (#{current_run.next}/#{total_runs}): #{e.class} #{e.message}", Bundler.ui.debug?
     end
 
     def keep_trying?
diff --git a/spec/bundler/fetcher/compact_index_spec.rb b/spec/bundler/fetcher/compact_index_spec.rb
index e111d8a3b6..f6c6ba2ee1 100644
--- a/spec/bundler/fetcher/compact_index_spec.rb
+++ b/spec/bundler/fetcher/compact_index_spec.rb
@@ -7,6 +7,10 @@ describe Bundler::Fetcher::CompactIndex do
   let(:display_uri) { URI("http://sampleuri.com") }
   let(:compact_index) { described_class.new(downloader, remote, display_uri) }
 
+  before do
+    allow(compact_index).to receive(:log_specs) {}
+  end
+
   describe "#specs_for_names" do
     it "has only one thread open at the end of the run" do
       compact_index.specs_for_names(["lskdjf"])
@@ -20,5 +24,31 @@ describe Bundler::Fetcher::CompactIndex do
 
       compact_index.specs_for_names(["lskdjf"])
     end
+
+    context "logging" do
+      before { allow(compact_index).to receive(:log_specs).and_call_original }
+
+      context "with debug on" do
+        before do
+          allow(Bundler).to receive_message_chain(:ui, :debug?).and_return(true)
+        end
+
+        it "should log at info level" do
+          expect(Bundler).to receive_message_chain(:ui, :debug).with('Looking up gems ["lskdjf"]')
+          compact_index.specs_for_names(["lskdjf"])
+        end
+      end
+
+      context "with debug off" do
+        before do
+          allow(Bundler).to receive_message_chain(:ui, :debug?).and_return(false)
+        end
+
+        it "should log at info level" do
+          expect(Bundler).to receive_message_chain(:ui, :info).with(".", false)
+          compact_index.specs_for_names(["lskdjf"])
+        end
+      end
+    end
   end
 end
diff --git a/spec/bundler/retry_spec.rb b/spec/bundler/retry_spec.rb
index cafa099b51..665ba9f2df 100644
--- a/spec/bundler/retry_spec.rb
+++ b/spec/bundler/retry_spec.rb
@@ -46,4 +46,37 @@ describe Bundler::Retry do
     end.to raise_error(error)
     expect(attempts).to eq(1)
   end
+
+  context "logging" do
+    let(:error)           { Bundler::GemfileNotFound }
+    let(:failure_message) { "Retrying test due to error (2/2): #{error} #{error}" }
+
+    context "with debugging on" do
+      it "print error message with newline" do
+        allow(Bundler.ui).to receive(:debug?).and_return(true)
+        expect(Bundler.ui).to_not receive(:info)
+        expect(Bundler.ui).to receive(:warn).with(failure_message, true)
+
+        expect do
+          Bundler::Retry.new("test", [], 1).attempt do
+            raise error
+          end
+        end.to raise_error(error)
+      end
+    end
+
+    context "with debugging on" do
+      it "print error message with newlines" do
+        allow(Bundler.ui).to  receive(:debug?).and_return(false)
+        expect(Bundler.ui).to receive(:info).with("")
+        expect(Bundler.ui).to receive(:warn).with(failure_message, false)
+
+        expect do
+          Bundler::Retry.new("test", [], 1).attempt do
+            raise error
+          end
+        end.to raise_error(error)
+      end
+    end
+  end
 end