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

Jump to bottom

Fix tautological conditions #30735

Merged
merged 8 commits into from
Apr 30, 2024
Merged

Fix tautological conditions #30735

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
52c003a
Fix tautological conditions
silverwind Apr 27, 2024
392a25b
remove impossible condition
silverwind Apr 28, 2024
b88d4db
Merge branch 'main' into taut
silverwind Apr 29, 2024
62ae228
fix two more issues in git_test.go
silverwind Apr 29, 2024
ec76a02
Merge branch 'main' into taut
silverwind Apr 29, 2024
95f5f64
Merge branch 'main' into taut
GiteaBot Apr 30, 2024
c02d91d
Merge branch 'main' into taut
GiteaBot Apr 30, 2024
5e19ed5
Merge branch 'main' into taut
GiteaBot Apr 30, 2024
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
6 changes: 0 additions & 6 deletions modules/indexer/code/indexer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,12 +178,6 @@ func Init() {
}()

rIndexer = elasticsearch.NewIndexer(setting.Indexer.RepoConnStr, setting.Indexer.RepoIndexerName)
if err != nil {
cancel()
(*globalIndexer.Load()).Close()
close(waitChannel)
log.Fatal("PID: %d Unable to create the elasticsearch Repository Indexer connstr: %s Error: %v", os.Getpid(), setting.Indexer.RepoConnStr, err)
}
existed, err = rIndexer.Init(ctx)
if err != nil {
cancel()
Expand Down
18 changes: 8 additions & 10 deletions routers/private/hook_post_receive.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,16 +117,14 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) {
}
}
if len(branchesToSync) > 0 {
if gitRepo == nil {
var err error
gitRepo, err = gitrepo.OpenRepository(ctx, repo)
if err != nil {
log.Error("Failed to open repository: %s/%s Error: %v", ownerName, repoName, err)
ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{
Err: fmt.Sprintf("Failed to open repository: %s/%s Error: %v", ownerName, repoName, err),
})
return
}
var err error
gitRepo, err = gitrepo.OpenRepository(ctx, repo)
if err != nil {
log.Error("Failed to open repository: %s/%s Error: %v", ownerName, repoName, err)
ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{
Err: fmt.Sprintf("Failed to open repository: %s/%s Error: %v", ownerName, repoName, err),
})
return
silverwind marked this conversation as resolved.
Show resolved Hide resolved
}

var (
Expand Down
2 changes: 1 addition & 1 deletion services/auth/source/oauth2/providers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ func createProvider(providerName string, source *Source) (goth.Provider, error)
}

// always set the name if provider is created so we can support multiple setups of 1 provider
if err == nil && provider != nil {
if provider != nil {
silverwind marked this conversation as resolved.
Show resolved Hide resolved
provider.SetName(providerName)
}

Expand Down
14 changes: 8 additions & 6 deletions services/convert/issue.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,13 +211,11 @@ func ToLabel(label *issues_model.Label, repo *repo_model.Repository, org *user_m
IsArchived: label.IsArchived(),
}

labelBelongsToRepo := label.BelongsToRepo()

// calculate URL
if label.BelongsToRepo() && repo != nil {
if repo != nil {
silverwind marked this conversation as resolved.
Show resolved Hide resolved
result.URL = fmt.Sprintf("%s/labels/%d", repo.APIURL(), label.ID)
} else {
log.Error("ToLabel did not get repo to calculate url for label with id '%d'", label.ID)
}
if labelBelongsToRepo && repo != nil {
result.URL = fmt.Sprintf("%s/labels/%d", repo.APIURL(), label.ID)
} else { // BelongsToOrg
if org != nil {
result.URL = fmt.Sprintf("%sapi/v1/orgs/%s/labels/%d", setting.AppURL, url.PathEscape(org.Name), label.ID)
Expand All @@ -226,6 +224,10 @@ func ToLabel(label *issues_model.Label, repo *repo_model.Repository, org *user_m
}
}

if labelBelongsToRepo && repo == nil {
log.Error("ToLabel did not get repo to calculate url for label with id '%d'", label.ID)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I still have objection for doing pure-refactoring in this period.

And ideally, it should be like this:

if labelBelongsToRepo {
    if repo != nil {
        ...
    } else {
       log.Error
    }
} else {
    if org != nil {
        ...
    } else {
        log.Error(...)
    }
}

Copy link
Member Author

@silverwind silverwind Apr 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Those sections are unlikely to be targeted by backports, so I think it's okay. Feel free to raise a PR for that.


return result
}

Expand Down
9 changes: 3 additions & 6 deletions tests/integration/git_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ func testGit(t *testing.T, u *url.URL) {
rawTest(t, &httpContext, little, big, littleLFS, bigLFS)
mediaTest(t, &httpContext, little, big, littleLFS, bigLFS)

t.Run("CreateAgitFlowPull", doCreateAgitFlowPull(dstPath, &httpContext, "master", "test/head"))
t.Run("CreateAgitFlowPull", doCreateAgitFlowPull(dstPath, &httpContext, "test/head"))
t.Run("BranchProtectMerge", doBranchProtectPRMerge(&httpContext, dstPath))
t.Run("AutoMerge", doAutoPRMerge(&httpContext, dstPath))
t.Run("CreatePRAndSetManuallyMerged", doCreatePRAndSetManuallyMerged(httpContext, httpContext, dstPath, "master", "test-manually-merge"))
Expand Down Expand Up @@ -122,7 +122,7 @@ func testGit(t *testing.T, u *url.URL) {
rawTest(t, &sshContext, little, big, littleLFS, bigLFS)
mediaTest(t, &sshContext, little, big, littleLFS, bigLFS)

t.Run("CreateAgitFlowPull", doCreateAgitFlowPull(dstPath, &sshContext, "master", "test/head2"))
t.Run("CreateAgitFlowPull", doCreateAgitFlowPull(dstPath, &sshContext, "test/head2"))
t.Run("BranchProtectMerge", doBranchProtectPRMerge(&sshContext, dstPath))
t.Run("MergeFork", func(t *testing.T) {
defer tests.PrintCurrentTest(t)()
Expand Down Expand Up @@ -329,9 +329,6 @@ func generateCommitWithNewData(size int, repoPath, email, fullName, prefix strin
}
written += n
}
if err != nil {
return "", err
}

// Commit
// Now here we should explicitly allow lfs filters to run
Expand Down Expand Up @@ -693,7 +690,7 @@ func doAutoPRMerge(baseCtx *APITestContext, dstPath string) func(t *testing.T) {
}
}

func doCreateAgitFlowPull(dstPath string, ctx *APITestContext, baseBranch, headBranch string) func(t *testing.T) {
func doCreateAgitFlowPull(dstPath string, ctx *APITestContext, headBranch string) func(t *testing.T) {
return func(t *testing.T) {
defer tests.PrintCurrentTest(t)()

Expand Down