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.

Sign up for GitHub

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

Use beforeCommit instead of baseCommit #22949

Merged
merged 15 commits into from
Feb 20, 2023
Merged

Use beforeCommit instead of baseCommit #22949

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
287516b
Use beforeCommit instead of baseCommit
kdumontnu Feb 17, 2023
e0576cf
Merge branch 'main' into fix-before_commit_three_dot
kdumontnu Feb 17, 2023
42887c2
Merge branch 'main' into fix-before_commit_three_dot
kdumontnu Feb 17, 2023
4a2875c
Merge branch 'main' into fix-before_commit_three_dot
kdumontnu Feb 18, 2023
e9c182d
Merge branch 'main' into fix-before_commit_three_dot
kdumontnu Feb 19, 2023
fb1dd85
Merge branch 'main' into fix-before_commit_three_dot
kdumontnu Feb 19, 2023
7bd6bcc
Merge branch 'main' into fix-before_commit_three_dot
kdumontnu Feb 19, 2023
d012886
Merge branch 'main' into fix-before_commit_three_dot
kdumontnu Feb 19, 2023
c87f0e3
Add integration tests
kdumontnu Feb 19, 2023
853e48a
Add repo20 graph
kdumontnu Feb 19, 2023
37fccdb
Fix linting
kdumontnu Feb 19, 2023
7bdd969
Fix integration test
kdumontnu Feb 19, 2023
eda13d6
Combine branch tests to one for performance
kdumontnu Feb 19, 2023
df6a87a
Merge branch 'main' into fix-before_commit_three_dot
kdumontnu Feb 20, 2023
b320bba
Merge branch 'main' into fix-before_commit_three_dot
kdumontnu Feb 20, 2023
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
11 changes: 5 additions & 6 deletions routers/web/repo/compare.go
View file
Open in desktop
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)
lunny marked this conversation as resolved.
Show resolved Hide resolved
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
View file
Open in desktop
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 •
edited
Loading

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