-
-
Notifications
You must be signed in to change notification settings - Fork 5.9k
Add .diff and .patch support to compare #34433
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
base: main
Are you sure you want to change the base?
Changes from 33 commits
8d799c2
f2a2acf
5cc1bda
54d37d1
ac25150
e11a339
104eecc
bcc4ade
a7aaa79
b46d314
4e2434b
7f72fe9
c6acfc1
016c2f3
a3c2953
4d7ea0d
bfa2d10
f86ddd5
acb57e5
180c1b0
2bcb546
a9c8ce9
17c860e
3274b84
d784a0f
c80f0b9
e9fac73
2905d35
688fc78
b9102f5
f4a0fe3
e442ab1
180386b
a8ef5d5
23e534d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -221,13 +221,9 @@ | |||
// base<-head: master...head:feature | ||||
// same repo: master...feature | ||||
|
||||
var ( | ||||
isSameRepo bool | ||||
infoPath string | ||||
err error | ||||
) | ||||
var isSameRepo bool | ||||
|
||||
infoPath = ctx.PathParam("*") | ||||
infoPath := ctx.PathParam("*") | ||||
var infos []string | ||||
if infoPath == "" { | ||||
infos = []string{baseRepo.DefaultBranch, baseRepo.DefaultBranch} | ||||
|
@@ -247,12 +243,14 @@ | |||
ci.BaseBranch = infos[0] | ||||
ctx.Data["BaseBranch"] = ci.BaseBranch | ||||
|
||||
// If there is no head repository, it means compare between same repository. | ||||
var err error | ||||
|
||||
// If there is no head repository, it means compare between the same repository. | ||||
headInfos := strings.Split(infos[1], ":") | ||||
if len(headInfos) == 1 { | ||||
isSameRepo = true | ||||
ci.HeadUser = ctx.Repo.Owner | ||||
ci.HeadBranch = headInfos[0] | ||||
ci.HeadBranch, ci.RawDiffType = parseRefForRawDiff(ctx, ctx.Repo.Repository, headInfos[0]) | ||||
} else if len(headInfos) == 2 { | ||||
headInfosSplit := strings.Split(headInfos[0], "/") | ||||
if len(headInfosSplit) == 1 { | ||||
|
@@ -265,20 +263,21 @@ | |||
} | ||||
return nil | ||||
} | ||||
ci.HeadBranch = headInfos[1] | ||||
// FIXME: how to correctly choose the head repository? The logic below (3-8) is quite complex, the real head repo is determined there | ||||
ci.HeadBranch, ci.RawDiffType = parseRefForRawDiff(ctx, ..., headInfos[1]) | ||||
Check failure on line 267 in routers/web/repo/compare.go
|
||||
isSameRepo = ci.HeadUser.ID == ctx.Repo.Owner.ID | ||||
if isSameRepo { | ||||
Check failure on line 269 in routers/web/repo/compare.go
|
||||
ci.HeadRepo = baseRepo | ||||
Check failure on line 270 in routers/web/repo/compare.go
|
||||
} | ||||
Check failure on line 271 in routers/web/repo/compare.go
|
||||
} else { | ||||
Check failure on line 272 in routers/web/repo/compare.go
|
||||
ci.HeadRepo, err = repo_model.GetRepositoryByOwnerAndName(ctx, headInfosSplit[0], headInfosSplit[1]) | ||||
if err != nil { | ||||
Check failure on line 274 in routers/web/repo/compare.go
|
||||
if repo_model.IsErrRepoNotExist(err) { | ||||
Check failure on line 275 in routers/web/repo/compare.go
|
||||
ctx.NotFound(nil) | ||||
} else { | ||||
ctx.ServerError("GetRepositoryByOwnerAndName", err) | ||||
} | ||||
return nil | ||||
} | ||||
if err := ci.HeadRepo.LoadOwner(ctx); err != nil { | ||||
if user_model.IsErrUserNotExist(err) { | ||||
|
@@ -288,7 +287,7 @@ | |||
} | ||||
return nil | ||||
} | ||||
ci.HeadBranch = headInfos[1] | ||||
ci.HeadBranch, ci.RawDiffType = parseRefForRawDiff(ctx, ci.HeadRepo, headInfos[1]) | ||||
ci.HeadUser = ci.HeadRepo.Owner | ||||
isSameRepo = ci.HeadRepo.ID == ctx.Repo.Repository.ID | ||||
} | ||||
|
@@ -729,6 +728,7 @@ | |||
return | ||||
} | ||||
|
||||
ctx.Data["PageIsCompareDiff"] = true | ||||
ctx.Data["PullRequestWorkInProgressPrefixes"] = setting.Repository.PullRequest.WorkInProgressPrefixes | ||||
ctx.Data["DirectComparison"] = ci.DirectComparison | ||||
ctx.Data["OtherCompareSeparator"] = ".." | ||||
|
@@ -743,6 +743,15 @@ | |||
return | ||||
} | ||||
|
||||
if ci.RawDiffType != "" { | ||||
err := git.GetRepoRawDiffForFile(ci.HeadGitRepo, ci.BaseBranch, ci.HeadBranch, ci.RawDiffType, "", ctx.Resp) | ||||
if err != nil { | ||||
ctx.ServerError("GetRepoRawDiffForFile", err) | ||||
return | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||
} | ||||
return | ||||
} | ||||
|
||||
baseTags, err := repo_model.GetTagNamesByRepoID(ctx, ctx.Repo.Repository.ID) | ||||
if err != nil { | ||||
ctx.ServerError("GetTagNamesByRepoID", err) | ||||
|
@@ -984,3 +993,20 @@ | |||
} | ||||
return diffLines, nil | ||||
} | ||||
|
||||
func parseRefForRawDiff(ctx *context.Context, refRepo *repo_model.Repository, refShortName string) (string, git.RawDiffType) { | ||||
if !strings.HasSuffix(refShortName, ".diff") && !strings.HasSuffix(refShortName, ".patch") { | ||||
return refShortName, "" | ||||
} | ||||
|
||||
if gitrepo.IsBranchExist(ctx, refRepo, refShortName) || gitrepo.IsTagExist(ctx, refRepo, refShortName) { | ||||
return refShortName, "" | ||||
} | ||||
|
||||
if s, ok := strings.CutSuffix(refShortName, ".diff"); ok { | ||||
return s, git.RawDiffNormal | ||||
} else if s, ok = strings.CutSuffix(refShortName, ".patch"); ok { | ||||
return s, git.RawDiffPatch | ||||
} | ||||
return refShortName, "" | ||||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -9,10 +9,13 @@ import ( | |||||
"net/url" | ||||||
"strings" | ||||||
"testing" | ||||||
"time" | ||||||
|
||||||
"code.gitea.io/gitea/models/db" | ||||||
"code.gitea.io/gitea/models/unittest" | ||||||
user_model "code.gitea.io/gitea/models/user" | ||||||
git_module "code.gitea.io/gitea/modules/git" | ||||||
"code.gitea.io/gitea/modules/gitrepo" | ||||||
"code.gitea.io/gitea/modules/test" | ||||||
repo_service "code.gitea.io/gitea/services/repository" | ||||||
"code.gitea.io/gitea/tests" | ||||||
|
@@ -158,3 +161,212 @@ func TestCompareCodeExpand(t *testing.T) { | |||||
} | ||||||
}) | ||||||
} | ||||||
|
||||||
func TestCompareRawDiffNormal(t *testing.T) { | ||||||
onGiteaRun(t, func(t *testing.T, u *url.URL) { | ||||||
user1 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) | ||||||
repo, err := repo_service.CreateRepositoryDirectly(db.DefaultContext, user1, user1, repo_service.CreateRepoOptions{ | ||||||
Name: "test_raw_diff", | ||||||
Readme: "Default", | ||||||
AutoInit: true, | ||||||
DefaultBranch: "main", | ||||||
}, true) | ||||||
assert.NoError(t, err) | ||||||
session := loginUser(t, user1.Name) | ||||||
|
||||||
r, _ := gitrepo.OpenRepository(db.DefaultContext, repo) | ||||||
|
||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
oldRef, _ := r.GetBranchCommit(repo.DefaultBranch) | ||||||
oldBlobRef, _ := revParse(r, oldRef.ID.String(), "README.md") | ||||||
|
||||||
testEditFile(t, session, user1.Name, repo.Name, "main", "README.md", strings.Repeat("a\n", 2)) | ||||||
|
||||||
newRef, _ := r.GetBranchCommit(repo.DefaultBranch) | ||||||
newBlobRef, _ := revParse(r, newRef.ID.String(), "README.md") | ||||||
|
||||||
req := NewRequest(t, "GET", fmt.Sprintf("/user1/test_raw_diff/compare/%s...%s.diff", oldRef.ID.String(), newRef.ID.String())) | ||||||
resp := session.MakeRequest(t, req, http.StatusOK) | ||||||
|
||||||
expected := fmt.Sprintf(`diff --git a/README.md b/README.md | ||||||
index %s..%s 100644 | ||||||
--- a/README.md | ||||||
+++ b/README.md | ||||||
@@ -1,2 +1,2 @@ | ||||||
-# test_raw_diff | ||||||
- | ||||||
+a | ||||||
+a | ||||||
`, oldBlobRef[:7], newBlobRef[:7]) | ||||||
assert.Equal(t, expected, resp.Body.String()) | ||||||
}) | ||||||
} | ||||||
|
||||||
func TestCompareRawDiffPatch(t *testing.T) { | ||||||
onGiteaRun(t, func(t *testing.T, u *url.URL) { | ||||||
user1 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) | ||||||
repo, err := repo_service.CreateRepositoryDirectly(db.DefaultContext, user1, user1, repo_service.CreateRepoOptions{ | ||||||
Name: "test_raw_diff", | ||||||
Readme: "Default", | ||||||
AutoInit: true, | ||||||
DefaultBranch: "main", | ||||||
}, true) | ||||||
assert.NoError(t, err) | ||||||
session := loginUser(t, user1.Name) | ||||||
|
||||||
r, _ := gitrepo.OpenRepository(db.DefaultContext, repo) | ||||||
|
||||||
// Get the old commit and blob reference | ||||||
oldRef, _ := r.GetBranchCommit(repo.DefaultBranch) | ||||||
oldBlobRef, _ := revParse(r, oldRef.ID.String(), "README.md") | ||||||
|
||||||
resp := testEditFile(t, session, user1.Name, repo.Name, "main", "README.md", strings.Repeat("a\n", 2)) | ||||||
|
||||||
newRef, _ := r.GetBranchCommit(repo.DefaultBranch) | ||||||
newBlobRef, _ := revParse(r, newRef.ID.String(), "README.md") | ||||||
|
||||||
// Get the last modified time from the response header | ||||||
respTs, _ := time.Parse(time.RFC1123, resp.Result().Header.Get("Last-Modified")) | ||||||
respTs = respTs.In(time.Local) | ||||||
|
||||||
// Format the timestamp to match the expected format in the patch | ||||||
customFormat := "Mon, 02 Jan 2006 15:04:05 -0700" | ||||||
respTsStr := respTs.Format(customFormat) | ||||||
|
||||||
req := NewRequest(t, "GET", fmt.Sprintf("/user1/test_raw_diff/compare/%s...%s.patch", oldRef.ID.String(), newRef.ID.String())) | ||||||
resp = session.MakeRequest(t, req, http.StatusOK) | ||||||
|
||||||
expected := fmt.Sprintf(`From %s Mon Sep 17 00:00:00 2001 | ||||||
From: User One <user1@example.com> | ||||||
Date: %s | ||||||
Subject: [PATCH] Update README.md | ||||||
|
||||||
--- | ||||||
README.md | 4 ++-- | ||||||
1 file changed, 2 insertions(+), 2 deletions(-) | ||||||
|
||||||
diff --git a/README.md b/README.md | ||||||
index %s..%s 100644 | ||||||
--- a/README.md | ||||||
+++ b/README.md | ||||||
@@ -1,2 +1,2 @@ | ||||||
-# test_raw_diff | ||||||
- | ||||||
+a | ||||||
+a | ||||||
`, newRef.ID.String(), respTsStr, oldBlobRef[:7], newBlobRef[:7]) | ||||||
assert.Equal(t, expected, resp.Body.String()) | ||||||
}) | ||||||
} | ||||||
|
||||||
func TestCompareRawDiffNormalSameOwnerDifferentRepo(t *testing.T) { | ||||||
onGiteaRun(t, func(t *testing.T, u *url.URL) { | ||||||
user1 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) | ||||||
repo, err := repo_service.CreateRepositoryDirectly(db.DefaultContext, user1, user1, repo_service.CreateRepoOptions{ | ||||||
Name: "test_raw_diff", | ||||||
Readme: "Default", | ||||||
AutoInit: true, | ||||||
DefaultBranch: "main", | ||||||
}, true) | ||||||
assert.NoError(t, err) | ||||||
session := loginUser(t, user1.Name) | ||||||
|
||||||
headRepo, err := repo_service.CreateRepositoryDirectly(db.DefaultContext, user1, user1, repo_service.CreateRepoOptions{ | ||||||
Name: "test_raw_diff_head", | ||||||
Readme: "Default", | ||||||
AutoInit: true, | ||||||
DefaultBranch: "main", | ||||||
}, true) | ||||||
assert.NoError(t, err) | ||||||
|
||||||
r, _ := gitrepo.OpenRepository(db.DefaultContext, repo) | ||||||
hr, _ := gitrepo.OpenRepository(db.DefaultContext, headRepo) | ||||||
|
||||||
oldRef, _ := r.GetBranchCommit(repo.DefaultBranch) | ||||||
oldBlobRef, _ := revParse(r, oldRef.ID.String(), "README.md") | ||||||
|
||||||
testEditFile(t, session, user1.Name, headRepo.Name, "main", "README.md", strings.Repeat("a\n", 2)) | ||||||
|
||||||
newRef, _ := hr.GetBranchCommit(headRepo.DefaultBranch) | ||||||
newBlobRef, _ := revParse(hr, newRef.ID.String(), "README.md") | ||||||
|
||||||
req := NewRequest(t, "GET", fmt.Sprintf("/user1/test_raw_diff/compare/%s...%s/%s:%s.diff", oldRef.ID.String(), user1.LowerName, headRepo.LowerName, newRef.ID.String())) | ||||||
resp := session.MakeRequest(t, req, http.StatusOK) | ||||||
|
||||||
expected := fmt.Sprintf(`diff --git a/README.md b/README.md | ||||||
index %s..%s 100644 | ||||||
--- a/README.md | ||||||
+++ b/README.md | ||||||
@@ -1,2 +1,2 @@ | ||||||
-# test_raw_diff | ||||||
- | ||||||
+a | ||||||
+a | ||||||
`, oldBlobRef[:7], newBlobRef[:7]) | ||||||
assert.Equal(t, expected, resp.Body.String()) | ||||||
}) | ||||||
} | ||||||
|
||||||
func TestCompareRawDiffNormalAcrossForks(t *testing.T) { | ||||||
onGiteaRun(t, func(t *testing.T, u *url.URL) { | ||||||
user1 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) | ||||||
user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) | ||||||
|
||||||
repo, err := repo_service.CreateRepositoryDirectly(db.DefaultContext, user1, user1, repo_service.CreateRepoOptions{ | ||||||
Name: "test_raw_diff", | ||||||
Readme: "Default", | ||||||
AutoInit: true, | ||||||
DefaultBranch: "main", | ||||||
}, true) | ||||||
assert.NoError(t, err) | ||||||
|
||||||
headRepo, err := repo_service.ForkRepository(db.DefaultContext, user2, user2, repo_service.ForkRepoOptions{ | ||||||
BaseRepo: repo, | ||||||
Name: repo.Name, | ||||||
Description: repo.Description, | ||||||
SingleBranch: "", | ||||||
}) | ||||||
assert.NoError(t, err) | ||||||
|
||||||
session := loginUser(t, user2.Name) | ||||||
|
||||||
r, _ := gitrepo.OpenRepository(db.DefaultContext, repo) | ||||||
hr, _ := gitrepo.OpenRepository(db.DefaultContext, headRepo) | ||||||
|
||||||
oldRef, _ := r.GetBranchCommit(repo.DefaultBranch) | ||||||
oldBlobRef, _ := revParse(r, oldRef.ID.String(), "README.md") | ||||||
|
||||||
testEditFile(t, session, user2.Name, headRepo.Name, "main", "README.md", strings.Repeat("a\n", 2)) | ||||||
|
||||||
newRef, _ := hr.GetBranchCommit(headRepo.DefaultBranch) | ||||||
newBlobRef, _ := revParse(hr, newRef.ID.String(), "README.md") | ||||||
|
||||||
session = loginUser(t, user1.Name) | ||||||
|
||||||
req := NewRequest(t, "GET", fmt.Sprintf("/user1/test_raw_diff/compare/%s...%s:%s.diff", oldRef.ID.String(), user2.LowerName, newRef.ID.String())) | ||||||
resp := session.MakeRequest(t, req, http.StatusOK) | ||||||
|
||||||
expected := fmt.Sprintf(`diff --git a/README.md b/README.md | ||||||
index %s..%s 100644 | ||||||
--- a/README.md | ||||||
+++ b/README.md | ||||||
@@ -1,2 +1,2 @@ | ||||||
-# test_raw_diff | ||||||
- | ||||||
+a | ||||||
+a | ||||||
`, oldBlobRef[:7], newBlobRef[:7]) | ||||||
assert.Equal(t, expected, resp.Body.String()) | ||||||
}) | ||||||
} | ||||||
|
||||||
// helper function to use rev-parse | ||||||
// revParse resolves a revision reference to other git-related objects | ||||||
func revParse(repo *git_module.Repository, ref, file string) (string, error) { | ||||||
stdout, _, err := git_module.NewCommand("rev-parse"). | ||||||
AddDynamicArguments(ref+":"+file). | ||||||
RunStdString(repo.Ctx, &git_module.RunOpts{Dir: repo.Path}) | ||||||
if err != nil { | ||||||
return "", err | ||||||
} | ||||||
return strings.TrimSpace(stdout), nil | ||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When trying to understand the logic, I find something strange, so I left a FIXME here.
Do you have ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ps: if it is too complex and we don't really need to support these cases, we can also skip the diff/patch support for these cases and leave some comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll address that, I prefer to support all use cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wxiaoguang is it clearer now?
I think i covered all use cases with integration tests as well, you think there is any other case we need to cover?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so.
ref1...headRepo:ref2
, but what if ref1 only exists in base repo and ref2 only exists in head repo?