Auto merge of #6447 - agrim123:agr-update-error-message, r=segiddins

Update error message on bundle add in case of duplication

### What was the end-user problem that led to this PR?

If the user tries to add a gem giving version requirement then bundler throws an error if the gem is already present.

### What was your diagnosis of the problem?

The error displayed is very descriptive but if the user is specifying a gem version maybe he wants to update the gem.

### What is your fix for the problem, implemented in this PR?

Added an instructive message to inform the user that gem can also be updated.

### Why did you choose this fix out of the possible options?

This seemed to be the best informative message.

Closes #6341

lib/bundler/dsl.rb

@@ -107,13 +107,28 @@ def gem(name, *args)
         if current.requirement != dep.requirement
           unless deleted_dep
             return if dep.type == :development
+
+            update_prompt = ""
+
+            if File.basename(@gemfile) == Injector::INJECTED_GEMS
+              if dep.requirements_list.include?(">= 0") && !current.requirements_list.include?(">= 0")
+                update_prompt = ". Gem already added"
+              else
+                update_prompt = ". If you want to update the gem version, run `bundle update #{current.name}`"
+
+                update_prompt += ". You may also need to change the version requirement specified in the Gemfile if it's too restrictive." unless current.requirements_list.include?(">= 0")
+              end
+            end
+
             raise GemfileError, "You cannot specify the same gem twice with different version requirements.\n" \
-                            "You specified: #{current.name} (#{current.requirement}) and #{dep.name} (#{dep.requirement})"
+                            "You specified: #{current.name} (#{current.requirement}) and #{dep.name} (#{dep.requirement})" \
+                             "#{update_prompt}"
           end
 
         else
           Bundler.ui.warn "Your Gemfile lists the gem #{current.name} (#{current.requirement}) more than once.\n" \
                           "You should probably keep only one of them.\n" \
+                          "Remove any duplicate entries and specify the gem only once (per group).\n" \
                           "While it's not a problem now, it could cause errors if you change the version of one of them later."
         end
 

lib/bundler/injector.rb

@@ -2,8 +2,10 @@
 
 module Bundler
   class Injector
-    def self.inject(deps, options = {})
-      injector = new(deps, options)
+    INJECTED_GEMS = "injected gems".freeze
+
+    def self.inject(new_deps, options = {})
+      injector = new(new_deps, options)
       injector.inject(Bundler.default_gemfile, Bundler.default_lockfile)
     end
 
@@ -36,8 +38,9 @@ def inject(gemfile_path, lockfile_path)
         @deps -= builder.dependencies
 
         # add new deps to the end of the in-memory Gemfile
-        # Set conservative versioning to false because we want to let the resolver resolve the version first
-        builder.eval_gemfile("injected gems", build_gem_lines(false)) if @deps.any?
+        # Set conservative versioning to false because
+        # we want to let the resolver resolve the version first
+        builder.eval_gemfile(INJECTED_GEMS, build_gem_lines(false)) if @deps.any?
 
         # resolve to see if the new deps broke anything
         @definition = builder.to_definition(lockfile_path, {})

spec/commands/add_spec.rb

@@ -172,4 +172,46 @@
       expect(out).to include("You specified: weakling (~> 0.0.1) and weakling (>= 0).")
     end
   end
+
+  describe "when a gem is added which is already specified in Gemfile with version" do
+    it "shows an error when added with different version requirement" do
+      install_gemfile <<-G
+        source "file://#{gem_repo2}"
+        gem "rack", "1.0"
+      G
+
+      bundle "add 'rack' --version=1.1"
+
+      expect(out).to include("You cannot specify the same gem twice with different version requirements")
+      expect(out).to include("If you want to update the gem version, run `bundle update rack`. You may also need to change the version requirement specified in the Gemfile if it's too restrictive")
+    end
+
+    it "shows error when added without version requirements" do
+      install_gemfile <<-G
+        source "file://#{gem_repo2}"
+        gem "rack", "1.0"
+      G
+
+      bundle "add 'rack'"
+
+      expect(out).to include("Gem already added.")
+      expect(out).to include("You cannot specify the same gem twice with different version requirements")
+      expect(out).not_to include("If you want to update the gem version, run `bundle update rack`. You may also need to change the version requirement specified in the Gemfile if it's too restrictive")
+    end
+  end
+
+  describe "when a gem is added which is already specified in Gemfile without version" do
+    it "shows an error when added with different version requirement" do
+      install_gemfile <<-G
+        source "file://#{gem_repo2}"
+        gem "rack"
+      G
+
+      bundle "add 'rack' --version=1.1"
+
+      expect(out).to include("You cannot specify the same gem twice with different version requirements")
+      expect(out).to include("If you want to update the gem version, run `bundle update rack`.")
+      expect(out).not_to include("You may also need to change the version requirement specified in the Gemfile if it's too restrictive")
+    end
+  end
 end

spec/commands/install_spec.rb

@@ -321,6 +321,56 @@
       expect(File.exist?(bundled_app("Gemfile.lock"))).to eq(true)
     end
 
+    context "throws a warning if a gem is added twice in Gemfile" do
+      it "without version requirements" do
+        install_gemfile <<-G
+          source "file://#{gem_repo2}"
+          gem "rack"
+          gem "rack"
+        G
+
+        expect(out).to include("Your Gemfile lists the gem rack (>= 0) more than once.")
+        expect(out).to include("Remove any duplicate entries and specify the gem only once (per group).")
+        expect(out).to include("While it's not a problem now, it could cause errors if you change the version of one of them later.")
+      end
+
+      it "with same versions" do
+        install_gemfile <<-G
+          source "file://#{gem_repo2}"
+          gem "rack", "1.0"
+          gem "rack", "1.0"
+        G
+
+        expect(out).to include("Your Gemfile lists the gem rack (= 1.0) more than once.")
+        expect(out).to include("Remove any duplicate entries and specify the gem only once (per group).")
+        expect(out).to include("While it's not a problem now, it could cause errors if you change the version of one of them later.")
+      end
+    end
+
+    context "throws an error if a gem is added twice in Gemfile" do
+      it "when version of one dependency is not specified" do
+        install_gemfile <<-G
+          source "file://#{gem_repo2}"
+          gem "rack"
+          gem "rack", "1.0"
+        G
+
+        expect(out).to include("You cannot specify the same gem twice with different version requirements")
+        expect(out).to include("You specified: rack (>= 0) and rack (= 1.0).")
+      end
+
+      it "when different versions of both dependencies are specified" do
+        install_gemfile <<-G
+          source "file://#{gem_repo2}"
+          gem "rack", "1.0"
+          gem "rack", "1.1"
+        G
+
+        expect(out).to include("You cannot specify the same gem twice with different version requirements")
+        expect(out).to include("You specified: rack (= 1.0) and rack (= 1.1).")
+      end
+    end
+
     it "gracefully handles error when rubygems server is unavailable" do
       install_gemfile <<-G, :artifice => nil
         source "file://#{gem_repo1}"