Repository: reviewdog/reviewdog
Pull request #1722 (2024-04-25 15:21:23)

Add a fallback to git diff command to get diff of a GitHub pull request on doghouse
- [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#issuecomment-2074711580

Similar to https://github.com/reviewdog/reviewdog/pull/1697 and https://github.com/reviewdog/reviewdog/pull/1714, I add a fallback to `git diff` command to get diff of a GitHub pull request on doghouse.

https://github.com/reviewdog/reviewdog/blob/8e405abb08572c6336a1bd6800c1bf68953c633f/cmd/reviewdog/main.go#L302-L307
This will enable fallback even if the reporter is `github-check`, `github-pr-check` or `github-pr-annotations`.

Note that this only runs if `git` command is installed.


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 6c2580e9..6197e1f2 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -12,7 +12,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - ...
 
 ### :bug: Fixes
-- ...
+- [#1722](https://github.com/reviewdog/reviewdog/pull/1722) Add a fallback to `git diff` command to get diff for `github-check` / `github-pr-check` reporters with GitHub Actions (in other words, when not in DogHouse server).
 
 ### :rotating_light: Breaking changes
 - ...
diff --git a/doghouse/appengine/checker.go b/doghouse/appengine/checker.go
index 7a18e3bd..a3926b61 100644
--- a/doghouse/appengine/checker.go
+++ b/doghouse/appengine/checker.go
@@ -59,7 +59,7 @@ func (gc *githubChecker) handleCheck(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	res, err := server.NewChecker(&req, gh).Check(ctx, true)
+	res, err := server.NewChecker(&req, gh, true /* inDogHouseServer */).Check(ctx, true)
 	if err != nil {
 		aelog.Errorf(ctx, "failed to run checker: %v", err)
 		w.WriteHeader(http.StatusBadRequest)
diff --git a/doghouse/client/github_client.go b/doghouse/client/github_client.go
index 50cbab2d..2a9b80d1 100644
--- a/doghouse/client/github_client.go
+++ b/doghouse/client/github_client.go
@@ -16,5 +16,5 @@ type GitHubClient struct {
 }
 
 func (c *GitHubClient) Check(ctx context.Context, req *doghouse.CheckRequest, makeRequest bool) (*doghouse.CheckResponse, error) {
-	return server.NewChecker(req, c.Client).Check(ctx, makeRequest)
+	return server.NewChecker(req, c.Client, false /* inDogHouseServer */).Check(ctx, makeRequest)
 }
diff --git a/doghouse/server/doghouse.go b/doghouse/server/doghouse.go
index 596607fc..693604ff 100644
--- a/doghouse/server/doghouse.go
+++ b/doghouse/server/doghouse.go
@@ -32,12 +32,17 @@ const maxAllowedSize = 65535
 const maxAnnotationsPerRequest = 50
 
 type Checker struct {
-	req *doghouse.CheckRequest
-	gh  checkerGitHubClientInterface
+	req              *doghouse.CheckRequest
+	gh               checkerGitHubClientInterface
+	inDogHouseServer bool // If true, this checker runs in the DogHouse server.
 }
 
-func NewChecker(req *doghouse.CheckRequest, gh *github.Client) *Checker {
-	return &Checker{req: req, gh: &checkerGitHubClient{Client: gh}}
+func NewChecker(req *doghouse.CheckRequest, gh *github.Client, inDogHouseServer bool) *Checker {
+	return &Checker{
+		req:              req,
+		gh:               &checkerGitHubClient{Client: gh},
+		inDogHouseServer: inDogHouseServer,
+	}
 }
 
 func (ch *Checker) Check(ctx context.Context, makeRequest bool) (*doghouse.CheckResponse, error) {
@@ -350,7 +355,8 @@ func (ch *Checker) pullRequestDiff(ctx context.Context, pr int) ([]*diff.FileDif
 }
 
 func (ch *Checker) rawPullRequestDiff(ctx context.Context, pr int) ([]byte, error) {
-	d, err := ch.gh.GetPullRequestDiff(ctx, ch.req.Owner, ch.req.Repo, pr)
+	fallbackToGitCLI := !ch.inDogHouseServer
+	d, err := ch.gh.GetPullRequestDiff(ctx, ch.req.Owner, ch.req.Repo, pr, fallbackToGitCLI)
 	if err != nil {
 		return nil, err
 	}
diff --git a/doghouse/server/github_checker.go b/doghouse/server/github_checker.go
index 3051f0ff..82b7b6af 100644
--- a/doghouse/server/github_checker.go
+++ b/doghouse/server/github_checker.go
@@ -2,15 +2,21 @@ package server
 
 import (
 	"context"
+	"fmt"
 	"io"
+	"log"
+	"net/http"
+	"os"
+	"os/exec"
 	"time"
 
 	"github.com/google/go-github/v60/github"
+	"github.com/reviewdog/reviewdog/service/serviceutil"
 	"github.com/vvakame/sdlog/aelog"
 )
 
 type checkerGitHubClientInterface interface {
-	GetPullRequestDiff(ctx context.Context, owner, repo string, number int) ([]byte, error)
+	GetPullRequestDiff(ctx context.Context, owner, repo string, number int, fallbackToGitCLI bool) ([]byte, error)
 	CreateCheckRun(ctx context.Context, owner, repo string, opt github.CreateCheckRunOptions) (*github.CheckRun, error)
 	UpdateCheckRun(ctx context.Context, owner, repo string, checkID int64, opt github.UpdateCheckRunOptions) (*github.CheckRun, error)
 }
@@ -19,12 +25,54 @@ type checkerGitHubClient struct {
 	*github.Client
 }
 
-func (c *checkerGitHubClient) GetPullRequestDiff(ctx context.Context, owner, repo string, number int) ([]byte, error) {
+func (c *checkerGitHubClient) GetPullRequestDiff(ctx context.Context, owner, repo string, number int, fallbackToGitCLI bool) ([]byte, error) {
 	opt := github.RawOptions{Type: github.Diff}
-	d, _, err := c.PullRequests.GetRaw(ctx, owner, repo, number, opt)
+	d, resp, err := c.PullRequests.GetRaw(ctx, owner, repo, number, opt)
+	if err != nil {
+		if fallbackToGitCLI && resp != nil && resp.StatusCode == http.StatusNotAcceptable && serviceutil.GitCommandExists() {
+			log.Print("fallback to use git command")
+			return c.getPullRequestDiffUsingGitCommand(ctx, owner, repo, number)
+		}
+
+		return nil, err
+	}
 	return []byte(d), err
 }
 
+// getPullRequestDiffUsingGitCommand returns a diff of PullRequest using git command.
+func (c *checkerGitHubClient) getPullRequestDiffUsingGitCommand(ctx context.Context, owner, repo string, number int) ([]byte, error) {
+	pr, _, err := c.PullRequests.Get(ctx, owner, repo, number)
+	if err != nil {
+		return nil, err
+	}
+
+	head := pr.GetHead()
+	headSha := head.GetSHA()
+
+	commitsComparison, _, err := c.Repositories.CompareCommits(ctx, owner, repo, headSha, pr.GetBase().GetSHA(), nil)
+	if err != nil {
+		return nil, err
+	}
+
+	mergeBaseSha := commitsComparison.GetMergeBaseCommit().GetSHA()
+
+	if os.Getenv("REVIEWDOG_SKIP_GIT_FETCH") != "true" {
+		for _, sha := range []string{mergeBaseSha, headSha} {
+			_, err := exec.Command("git", "fetch", "--depth=1", head.GetRepo().GetHTMLURL(), sha).CombinedOutput()
+			if err != nil {
+				return nil, fmt.Errorf("failed to run git fetch: %w", err)
+			}
+		}
+	}
+
+	bytes, err := exec.Command("git", "diff", "--find-renames", mergeBaseSha, headSha).CombinedOutput()
+	if err != nil {
+		return nil, fmt.Errorf("failed to run git diff: %w", err)
+	}
+
+	return bytes, nil
+}
+
 func (c *checkerGitHubClient) CreateCheckRun(ctx context.Context, owner, repo string, opt github.CreateCheckRunOptions) (*github.CheckRun, error) {
 	checkRun, _, err := c.Checks.CreateCheckRun(ctx, owner, repo, opt)
 	return checkRun, err
diff --git a/service/github/github.go b/service/github/github.go
index 00857a2b..e748b0ca 100644
--- a/service/github/github.go
+++ b/service/github/github.go
@@ -305,6 +305,7 @@ func (g *PullRequest) Diff(ctx context.Context) ([]byte, error) {
 	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 {
+			// git command should exist here. See NewGitHubPullRequest.
 			log.Print("fallback to use git command")
 			return g.diffUsingGitCommand(ctx)
 		}
diff --git a/service/serviceutil/serviceutil.go b/service/serviceutil/serviceutil.go
index 320b664d..27d8b165 100644
--- a/service/serviceutil/serviceutil.go
+++ b/service/serviceutil/serviceutil.go
@@ -3,6 +3,7 @@ package serviceutil
 import (
 	"fmt"
 	"os"
+	"os/exec"
 	"path/filepath"
 	"strings"
 )
@@ -41,6 +42,12 @@ func GetGitRoot() (string, error) {
 	return findGitRoot(cwd)
 }
 
+// GitCommandExists checks if git command is installed.
+func GitCommandExists() bool {
+	_, err := exec.Command("git", "-v").CombinedOutput()
+	return err == nil
+}
+
 func findGitRoot(path string) (string, error) {
 	gitPath, err := findDotGitPath(path)
 	if err != nil {


Test patch:
diff --git a/doghouse/server/doghouse_test.go b/doghouse/server/doghouse_test.go
index eb14db47..967ec63f 100644
--- a/doghouse/server/doghouse_test.go
+++ b/doghouse/server/doghouse_test.go
@@ -17,13 +17,13 @@ import (
 )
 
 type fakeCheckerGitHubCli struct {
-	FakeGetPullRequestDiff func(ctx context.Context, owner, repo string, number int) ([]byte, error)
+	FakeGetPullRequestDiff func(ctx context.Context, owner, repo string, number int, fallbackToGitCLI bool) ([]byte, error)
 	FakeCreateCheckRun     func(ctx context.Context, owner, repo string, opt github.CreateCheckRunOptions) (*github.CheckRun, error)
 	FakeUpdateCheckRun     func(ctx context.Context, owner, repo string, checkID int64, opt github.UpdateCheckRunOptions) (*github.CheckRun, error)
 }
 
-func (f *fakeCheckerGitHubCli) GetPullRequestDiff(ctx context.Context, owner, repo string, number int) ([]byte, error) {
-	return f.FakeGetPullRequestDiff(ctx, owner, repo, number)
+func (f *fakeCheckerGitHubCli) GetPullRequestDiff(ctx context.Context, owner, repo string, number int, fallbackToGitCLI bool) ([]byte, error) {
+	return f.FakeGetPullRequestDiff(ctx, owner, repo, number, fallbackToGitCLI)
 }
 
 func (f *fakeCheckerGitHubCli) CreateCheckRun(ctx context.Context, owner, repo string, opt github.CreateCheckRunOptions) (*github.CheckRun, error) {
@@ -191,7 +191,7 @@ func TestCheck_OK(t *testing.T) {
 	}
 
 	cli := &fakeCheckerGitHubCli{}
-	cli.FakeGetPullRequestDiff = func(ctx context.Context, owner, repo string, number int) ([]byte, error) {
+	cli.FakeGetPullRequestDiff = func(ctx context.Context, owner, repo string, number int, fallbackToGitCLI bool) ([]byte, error) {
 		return []byte(sampleDiff), nil
 	}
 	cli.FakeCreateCheckRun = func(ctx context.Context, owner, repo string, opt github.CreateCheckRunOptions) (*github.CheckRun, error) {
@@ -361,7 +361,7 @@ func testOutsideDiff(t *testing.T, outsideDiff bool, filterMode filter.Mode) {
 	}
 
 	cli := &fakeCheckerGitHubCli{}
-	cli.FakeGetPullRequestDiff = func(ctx context.Context, owner, repo string, number int) ([]byte, error) {
+	cli.FakeGetPullRequestDiff = func(ctx context.Context, owner, repo string, number int, fallbackToGitCLI bool) ([]byte, error) {
 		return []byte(sampleDiff), nil
 	}
 	cli.FakeCreateCheckRun = func(ctx context.Context, owner, repo string, opt github.CreateCheckRunOptions) (*github.CheckRun, error) {
@@ -451,7 +451,7 @@ func TestCheck_OK_multiple_update_runs(t *testing.T) {
 	}
 
 	cli := &fakeCheckerGitHubCli{}
-	cli.FakeGetPullRequestDiff = func(ctx context.Context, owner, repo string, number int) ([]byte, error) {
+	cli.FakeGetPullRequestDiff = func(ctx context.Context, owner, repo string, number int, fallbackToGitCLI bool) ([]byte, error) {
 		return []byte(sampleDiff), nil
 	}
 	cli.FakeCreateCheckRun = func(ctx context.Context, owner, repo string, opt github.CreateCheckRunOptions) (*github.CheckRun, error) {
@@ -532,7 +532,7 @@ func TestCheck_OK_nonPullRequests(t *testing.T) {
 	}
 
 	cli := &fakeCheckerGitHubCli{}
-	cli.FakeGetPullRequestDiff = func(ctx context.Context, owner, repo string, number int) ([]byte, error) {
+	cli.FakeGetPullRequestDiff = func(ctx context.Context, owner, repo string, number int, fallbackToGitCLI bool) ([]byte, error) {
 		t.Errorf("GetPullRequestDiff should not be called")
 		return nil, nil
 	}
@@ -589,7 +589,7 @@ func TestCheck_OK_nonPullRequests(t *testing.T) {
 func TestCheck_fail_diff(t *testing.T) {
 	req := &doghouse.CheckRequest{PullRequest: 1}
 	cli := &fakeCheckerGitHubCli{}
-	cli.FakeGetPullRequestDiff = func(ctx context.Context, owner, repo string, number int) ([]byte, error) {
+	cli.FakeGetPullRequestDiff = func(ctx context.Context, owner, repo string, number int, fallbackToGitCLI bool) ([]byte, error) {
 		return nil, errors.New("test diff failure")
 	}
 	cli.FakeCreateCheckRun = func(ctx context.Context, owner, repo string, opt github.CreateCheckRunOptions) (*github.CheckRun, error) {
@@ -608,7 +608,7 @@ func TestCheck_fail_invalid_diff(t *testing.T) {
 	t.Skip("Parse invalid diff function somehow doesn't return error")
 	req := &doghouse.CheckRequest{PullRequest: 1}
 	cli := &fakeCheckerGitHubCli{}
-	cli.FakeGetPullRequestDiff = func(ctx context.Context, owner, repo string, number int) ([]byte, error) {
+	cli.FakeGetPullRequestDiff = func(ctx context.Context, owner, repo string, number int, fallbackToGitCLI bool) ([]byte, error) {
 		return []byte("invalid diff"), nil
 	}
 	cli.FakeCreateCheckRun = func(ctx context.Context, owner, repo string, opt github.CreateCheckRunOptions) (*github.CheckRun, error) {
@@ -626,7 +626,7 @@ func TestCheck_fail_invalid_diff(t *testing.T) {
 func TestCheck_fail_check(t *testing.T) {
 	req := &doghouse.CheckRequest{PullRequest: 1}
 	cli := &fakeCheckerGitHubCli{}
-	cli.FakeGetPullRequestDiff = func(ctx context.Context, owner, repo string, number int) ([]byte, error) {
+	cli.FakeGetPullRequestDiff = func(ctx context.Context, owner, repo string, number int, fallbackToGitCLI bool) ([]byte, error) {
 		return []byte(sampleDiff), nil
 	}
 	cli.FakeCreateCheckRun = func(ctx context.Context, owner, repo string, opt github.CreateCheckRunOptions) (*github.CheckRun, error) {
@@ -644,7 +644,7 @@ func TestCheck_fail_check(t *testing.T) {
 func TestCheck_fail_check_with_403(t *testing.T) {
 	req := &doghouse.CheckRequest{PullRequest: 1}
 	cli := &fakeCheckerGitHubCli{}
-	cli.FakeGetPullRequestDiff = func(ctx context.Context, owner, repo string, number int) ([]byte, error) {
+	cli.FakeGetPullRequestDiff = func(ctx context.Context, owner, repo string, number int, fallbackToGitCLI bool) ([]byte, error) {
 		return []byte(sampleDiff), nil
 	}
 	cli.FakeCreateCheckRun = func(ctx context.Context, owner, repo string, opt github.CreateCheckRunOptions) (*github.CheckRun, error) {
diff --git a/doghouse/server/github_checker_test.go b/doghouse/server/github_checker_test.go
new file mode 100644
index 00000000..d95cd5d2
--- /dev/null
+++ b/doghouse/server/github_checker_test.go
@@ -0,0 +1,169 @@
+package server
+
+import (
+	"context"
+	"encoding/json"
+	"net/http"
+	"net/http/httptest"
+	"net/url"
+	"os"
+	"strings"
+	"testing"
+
+	"github.com/google/go-github/v60/github"
+	"github.com/reviewdog/reviewdog/doghouse"
+	"golang.org/x/oauth2"
+)
+
+const notokenSkipTestMes = "skipping test (requires actual Personal access tokens. export REVIEWDOG_TEST_GITHUB_API_TOKEN=<GitHub Personal Access Token>)"
+
+func setupGitHubClient() *github.Client {
+	token := os.Getenv("REVIEWDOG_TEST_GITHUB_API_TOKEN")
+	if token == "" {
+		return nil
+	}
+	ts := oauth2.StaticTokenSource(
+		&oauth2.Token{AccessToken: token},
+	)
+	tc := oauth2.NewClient(context.TODO(), ts)
+	return github.NewClient(tc)
+}
+
+func TestChecker_GetPullRequestDiff(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skipping test which contains actual API requests in short mode")
+	}
+	client := setupGitHubClient()
+	if client == nil {
+		t.Skip(notokenSkipTestMes)
+	}
+
+	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%
+ 
+-comment:
+-  layout: "header"
++comment: false
+`
+
+	// https://github.com/reviewdog/reviewdog/pull/73
+	owner := "haya14busa"
+	repo := "reviewdog"
+	pr := 73
+	b, err := NewChecker(&doghouse.CheckRequest{}, client, false /* inDogHouseServer */).gh.GetPullRequestDiff(context.Background(), owner, repo, pr, false /* fallbackToGitCLI */)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if got := string(b); got != want {
+		t.Errorf("got:\n%v\nwant:\n%v", got, want)
+	}
+}
+
+func TestChecker_GetPullRequestDiff_fake(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") {
+			t.Errorf("Accept header doesn't contain 'diff': %v", accept)
+		}
+		w.Write([]byte("Pull Request diff"))
+	})
+	ts := httptest.NewServer(mux)
+	defer ts.Close()
+
+	cli := github.NewClient(nil)
+	cli.BaseURL, _ = url.Parse(ts.URL + "/")
+
+	gh := NewChecker(&doghouse.CheckRequest{}, cli, false /* inDoghouseServer */).gh
+
+	if _, err := gh.GetPullRequestDiff(context.Background(), "o", "r", 14, false /* fallbackToGitCli */); err != nil {
+		t.Fatal(err)
+	}
+	if apiCalled != 1 {
+		t.Errorf("GitHub API should be called once; called %v times", apiCalled)
+	}
+}
+
+func TestChecker_GetPullRequestDiff_fake_fallback(t *testing.T) {
+	apiCalled := 0
+	mux := http.NewServeMux()
+	headSHA := "HEAD^"
+	baseSHA := "HEAD"
+	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)
+		}
+
+		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)
+		}
+	})
+	mux.HandleFunc("/repos/o/r/compare/"+headSHA+"..."+baseSHA, func(w http.ResponseWriter, r *http.Request) {
+		if r.Method != http.MethodGet {
+			t.Errorf("unexpected access: %v %v", r.Method, r.URL)
+		}
+		if accept := r.Header.Get("Accept"); accept != "application/vnd.github.v3+json" {
+			t.Errorf("Accept header doesn't contain 'diff': %v", accept)
+		}
+
+		mergeBaseSha := "HEAD^"
+
+		commitsComparisonJSON, err := json.Marshal(github.CommitsComparison{
+			MergeBaseCommit: &github.RepositoryCommit{
+				SHA: &mergeBaseSha,
+			},
+		})
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		if _, err := w.Write(commitsComparisonJSON); err != nil {
+			t.Fatal(err)
+		}
+	})
+	ts := httptest.NewServer(mux)
+	defer ts.Close()
+
+	t.Setenv("REVIEWDOG_SKIP_GIT_FETCH", "true")
+
+	cli := github.NewClient(nil)
+	cli.BaseURL, _ = url.Parse(ts.URL + "/")
+
+	gh := NewChecker(&doghouse.CheckRequest{}, cli, false /* inDogHouseServer */).gh
+
+	if _, err := gh.GetPullRequestDiff(context.Background(), "o", "r", 14, true /* fallbackToGitCli */); err != nil {
+		t.Fatal(err)
+	}
+	if apiCalled != 2 {
+		t.Errorf("GitHub API should be called twice; called %v times", apiCalled)
+	}
+}