Skip to content

Use beforeCommit instead of baseCommit #22949

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

Merged
merged 15 commits into from
Feb 20, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion models/db/iterate_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ func TestIterate(t *testing.T) {
return nil
})
assert.NoError(t, err)
assert.EqualValues(t, 81, repoCnt)
assert.EqualValues(t, 83, repoCnt)

err = db.Iterate(db.DefaultContext, nil, func(ctx context.Context, repoUnit *repo_model.RepoUnit) error {
reopUnit2 := repo_model.RepoUnit{ID: repoUnit.ID}
Expand Down
13 changes: 13 additions & 0 deletions models/fixtures/repo_unit.yml
Original file line number Diff line number Diff line change
Expand Up @@ -556,3 +556,16 @@
repo_id: 54
type: 1
created_unix: 946684810

-
id: 82
repo_id: 31
type: 1
created_unix: 946684810

-
id: 83
repo_id: 31
type: 3
config: "{\"IgnoreWhitespaceConflicts\":false,\"AllowMerge\":true,\"AllowRebase\":true,\"AllowRebaseMerge\":true,\"AllowSquash\":true}"
created_unix: 946684810
11 changes: 5 additions & 6 deletions routers/web/repo/compare.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,8 @@ const (
)

// setCompareContext sets context data.
func setCompareContext(ctx *context.Context, base, head *git.Commit, headOwner, headName string) {
ctx.Data["BaseCommit"] = base
func setCompareContext(ctx *context.Context, before, head *git.Commit, headOwner, headName string) {
ctx.Data["BeforeCommit"] = before
ctx.Data["HeadCommit"] = head

ctx.Data["GetBlobByPathForCommit"] = func(commit *git.Commit, path string) *git.Blob {
Expand All @@ -59,7 +59,7 @@ func setCompareContext(ctx *context.Context, base, head *git.Commit, headOwner,
return blob
}

setPathsCompareContext(ctx, base, head, headOwner, headName)
setPathsCompareContext(ctx, before, head, headOwner, headName)
setImageCompareContext(ctx)
setCsvCompareContext(ctx)
}
Expand Down Expand Up @@ -629,9 +629,8 @@ func PrepareCompareDiff(
}

baseGitRepo := ctx.Repo.GitRepo
baseCommitID := ci.CompareInfo.BaseCommitID

baseCommit, err := baseGitRepo.GetCommit(baseCommitID)
beforeCommit, err := baseGitRepo.GetCommit(beforeCommitID)
if err != nil {
ctx.ServerError("GetCommit", err)
return false
Expand Down Expand Up @@ -668,7 +667,7 @@ func PrepareCompareDiff(
ctx.Data["Username"] = ci.HeadUser.Name
ctx.Data["Reponame"] = ci.HeadRepo.Name

setCompareContext(ctx, baseCommit, headCommit, ci.HeadUser.Name, repo.Name)
setCompareContext(ctx, beforeCommit, headCommit, ci.HeadUser.Name, repo.Name)

return false
}
Expand Down
2 changes: 1 addition & 1 deletion templates/repo/diff/box.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@
<div id="diff-file-boxes" class="sixteen wide column">
{{range $i, $file := .Diff.Files}}
{{/*notice: the index of Diff.Files should not be used for element ID, because the index will be restarted from 0 when doing load-more for PRs with a lot of files*/}}
{{$blobBase := call $.GetBlobByPathForCommit $.BaseCommit $file.OldName}}
{{$blobBase := call $.GetBlobByPathForCommit $.BeforeCommit $file.OldName}}
{{$blobHead := call $.GetBlobByPathForCommit $.HeadCommit $file.Name}}
Copy link
Member

Choose a reason for hiding this comment

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

In a similar fashion, shouldn't this be AfterCommit then?

Copy link
Member

Choose a reason for hiding this comment

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

Most of the variables using this start with "head".

Copy link
Member

Choose a reason for hiding this comment

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

Yes, and most of the variables intended for the base branch start with base?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’ll have to take a look at where AfterCommit comes from. I don’t see how that would be the case though: we should be comparing the “current commit” against the last common ancestor. I expect AfterCommit will just be the same commit ref as BeforeCommit in a three dot diff.

Copy link
Contributor Author

@kdumontnu kdumontnu Feb 19, 2023

Choose a reason for hiding this comment

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

I checked it out, and AfterCommitID is just set to headCommitID here.

Whereas, beforeCommit depends on whether it's a direct comparison (2-dot) or indirect comparison (3-dot) here.

I could change HeadCommit to AfterCommit in the context, but it's just a matter of naming preference. Maybe the table below will help clarify (in parenthesis is what we name them in the code). In other places in the code we call it "start" and "end".

use-case before (from/red) after (to/green)
commit parent ("parentCommit" ) commit ("commit")
direct compare base head
indirect compare merge-base ("MergeBase") head
PR compare (indirect) ^ ("startCommit") ^ ("endCommit")

So, there's probably opportunity for cleanup here, but the behavior should be correct now (from the scope I checked).

{{$isImage := or (call $.IsBlobAnImage $blobBase) (call $.IsBlobAnImage $blobHead)}}
{{$isCsv := (call $.IsCsvFile $file)}}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
#!/usr/bin/env bash
"/home/tris/Projects/go/src/code.gitea.io/gitea/gitea" hook --config='/home/tris/Projects/go/src/code.gitea.io/gitea/custom/conf/app.ini' post-receive
"$GITEA_ROOT/gitea" hook --config="$GITEA_ROOT/$GITEA_CONF" post-receive
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
#!/usr/bin/env bash
"/home/tris/Projects/go/src/code.gitea.io/gitea/gitea" hook --config='/home/tris/Projects/go/src/code.gitea.io/gitea/custom/conf/app.ini' pre-receive
"$GITEA_ROOT/gitea" hook --config="$GITEA_ROOT/$GITEA_CONF" pre-receive
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
#!/usr/bin/env bash
"/home/tris/Projects/go/src/code.gitea.io/gitea/gitea" hook --config='/home/tris/Projects/go/src/code.gitea.io/gitea/custom/conf/app.ini' update $1 $2 $3
"$GITEA_ROOT/gitea" hook --config="$GITEA_ROOT/$GITEA_CONF" update $1 $2 $3
Binary file not shown.
Binary file not shown.
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
x��An�0 s�+���l� �[_A�TkIJC>���z[,f1��Z�!�K�̀��S��L5[,�D҉'�:a�Rнe���Dl� �:^C�g�lH�d���>iqr��m��s1���K���m=?U��3o������������k�ߝ{��@w�ʼ���E]
Binary file not shown.
Binary file not shown.
Binary file not shown.
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
x��A
�0@Q�9�\@�N�&w�"�L��4Ҧ���+�����Z�؀�vm�`��!&�u֎m��FK�l��a���8t�]�l;��H�}g��9'2}�{�*l�Q�}&�+�i+un�v��0N�X� ���,!�{D����&�:uI �����<N���qEo
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
x��An�0EY�s�=v2FB v=D5�';U�T���
������Z묀���%�P(z�����p�D�%8�!8[�/o�r�R�1F�p�HS.����3�$��]���E��g��ڴ��{�y9�~�{ ��v�����S��gn��
����_���2���c6tuI�
Binary file not shown.
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
c8e31bc7688741a5287fcde4fbb8fc129ca07027
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
cfe3b3c1fd36fba04f9183287b106497e1afe986
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
8babce967f21b9dfa6987f943b91093dac58a4f0
78 changes: 78 additions & 0 deletions tests/integration/compare_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
package integration

import (
"fmt"
"net/http"
"strings"
"testing"
Expand Down Expand Up @@ -40,3 +41,80 @@ func TestCompareDefault(t *testing.T) {
selection := htmlDoc.doc.Find(".choose.branch .filter.dropdown")
assert.Lenf(t, selection.Nodes, 2, "The template has changed")
}

// Ensure the comparison matches what we expect
func inspectCompare(t *testing.T, htmlDoc *HTMLDoc, diffCount int, diffChanges []string) {
selection := htmlDoc.doc.Find("#diff-file-boxes").Children()

assert.Lenf(t, selection.Nodes, diffCount, "Expected %v diffed files, found: %v", diffCount, len(selection.Nodes))

for _, diffChange := range diffChanges {
selection = htmlDoc.doc.Find(fmt.Sprintf("[data-new-filename=\"%s\"]", diffChange))
assert.Lenf(t, selection.Nodes, 1, "Expected 1 match for [data-new-filename=\"%s\"], found: %v", diffChange, len(selection.Nodes))
}
}

// Git commit graph for repo20
// * 8babce9 (origin/remove-files-b) Add a dummy file
// * b67e43a Delete test.csv and link_hi
// | * cfe3b3c (origin/remove-files-a) Delete test.csv and link_hi
// |/
// * c8e31bc (origin/add-csv) Add test csv file
// * 808038d (HEAD -> master, origin/master, origin/HEAD) Added test links

func TestCompareBranches(t *testing.T) {
defer tests.PrepareTestEnv(t)()

session := loginUser(t, "user2")

// Inderect compare remove-files-b (head) with add-csv (base) branch
//
// 'link_hi' and 'test.csv' are deleted, 'test.txt' is added
req := NewRequest(t, "GET", "/user2/repo20/compare/add-csv...remove-files-b")
resp := session.MakeRequest(t, req, http.StatusOK)
htmlDoc := NewHTMLParser(t, resp.Body)

diffCount := 3
diffChanges := []string{"link_hi", "test.csv", "test.txt"}

inspectCompare(t, htmlDoc, diffCount, diffChanges)

// Inderect compare remove-files-b (head) with remove-files-a (base) branch
//
// 'link_hi' and 'test.csv' are deleted, 'test.txt' is added

req = NewRequest(t, "GET", "/user2/repo20/compare/remove-files-a...remove-files-b")
resp = session.MakeRequest(t, req, http.StatusOK)
htmlDoc = NewHTMLParser(t, resp.Body)

diffCount = 3
diffChanges = []string{"link_hi", "test.csv", "test.txt"}

inspectCompare(t, htmlDoc, diffCount, diffChanges)

// Inderect compare remove-files-a (head) with remove-files-b (base) branch
//
// 'link_hi' and 'test.csv' are deleted

req = NewRequest(t, "GET", "/user2/repo20/compare/remove-files-b...remove-files-a")
resp = session.MakeRequest(t, req, http.StatusOK)
htmlDoc = NewHTMLParser(t, resp.Body)

diffCount = 2
diffChanges = []string{"link_hi", "test.csv"}

inspectCompare(t, htmlDoc, diffCount, diffChanges)

// Direct compare remove-files-b (head) with remove-files-a (base) branch
//
// 'test.txt' is deleted

req = NewRequest(t, "GET", "/user2/repo20/compare/remove-files-b..remove-files-a")
resp = session.MakeRequest(t, req, http.StatusOK)
htmlDoc = NewHTMLParser(t, resp.Body)

diffCount = 1
diffChanges = []string{"test.txt"}

inspectCompare(t, htmlDoc, diffCount, diffChanges)
}