Repository: reviewdog/reviewdog
Pull request #1697 (2024-03-31 10:48:26)

Add a fallback to git diff command to get diff of a GitHub pull request 
- [x] Updated Unreleased section in [CHANGELOG](https://github.com/reviewdog/reviewdog/blob/master/CHANGELOG.md) or it's not notable changes.

Fix https://github.com/reviewdog/reviewdog/issues/1696

Based on https://github.com/reviewdog/reviewdog/issues/1696#issuecomment-2028410272, I add a fallback to `git diff` command to get diff of a GitHub pull request.

atiqueakhtar: Hi @massongit ,

Thank you for investigating the fix for this issue. Are we on track to resolve the check failure `(reviewdog / reviewdog (github-check) (pull_request))` soon? We're eager to know if this can be addressed soon, as the resolution is critical for our pending PRs for which reviewdog is failing.

Appreciate your efforts on this.
massongit: I think it will take several days to weeks to resolve this issue.

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 0adc0a17..c74e6eda 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -13,6 +13,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### :bug: Fixes
 - Fix: no longer fails if it runs in GitHub Actions and does not use the annotation reporter
+- Add a fallback to `git diff` command to get diff of a GitHub pull request
 
 ### :rotating_light: Breaking changes
 - ...
diff --git a/service/github/github.go b/service/github/github.go
index b73511d1..40e05167 100644
--- a/service/github/github.go
+++ b/service/github/github.go
@@ -7,6 +7,7 @@ import (
 	"log"
 	"net/http"
 	"os"
+	"os/exec"
 	"path/filepath"
 	"strings"
 	"sync"
@@ -301,13 +302,42 @@ func (g *PullRequest) setPostedComment(ctx context.Context) error {
 // Diff returns a diff of PullRequest.
 func (g *PullRequest) Diff(ctx context.Context) ([]byte, error) {
 	opt := github.RawOptions{Type: github.Diff}
-	d, _, err := g.cli.PullRequests.GetRaw(ctx, g.owner, g.repo, g.pr, opt)
+	d, resp, err := g.cli.PullRequests.GetRaw(ctx, g.owner, g.repo, g.pr, opt)
 	if err != nil {
+		if resp != nil && resp.StatusCode == http.StatusNotAcceptable {
+			log.Print("fallback to use git command")
+			return g.diffUsingGitCommand(ctx)
+		}
+
 		return nil, err
 	}
 	return []byte(d), nil
 }
 
+// diffUsingGitCommand returns a diff of PullRequest using git command.
+func (g *PullRequest) diffUsingGitCommand(ctx context.Context) ([]byte, error) {
+	pr, _, err := g.cli.PullRequests.Get(ctx, g.owner, g.repo, g.pr)
+	if err != nil {
+		return nil, err
+	}
+
+	headSha := pr.GetHead().GetSHA()
+
+	mergeBaseBytes, err := exec.Command("git", "merge-base", headSha, pr.GetBase().GetSHA()).Output()
+	if err != nil {
+		return nil, fmt.Errorf("failed to get merge-base commit: %w", err)
+	}
+
+	mergeBase := strings.Trim(string(mergeBaseBytes), "\n")
+
+	bytes, err := exec.Command("git", "diff", "--find-renames", mergeBase, headSha).CombinedOutput()
+	if err != nil {
+		return nil, fmt.Errorf("failed to run git diff: %s%w", bytes, err)
+	}
+
+	return bytes, nil
+}
+
 // Strip returns 1 as a strip of git diff.
 func (g *PullRequest) Strip() int {
 	return 1


Test patch:
diff --git a/service/github/github_test.go b/service/github/github_test.go
index f359d40c..5e9fccb9 100644
--- a/service/github/github_test.go
+++ b/service/github/github_test.go
@@ -101,55 +101,23 @@ func TestGitHubPullRequest_Diff(t *testing.T) {
 		t.Skip(notokenSkipTestMes)
 	}
 
-	want := `diff --git a/diff.go b/diff.go
-index b380b67..6abc0f1 100644
---- a/diff.go
-+++ b/diff.go
-@@ -4,6 +4,9 @@ import (
- 	"os/exec"
- )
+	want := `diff --git a/.codecov.yml b/.codecov.yml
+index aa49124774..781ee2492f 100644
+--- a/.codecov.yml
++++ b/.codecov.yml
+@@ -7,5 +7,4 @@ coverage:
+       default:
+         target: 0%
  
-+func TestNewExportedFunc() {
-+}
-+
- var _ DiffService = &DiffString{}
- 
- type DiffString struct {
-diff --git a/reviewdog.go b/reviewdog.go
-index 61450f3..f63f149 100644
---- a/reviewdog.go
-+++ b/reviewdog.go
-@@ -10,18 +10,18 @@ import (
- 	"github.com/reviewdog/reviewdog/diff"
- )
- 
-+var TestExportedVarWithoutComment = 1
-+
-+func NewReviewdog(p Parser, c CommentService, d DiffService) *Reviewdog {
-+	return &Reviewdog{p: p, c: c, d: d}
-+}
-+
- type Reviewdog struct {
- 	p Parser
- 	c CommentService
- 	d DiffService
- }
- 
--func NewReviewdog(p Parser, c CommentService, d DiffService) *Reviewdog {
--	return &Reviewdog{p: p, c: c, d: d}
--}
--
--// CheckResult represents a checked result of static analysis tools.
--// :h error-file-format
- type CheckResult struct {
- 	Path    string   // file path
- 	Lnum    int      // line number
+-comment:
+-  layout: "header"
++comment: false
 `
 
-	// https://github.com/reviewdog/reviewdog/pull/2
+	// https://github.com/reviewdog/reviewdog/pull/73
 	owner := "haya14busa"
 	repo := "reviewdog"
-	pr := 2
+	pr := 73
 	g, err := NewGitHubPullRequest(client, owner, repo, pr, "", "warning")
 	if err != nil {
 		t.Fatal(err)
@@ -1334,6 +1302,58 @@ func TestGitHubPullRequest_Diff_fake(t *testing.T) {
 	}
 }
 
+func TestGitHubPullRequest_Diff_fake_fallback(t *testing.T) {
+	apiCalled := 0
+	mux := http.NewServeMux()
+	mux.HandleFunc("/repos/o/r/pulls/14", func(w http.ResponseWriter, r *http.Request) {
+		apiCalled++
+		if r.Method != http.MethodGet {
+			t.Errorf("unexpected access: %v %v", r.Method, r.URL)
+		}
+		if accept := r.Header.Get("Accept"); strings.Contains(accept, "diff") {
+			w.WriteHeader(http.StatusNotAcceptable)
+			return
+		}
+		if accept := r.Header.Get("Accept"); accept != "application/vnd.github.v3+json" {
+			t.Errorf("Accept header doesn't contain 'diff': %v", accept)
+		}
+
+		headSHA := "HEAD^"
+		baseSHA := "HEAD"
+
+		pullRequestJSON, err := json.Marshal(github.PullRequest{
+			Head: &github.PullRequestBranch{
+				SHA: &headSHA,
+			},
+			Base: &github.PullRequestBranch{
+				SHA: &baseSHA,
+			},
+		})
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		if _, err := w.Write(pullRequestJSON); err != nil {
+			t.Fatal(err)
+		}
+	})
+	ts := httptest.NewServer(mux)
+	defer ts.Close()
+
+	cli := github.NewClient(nil)
+	cli.BaseURL, _ = url.Parse(ts.URL + "/")
+	g, err := NewGitHubPullRequest(cli, "o", "r", 14, "sha", "warning")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if _, err := g.Diff(context.Background()); err != nil {
+		t.Fatal(err)
+	}
+	if apiCalled != 2 {
+		t.Errorf("GitHub API should be called twice; called %v times", apiCalled)
+	}
+}
+
 func TestGitHubPullRequest_Post_NoPermission(t *testing.T) {
 	cwd, _ := os.Getwd()
 	defer os.Chdir(cwd)