Fix GetCommitBranchStart bug
#33298
Fix GetCommitBranchStart bug
#33298
Conversation
GiteaBot
added
the
lgtm/need 2
pull-request-size
bot
added
the
size/S
pull-request-size
bot
added
size/XS
| for _, commitID := range parts { | ||
| branches, err := repo.getBranches(env, string(commitID), 2) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| for _, b := range branches { | ||
| if b != branch { | ||
| return startCommitID, nil | ||
| return string(commitID), nil |
There was a problem hiding this comment.
I am unable to understand what GetCommitBranchStart is doing here ....
The old code seems to "return the last found one", the new code seems to "return the first found one"?
There was a problem hiding this comment.
I am unable to understand what
GetCommitBranchStartis doing here ....
According to #31738, GetCommitBranchStart is used to find the divergence commit of a newly created branch.
The old code seems to "return the last found one", the new code seems to "return the first found one"?
The old code returns "the first commit after the divergence point", but I think here we should return "the divergence point". Because "the first commit after the divergence point" may not exist. For example, it does not exist when we create a new branch without any changes.
There was a problem hiding this comment.
Could there be some comments (with some examples)?
There was a problem hiding this comment.
Sure. I'll try to add some comments and test cases.
pull-request-size
bot
added
size/S
pull-request-size
bot
added
size/M
| @@ -466,7 +497,7 @@ func doProtectBranch(ctx APITestContext, branch, userToWhitelistPush, userToWhit | |||
| // Check if master branch has been locked successfully | |||
| flashCookie := ctx.Session.GetCookie(gitea_context.CookieNameFlash) | |||
| assert.NotNil(t, flashCookie) | |||
| assert.EqualValues(t, "success%3DBranch%2Bprotection%2Bfor%2Brule%2B%2522"+url.QueryEscape(branch)+"%2522%2Bhas%2Bbeen%2Bupdated.", flashCookie.Value) | |||
| assert.EqualValues(t, "success%3DBranch%2Bprotection%2Bfor%2Brule%2B%2522"+url.QueryEscape(url.QueryEscape(branch))+"%2522%2Bhas%2Bbeen%2Bupdated.", flashCookie.Value) | |||
There was a problem hiding this comment.
I have to escape branch twice here, otherwise the test will fail. The reason may be that we escaped the cookie value twice:
gitea/services/context/context.go
Line 184 in 7582eb0
gitea/modules/web/middleware/cookie.go
Line 44 in 7582eb0
| @@ -336,7 +366,7 @@ func doBranchProtectPRMerge(baseCtx *APITestContext, dstPath string) func(t *tes | |||
|
|
|||
| // Protect branch without any whitelisting | |||
| t.Run("ProtectBranchNoWhitelist", func(t *testing.T) { | |||
| doProtectBranch(ctx, "protected", "", "", "") | |||
| doProtectBranch(ctx, "protected", "", "", "", "") | |||
There was a problem hiding this comment.
Maybe I found some other problems. Will the branch protection rule be created? Should this be doProtectBranch(ctx, "protected", "", "", "", "")(t) ?
Fix #33265