Finalise the workspace API to support store_change as a seperate call

common/lib/dependabot/workspace/base.rb

@@ -23,14 +23,11 @@ def failed_change_attempts
       end
 
       def change(memo = nil)
-        change_attempt = nil
         Dir.chdir(path) { yield(path) }
       rescue StandardError => e
-        change_attempt = capture_failed_change_attempt(memo, e)
+        capture_failed_change_attempt(memo, e)
         clean # clean up any failed changes
         raise e
-      ensure
-        change_attempts << change_attempt unless change_attempt.nil?
       end
 
       def store_change(memo = nil); end

common/lib/dependabot/workspace/git.rb

@@ -17,6 +17,10 @@ def initialize(path)
         configure_git
       end
 
+      def changed?
+        changes.any? || !changed_files.empty?
+      end
+
       def to_patch
         run_shell_command("git diff --patch #{@initial_head_sha}.. .")
       end
@@ -31,22 +35,19 @@ def reset!
       end
 
       def store_change(memo = nil)
-        changed_files = run_shell_command("git status --short .").strip
         return nil if changed_files.empty?
 
         sha, diff = commit(memo)
-        ChangeAttempt.new(self, id: sha, memo: memo, diff: diff)
+        change_attempts << ChangeAttempt.new(self, id: sha, memo: memo, diff: diff)
       end
 
       protected
 
       def capture_failed_change_attempt(memo = nil, error = nil)
-        changed_files =
-          run_shell_command("git status --untracked-files=all --ignored=matching --short .").strip
-        return nil if changed_files.nil? && error.nil?
+        return nil if changed_files(ignored_mode: "matching").empty? && error.nil?
 
         sha, diff = stash(memo)
-        ChangeAttempt.new(self, id: sha, memo: memo, diff: diff, error: error)
+        change_attempts << ChangeAttempt.new(self, id: sha, memo: memo, diff: diff, error: error)
       end
 
       private
@@ -64,6 +65,10 @@ def last_stash_sha
         run_shell_command("git rev-parse refs/stash").strip
       end
 
+      def changed_files(ignored_mode: "traditional")
+        run_shell_command("git status --untracked-files=all --ignored=#{ignored_mode} --short .").strip
+      end
+
       def stash(memo = nil)
         msg = memo || "workspace change attempt"
         run_shell_command("git add --all --force .")

common/spec/dependabot/workspace/git_spec.rb

@@ -37,28 +37,21 @@
     context "when there are changes" do
       before do
         workspace.change("rspec") { `echo 'gem "rspec", "~> 3.12.0", group: :test' >> Gemfile` }
+        workspace.store_change("rspec")
         workspace.change("timecop") { `echo 'gem "timecop", "~> 0.9.6", group: :test' >> Gemfile` }
+        workspace.store_change("rspec")
       end
 
-      # rubocop:disable Layout/TrailingWhitespace
       it "returns the diff of all changes" do
         expect(workspace.changes.size).to eq(2)
-        expect(workspace.to_patch).to eq(
+        expect(workspace.to_patch).to end_with(
           <<~DIFF
-            diff --git a/Gemfile b/Gemfile
-            index 0e6bb68..7a464a8 100644
-            --- a/Gemfile
-            +++ b/Gemfile
-            @@ -1,3 +1,5 @@
-             source "https://rubygems.org"
-             
              gem "activesupport", ">= 6.0.0"
             +gem "rspec", "~> 3.12.0", group: :test
             +gem "timecop", "~> 0.9.6", group: :test
           DIFF
         )
       end
-      # rubocop:enable Layout/TrailingWhitespace
 
       context "when there are failed change attempts" do
         before do
@@ -70,65 +63,34 @@
           # nop
         end
 
-        # rubocop:disable Layout/TrailingWhitespace
         it "returns the diff of all changes excluding failed change attempts" do
           expect(workspace.changes.size).to eq(2)
           expect(workspace.failed_change_attempts.size).to eq(1)
-          expect(workspace.to_patch).to eq(
+          expect(workspace.to_patch).to end_with(
             <<~DIFF
-              diff --git a/Gemfile b/Gemfile
-              index 0e6bb68..7a464a8 100644
-              --- a/Gemfile
-              +++ b/Gemfile
-              @@ -1,3 +1,5 @@
-               source "https://rubygems.org"
-               
                gem "activesupport", ">= 6.0.0"
               +gem "rspec", "~> 3.12.0", group: :test
               +gem "timecop", "~> 0.9.6", group: :test
             DIFF
           )
         end
-        # rubocop:enable Layout/TrailingWhitespace
       end
     end
   end
 
   describe "#change" do
     context "on success" do
-      # rubocop:disable Layout/TrailingWhitespace
       it "captures the change" do
         workspace.change("timecop") do
           `echo 'gem "timecop", "~> 0.9.6", group: :test' >> Gemfile`
         end
         expect(workspace.failed_change_attempts).to be_empty
-        expect(workspace.changes.size).to eq(1)
+        expect(workspace.changes.size).to eq(0)
         expect(workspace).to be_changed
-        expect(workspace.change_attempts.size).to eq(1)
-        expect(workspace.change_attempts.first.id).to eq(`git rev-parse HEAD`.strip)
-        expect(workspace.change_attempts.first.diff).to eq(
-          <<~DIFF
-            diff --git a/Gemfile b/Gemfile
-            index 0e6bb68..77d166d 100644
-            --- a/Gemfile
-            +++ b/Gemfile
-            @@ -1,3 +1,4 @@
-             source "https://rubygems.org"
-             
-             gem "activesupport", ">= 6.0.0"
-            +gem "timecop", "~> 0.9.6", group: :test
-          DIFF
-        )
-        expect(workspace.change_attempts.first.memo).to eq("timecop")
-        expect(workspace.change_attempts.first.error).to be_nil
-        expect(workspace.change_attempts.first.error?).to be_falsy
-        expect(workspace.change_attempts.first.success?).to be_truthy
       end
-      # rubocop:enable Layout/TrailingWhitespace
     end
 
     context "on error" do
-      # rubocop:disable Layout/TrailingWhitespace
       it "captures the failed change attempt" do
         expect do
           workspace.change("timecop") do
@@ -142,15 +104,8 @@
         expect(workspace.failed_change_attempts.size).to eq(1)
         expect(workspace.change_attempts.size).to eq(1)
         expect(workspace.change_attempts.first.id).to eq(`git rev-parse refs/stash`.strip)
-        expect(workspace.change_attempts.first.diff).to eq(
+        expect(workspace.change_attempts.first.diff).to end_with(
           <<~DIFF
-            diff --git a/Gemfile b/Gemfile
-            index 0e6bb68..77d166d 100644
-            --- a/Gemfile
-            +++ b/Gemfile
-            @@ -1,3 +1,4 @@
-             source "https://rubygems.org"
-             
              gem "activesupport", ">= 6.0.0"
             +gem "timecop", "~> 0.9.6", group: :test
           DIFF
@@ -161,9 +116,7 @@
         expect(workspace.change_attempts.first.error?).to be_truthy
         expect(workspace.change_attempts.first.success?).to be_falsy
       end
-      # rubocop:enable Layout/TrailingWhitespace
 
-      # rubocop:disable Layout/TrailingWhitespace
       context "when there are untracked/ignored files" do
         # See: common/spec/fixtures/projects/simple/.gitignore
 
@@ -180,43 +133,27 @@
 
         it "captures the untracked/ignored files" do
           expect(workspace.failed_change_attempts.size).to eq(1)
-          expect(workspace.failed_change_attempts.first.diff).to eq(
+          expect(workspace.failed_change_attempts.first.diff).to include(
             <<~DIFF
-              diff --git a/Gemfile b/Gemfile
-              index 0e6bb68..958f479 100644
-              --- a/Gemfile
-              +++ b/Gemfile
-              @@ -1,3 +1,4 @@
-               source "https://rubygems.org"
-               
                gem "activesupport", ">= 6.0.0"
               +gem "fail"
-              diff --git a/ignored-file.txt b/ignored-file.txt
-              new file mode 100644
-              index 0000000..85d9237
-              --- /dev/null
-              +++ b/ignored-file.txt
-              @@ -0,0 +1 @@
-              +ignored output
-              diff --git a/untracked-file.txt b/untracked-file.txt
-              new file mode 100644
-              index 0000000..b65afb0
-              --- /dev/null
-              +++ b/untracked-file.txt
-              @@ -0,0 +1 @@
-              +untracked output
             DIFF
           )
+          expect(workspace.failed_change_attempts.first.diff).to include(
+            "diff --git a/ignored-file.txt b/ignored-file.txt",
+            "diff --git a/untracked-file.txt b/untracked-file.txt"
+          )
         end
       end
-      # rubocop:enable Layout/TrailingWhitespace
     end
   end
 
   describe "#reset!" do
     it "clears change attempts, drops stashes, and resets HEAD to initial_head_sha" do
       workspace.change("rspec") { `echo 'gem "rspec", "~> 3.12.0", group: :test' >> Gemfile` }
+      workspace.store_change("rspec")
       workspace.change("timecop") { `echo 'gem "timecop", "~> 0.9.6", group: :test' >> Gemfile` }
+      workspace.store_change("timecop")
 
       begin
         workspace.change do
@@ -261,4 +198,39 @@
       expect(`git stash list | wc -l`.strip).to eq("0")
     end
   end
+
+  describe "#store_change" do
+    context "when there are no changes to store" do
+      it "returns nil and doesn't add any changes to the workspace" do
+        workspace.store_change("noop")
+
+        expect(workspace).not_to be_changed
+        expect(workspace.changes).to be_empty
+      end
+    end
+
+    context "when there are changes to store" do
+      it "captures the stores the changes correctly" do
+        workspace.change("timecop") do
+          `echo 'gem "timecop", "~> 0.9.6", group: :test' >> Gemfile`
+        end
+        workspace.store_change("Update timecop")
+        expect(workspace.failed_change_attempts).to be_empty
+        expect(workspace.changes.size).to eq(1)
+        expect(workspace).to be_changed
+        expect(workspace.change_attempts.size).to eq(1)
+        expect(workspace.change_attempts.first.id).to eq(`git rev-parse HEAD`.strip)
+        expect(workspace.change_attempts.first.diff).to end_with(
+          <<~DIFF
+             gem "activesupport", ">= 6.0.0"
+            +gem "timecop", "~> 0.9.6", group: :test
+          DIFF
+        )
+        expect(workspace.change_attempts.first.memo).to eq("Update timecop")
+        expect(workspace.change_attempts.first.error).to be_nil
+        expect(workspace.change_attempts.first.error?).to be_falsy
+        expect(workspace.change_attempts.first.success?).to be_truthy
+      end
+    end
+  end
 end