Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUGFIX] Fix Danger::GitRepo::find_merge_base_with_incremental_fetch return value #1498

Conversation

nagataaaas
Copy link
Contributor

Looks like there's a bug.
In the method Danger::GitRepo::find_merge_base_with_incremental_fetch which supposed to return string(possible_merge_base), we return boolean(possible_merge_base is found).

This causes an error that passes string true or false as a branch to git.

$ bundle exec danger --verbose --base={base} --head={head} --fail-on-errors=true
  
bundler: failed to load command: danger (repo/vendor/bundle/ruby/3.2.0/bin/danger)
repo/vendor/bundle/ruby/3.2.0/gems/git-1.19.1/lib/git/lib.rb:[12]({job_id}?pr={pr_number}):in `command':  (Danger::DSLError)
[!] Invalid `Dangerfile` file: git '--git-dir=repo/.git' '--work-tree=repo' '-c' 'core.quotePath=true' '-c' 'color.ui=false' 'diff' '-p' 'true' '{head}'  2>&1
status: pid 383 exit 128
output: "fatal: ambiguous argument 'true': unknown revision or path not in the working tree.\nUse '--' to separate paths from revisions, like this:\n'git <command> [<revision>...] -- [<file>...]'\n"
 #  from Dangerfile:[13]
 #  -------------------------------------------
 #  
 >  active_files = (git.modified_files + git.added_files).uniq
 #  
 #  -------------------------------------------

	from repo/vendor/bundle/ruby/3.2.0/gems/git-1.19.1/lib/git/lib.rb:492:in `diff_full'
	from repo/vendor/bundle/ruby/3.2.0/gems/git-1.19.1/lib/git/diff.rb:104:in `cache_full'
	from repo/vendor/bundle/ruby/3.2.0/gems/git-1.19.1/lib/git/diff.rb:109:in `process_full'
	from repo/vendor/bundle/ruby/3.2.0/gems/git-1.19.1/lib/git/diff.rb:68:in `each'
	from repo/vendor/bundle/ruby/3.2.0/gems/danger-9.5.0/lib/danger/danger_core/plugins/dangerfile_git_plugin.rb:75:in `select'
	from repo/vendor/bundle/ruby/3.2.0/gems/danger-9.5.0/lib/danger/danger_core/plugins/dangerfile_git_plugin.rb:75:in `modified_files'
	from Dangerfile:n:in `eval_file'

@nagataaaas nagataaaas changed the title Fix Danger::GitRepo::find_merge_base_with_incremental_fetch return value [BUGFIX] Fix Danger::GitRepo::find_merge_base_with_incremental_fetch return value Sep 16, 2024
Copy link
Member

@manicmaniac manicmaniac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nagataaaas
Thank you for your contribution 🥇

The purpose of this PR seems reasonable to me, but there appears to be a minor bug. Could you please fix it?

@manicmaniac
Copy link
Member

manicmaniac commented Sep 26, 2024

📝 I wrote a couple of small and ad-hoc test cases to check this PR. the latter one returns nil when no possible merge base is found failed with the current implementation.

diff --git a/spec/lib/danger/scm_source/git_repo_spec.rb b/spec/lib/danger/scm_source/git_repo_spec.rb
index 6509d27a..689b0461 100644
--- a/spec/lib/danger/scm_source/git_repo_spec.rb
+++ b/spec/lib/danger/scm_source/git_repo_spec.rb
@@ -269,4 +269,41 @@ RSpec.describe Danger::GitRepo, host: :github do
       end
     end
   end
+
+  describe "#find_merge_base_with_incremental_fetch" do
+    it "returns SHA1 hash of the possible merge base if any" do
+      Dir.mktmpdir do |dir|
+        Dir.chdir(dir) do
+          `git clone --depth=1 --single-branch --branch=master git@github.com:danger/danger.git`
+          repo_path = File.join(dir, "danger")
+          Dir.chdir(repo_path) do
+            @dm = testing_dangerfile
+            repo = Git.open(repo_path)
+            # https://github.com/danger/danger/commit/16ebebd8045667dbd034c106d4a4deb5dbde1cba
+            from = "16ebebd8045667dbd034c106d4a4deb5dbde1cba"
+            to = "master"
+            possible_merge_base = @dm.env.scm.send(:find_merge_base_with_incremental_fetch, repo, from, to)
+            expect(possible_merge_base).to match /[0-9a-f]{40}/
+          end
+        end
+      end
+    end
+
+    it "returns nil when no possible merge base is found" do
+      Dir.mktmpdir do |dir|
+        Dir.chdir(dir) do
+          `git clone --depth=1 --single-branch --branch=master git@github.com:danger/danger.git`
+          repo_path = File.join(dir, "danger")
+          Dir.chdir(repo_path) do
+            @dm = testing_dangerfile
+            repo = Git.open(repo_path)
+            from = "0000000000000000000000000000000000000000"
+            to = "master"
+            possible_merge_base = @dm.env.scm.send(:find_merge_base_with_incremental_fetch, repo, from, to)
+            expect(possible_merge_base).to be_nil
+          end
+        end
+      end
+    end
+  end
 end

⚠️ Do not merge this patch. The test is very slow and hard-coded commit hash cannot be valid into the future.

Copy link
Member

@manicmaniac manicmaniac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, Thank you @nagataaaas 👍

@manicmaniac manicmaniac merged commit 16eca2e into danger:master Sep 27, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants