diff --git a/custom/conf/app.example.ini b/custom/conf/app.example.ini index 803231ff12..5eb4a5e995 100644 --- a/custom/conf/app.example.ini +++ b/custom/conf/app.example.ini @@ -1153,9 +1153,6 @@ LEVEL = Info ;; Add co-authored-by and co-committed-by trailers if committer does not match author ;ADD_CO_COMMITTER_TRAILERS = true ;; -;; In addition to testing patches using the three-way merge method, re-test conflicting patches with git apply -;TEST_CONFLICTING_PATCHES_WITH_GIT_APPLY = false -;; ;; Retarget child pull requests to the parent pull request branch target on merge of parent pull request. It only works on merged PRs where the head and base branch target the same repo. ;RETARGET_CHILDREN_ON_MERGE = true ;; diff --git a/models/auth/source.go b/models/auth/source.go index c0b262f870..7a008f08a8 100644 --- a/models/auth/source.go +++ b/models/auth/source.go @@ -12,6 +12,7 @@ import ( "code.gitea.io/gitea/models/db" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/optional" + "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/timeutil" "code.gitea.io/gitea/modules/util" @@ -139,7 +140,10 @@ func init() { // BeforeSet is invoked from XORM before setting the value of a field of this object. func (source *Source) BeforeSet(colName string, val xorm.Cell) { if colName == "type" { - typ := Type(db.Cell2Int64(val)) + typ, _, err := db.CellToInt(val, NoType) + if err != nil { + setting.PanicInDevOrTesting("Unable to convert login source (id=%d) type: %v", source.ID, err) + } constructor, ok := registeredConfigs[typ] if !ok { return diff --git a/models/auth/source_test.go b/models/auth/source_test.go index ebc462c581..fbd663a64f 100644 --- a/models/auth/source_test.go +++ b/models/auth/source_test.go @@ -17,13 +17,9 @@ import ( ) type TestSource struct { - auth_model.ConfigBase + auth_model.ConfigBase `json:"-"` - Provider string - ClientID string - ClientSecret string - OpenIDConnectAutoDiscoveryURL string - IconURL string + TestField string } // FromDB fills up a LDAPConfig from serialized format. @@ -37,27 +33,23 @@ func (source *TestSource) ToDB() ([]byte, error) { } func TestDumpAuthSource(t *testing.T) { - assert.NoError(t, unittest.PrepareTestDatabase()) + require.NoError(t, unittest.PrepareTestDatabase()) authSourceSchema, err := unittest.GetXORMEngine().TableInfo(new(auth_model.Source)) - assert.NoError(t, err) + require.NoError(t, err) auth_model.RegisterTypeConfig(auth_model.OAuth2, new(TestSource)) + source := &auth_model.Source{ + Type: auth_model.OAuth2, + Name: "TestSource", + Cfg: &TestSource{TestField: "TestValue"}, + } + require.NoError(t, auth_model.CreateSource(t.Context(), source)) - auth_model.CreateSource(t.Context(), &auth_model.Source{ - Type: auth_model.OAuth2, - Name: "TestSource", - IsActive: false, - Cfg: &TestSource{ - Provider: "ConvertibleSourceName", - ClientID: "42", - }, - }) - - sb := new(strings.Builder) - - // TODO: this test is quite hacky, it should use a low-level "select" (without model processors) but not a database dump - engine := unittest.GetXORMEngine() - require.NoError(t, engine.DumpTables([]*schemas.Table{authSourceSchema}, sb)) - assert.Contains(t, sb.String(), `"Provider":"ConvertibleSourceName"`) + // intentionally test the "dump" to make sure the dumped JSON is correct: https://github.com/go-gitea/gitea/pull/16847 + sb := &strings.Builder{} + require.NoError(t, unittest.GetXORMEngine().DumpTables([]*schemas.Table{authSourceSchema}, sb)) + // the dumped SQL is something like: + // INSERT INTO `login_source` (`id`, `type`, `name`, `is_active`, `is_sync_enabled`, `two_factor_policy`, `cfg`, `created_unix`, `updated_unix`) VALUES (1,6,'TestSource',0,0,'','{"TestField":"TestValue"}',1774179784,1774179784); + assert.Contains(t, sb.String(), `'{"TestField":"TestValue"}'`) } diff --git a/models/db/convert.go b/models/db/convert.go index 80b0f7b04b..374dbfd8a8 100644 --- a/models/db/convert.go +++ b/models/db/convert.go @@ -5,12 +5,11 @@ package db import ( "fmt" - "strconv" - "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" "xorm.io/xorm" + "xorm.io/xorm/convert" "xorm.io/xorm/schemas" ) @@ -74,15 +73,14 @@ WHERE ST.name ='varchar'`) return err } -// Cell2Int64 converts a xorm.Cell type to int64, -// and handles possible irregular cases. -func Cell2Int64(val xorm.Cell) int64 { - switch (*val).(type) { - case []uint8: - log.Trace("Cell2Int64 ([]uint8): %v", *val) - - v, _ := strconv.ParseInt(string((*val).([]uint8)), 10, 64) - return v +// CellToInt converts a xorm.Cell field value to an int value +func CellToInt[T ~int | int64](cell xorm.Cell, def T) (ret T, has bool, err error) { + if *cell == nil { + return def, false, nil } - return (*val).(int64) + val, err := convert.AsInt64(*cell) + if err != nil { + return def, false, err + } + return T(val), true, err } diff --git a/models/repo/repo_unit.go b/models/repo/repo_unit.go index 18ebd6be54..c32adbbcd4 100644 --- a/models/repo/repo_unit.go +++ b/models/repo/repo_unit.go @@ -227,7 +227,11 @@ func (cfg *ProjectsConfig) IsProjectsAllowed(m ProjectsMode) bool { func (r *RepoUnit) BeforeSet(colName string, val xorm.Cell) { switch colName { case "type": - r.Type = unit.Type(db.Cell2Int64(val)) + var err error + r.Type, _, err = db.CellToInt(val, unit.TypeInvalid) + if err != nil { + setting.PanicInDevOrTesting("Unable to convert repo unit (id=%d) type: %v", r.ID, err) + } switch r.Type { case unit.TypeExternalWiki: r.Config = new(ExternalWikiConfig) diff --git a/modules/setting/repository.go b/modules/setting/repository.go index f4e45d4702..9195b7ee50 100644 --- a/modules/setting/repository.go +++ b/modules/setting/repository.go @@ -86,7 +86,6 @@ var ( DefaultMergeMessageOfficialApproversOnly bool PopulateSquashCommentWithCommitMessages bool AddCoCommitterTrailers bool - TestConflictingPatchesWithGitApply bool RetargetChildrenOnMerge bool DelayCheckForInactiveDays int DefaultDeleteBranchAfterMerge bool @@ -211,7 +210,6 @@ var ( DefaultMergeMessageOfficialApproversOnly bool PopulateSquashCommentWithCommitMessages bool AddCoCommitterTrailers bool - TestConflictingPatchesWithGitApply bool RetargetChildrenOnMerge bool DelayCheckForInactiveDays int DefaultDeleteBranchAfterMerge bool diff --git a/modules/translation/mock.go b/modules/translation/mock.go index f457271ea5..02b19d9583 100644 --- a/modules/translation/mock.go +++ b/modules/translation/mock.go @@ -5,8 +5,8 @@ package translation import ( "fmt" + "html" "html/template" - "strings" ) // MockLocale provides a mocked locale without any translations @@ -20,25 +20,40 @@ func (l MockLocale) Language() string { return "en" } -func (l MockLocale) TrString(s string, args ...any) string { - return sprintAny(s, args...) +func (l MockLocale) TrString(format string, args ...any) (ret string) { + ret = format + ":" + for _, arg := range args { + // usually there is no arg or at most 1-2 args, so a simple string concatenation is more efficient + switch v := arg.(type) { + case string: + ret += v + "," + default: + ret += fmt.Sprint(v) + "," + } + } + return ret[:len(ret)-1] } -func (l MockLocale) Tr(s string, args ...any) template.HTML { - return template.HTML(sprintAny(s, args...)) +func (l MockLocale) Tr(format string, args ...any) (ret template.HTML) { + ret = template.HTML(html.EscapeString(format)) + ":" + for _, arg := range args { + // usually there is no arg or at most 1-2 args, so a simple string concatenation is more efficient + switch v := arg.(type) { + case template.HTML: + ret += v + "," + case string: + ret += template.HTML(html.EscapeString(v)) + "," + default: + ret += template.HTML(html.EscapeString(fmt.Sprint(v))) + "," + } + } + return ret[:len(ret)-1] } func (l MockLocale) TrN(cnt any, key1, keyN string, args ...any) template.HTML { - return template.HTML(sprintAny(key1, args...)) + return l.Tr(key1, args...) } func (l MockLocale) PrettyNumber(v any) string { return fmt.Sprint(v) } - -func sprintAny(s string, args ...any) string { - if len(args) == 0 { - return s - } - return s + ":" + fmt.Sprintf(strings.Repeat(",%v", len(args))[1:], args...) -} diff --git a/routers/common/maintenancemode.go b/routers/common/maintenancemode.go index b5827ac94f..adf099df57 100644 --- a/routers/common/maintenancemode.go +++ b/routers/common/maintenancemode.go @@ -7,29 +7,39 @@ import ( "net/http" "strings" + "code.gitea.io/gitea/modules/container" "code.gitea.io/gitea/modules/setting" ) -func isMaintenanceModeAllowedRequest(req *http.Request) bool { - if strings.HasPrefix(req.URL.Path, "/-/") { - // URLs like "/-/admin", "/-/fetch-redirect" and "/-/markup" are still accessible in maintenance mode - return true - } - if strings.HasPrefix(req.URL.Path, "/api/internal/") { - // internal APIs should be allowed - return true - } - if strings.HasPrefix(req.URL.Path, "/user/") { - // URLs like "/user/signin" and "/user/signup" are still accessible in maintenance mode - return true - } - if strings.HasPrefix(req.URL.Path, "/assets/") { - return true - } - return false -} - func MaintenanceModeHandler() func(h http.Handler) http.Handler { + allowedPrefixes := []string{ + "/.well-known/", + "/assets/", + "/avatars/", + + // admin: "/-/admin" + // general-purpose URLs: "/-/fetch-redirect", "/-/markup", etc. + "/-/", + + // internal APIs + "/api/internal/", + + // user login (for admin to login): "/user/login", "/user/logout", "/catpcha/..." + "/user/", + "/captcha/", + } + allowedPaths := container.SetOf( + "/api/healthz", + ) + isMaintenanceModeAllowedRequest := func(req *http.Request) bool { + for _, prefix := range allowedPrefixes { + if strings.HasPrefix(req.URL.Path, prefix) { + return true + } + } + return allowedPaths.Contains(req.URL.Path) + } + return func(next http.Handler) http.Handler { return http.HandlerFunc(func(resp http.ResponseWriter, req *http.Request) { maintenanceMode := setting.Config().Instance.MaintenanceMode.Value(req.Context()) diff --git a/services/pull/patch.go b/services/pull/patch.go index 30f07f8931..114a437cbc 100644 --- a/services/pull/patch.go +++ b/services/pull/patch.go @@ -5,7 +5,6 @@ package pull import ( - "bufio" "context" "fmt" "io" @@ -15,15 +14,12 @@ import ( git_model "code.gitea.io/gitea/models/git" issues_model "code.gitea.io/gitea/models/issues" - "code.gitea.io/gitea/models/unit" - "code.gitea.io/gitea/modules/container" "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/git/gitcmd" "code.gitea.io/gitea/modules/gitrepo" "code.gitea.io/gitea/modules/glob" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/process" - "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/util" ) @@ -57,15 +53,6 @@ func DownloadDiffOrPatch(ctx context.Context, pr *issues_model.PullRequest, w io return nil } -var patchErrorSuffices = []string{ - ": already exists in index", - ": patch does not apply", - ": already exists in working directory", - "unrecognized input", - ": No such file or directory", - ": does not exist in index", -} - func checkPullRequestBranchMergeable(ctx context.Context, pr *issues_model.PullRequest) error { ctx, _, finished := process.GetManager().AddContext(ctx, fmt.Sprintf("checkPullRequestBranchMergeable: %s", pr)) defer finished() @@ -349,151 +336,10 @@ func checkConflictsByTmpRepo(ctx context.Context, pr *issues_model.PullRequest, } // 3. OK the three-way merge method has detected conflicts - // 3a. Are still testing with GitApply? If not set the conflict status and move on - if !setting.Repository.PullRequest.TestConflictingPatchesWithGitApply { - pr.Status = issues_model.PullRequestStatusConflict - pr.ConflictedFiles = conflictFiles - - log.Trace("Found %d files conflicted: %v", len(pr.ConflictedFiles), pr.ConflictedFiles) - return true, nil - } - - // 3b. Create a plain patch from head to base - tmpPatchFile, cleanup, err := setting.AppDataTempDir("git-repo-content").CreateTempFileRandom("patch") - if err != nil { - log.Error("Unable to create temporary patch file! Error: %v", err) - return false, fmt.Errorf("unable to create temporary patch file! Error: %w", err) - } - defer cleanup() - - if err := gitRepo.GetDiffBinary(pr.MergeBase+"...tracking", tmpPatchFile); err != nil { - log.Error("Unable to get patch file from %s to %s in %s Error: %v", pr.MergeBase, pr.HeadBranch, pr.BaseRepo.FullName(), err) - return false, fmt.Errorf("unable to get patch file from %s to %s in %s Error: %w", pr.MergeBase, pr.HeadBranch, pr.BaseRepo.FullName(), err) - } - stat, err := tmpPatchFile.Stat() - if err != nil { - return false, fmt.Errorf("unable to stat patch file: %w", err) - } - patchPath := tmpPatchFile.Name() - tmpPatchFile.Close() - - // 3c. if the size of that patch is 0 - there can be no conflicts! - if stat.Size() == 0 { - log.Debug("PullRequest[%d]: Patch is empty - ignoring", pr.ID) - pr.Status = issues_model.PullRequestStatusEmpty - return false, nil - } - - log.Trace("PullRequest[%d].checkPullRequestMergeableByTmpRepo (patchPath): %s", pr.ID, patchPath) - - // 4. Read the base branch in to the index of the temporary repository - _, _, err = gitcmd.NewCommand("read-tree", tmpRepoBaseBranch).WithDir(tmpBasePath).RunStdString(ctx) - if err != nil { - return false, fmt.Errorf("git read-tree %s: %w", pr.BaseBranch, err) - } - - // 5. Now get the pull request configuration to check if we need to ignore whitespace - prUnit, err := pr.BaseRepo.GetUnit(ctx, unit.TypePullRequests) - if err != nil { - return false, err - } - prConfig := prUnit.PullRequestsConfig() - - // 6. Prepare the arguments to apply the patch against the index - cmdApply := gitcmd.NewCommand("apply", "--check", "--cached") - if prConfig.IgnoreWhitespaceConflicts { - cmdApply.AddArguments("--ignore-whitespace") - } - is3way := false - if git.DefaultFeatures().CheckVersionAtLeast("2.32.0") { - cmdApply.AddArguments("--3way") - is3way = true - } - cmdApply.AddDynamicArguments(patchPath) - - // 7. Prep the pipe: - // - Here we could do the equivalent of: - // `git apply --check --cached patch_file > conflicts` - // Then iterate through the conflicts. However, that means storing all the conflicts - // in memory - which is very wasteful. - // - alternatively we can do the equivalent of: - // `git apply --check ... | grep ...` - // meaning we don't store all the conflicts unnecessarily. - stderrReader, stderrReaderClose := cmdApply.MakeStderrPipe() - defer stderrReaderClose() - - // 8. Run the check command - conflict = false - err = cmdApply. - WithDir(tmpBasePath). - WithPipelineFunc(func(ctx gitcmd.Context) error { - const prefix = "error: patch failed:" - const errorPrefix = "error: " - const threewayFailed = "Failed to perform three-way merge..." - const appliedPatchPrefix = "Applied patch to '" - const withConflicts = "' with conflicts." - - conflicts := make(container.Set[string]) - - // Now scan the output from the command - scanner := bufio.NewScanner(stderrReader) - for scanner.Scan() { - line := scanner.Text() - log.Trace("PullRequest[%d].checkPullRequestMergeableByTmpRepo: stderr: %s", pr.ID, line) - if strings.HasPrefix(line, prefix) { - conflict = true - filepath := strings.TrimSpace(strings.Split(line[len(prefix):], ":")[0]) - conflicts.Add(filepath) - } else if is3way && line == threewayFailed { - conflict = true - } else if strings.HasPrefix(line, errorPrefix) { - conflict = true - for _, suffix := range patchErrorSuffices { - if strings.HasSuffix(line, suffix) { - filepath := strings.TrimSpace(strings.TrimSuffix(line[len(errorPrefix):], suffix)) - if filepath != "" { - conflicts.Add(filepath) - } - break - } - } - } else if is3way && strings.HasPrefix(line, appliedPatchPrefix) && strings.HasSuffix(line, withConflicts) { - conflict = true - filepath := strings.TrimPrefix(strings.TrimSuffix(line, withConflicts), appliedPatchPrefix) - if filepath != "" { - conflicts.Add(filepath) - } - } - // only list part of conflicted files - if len(conflicts) >= gitrepo.MaxConflictedDetectFiles { - break - } - } - - if len(conflicts) > 0 { - pr.ConflictedFiles = make([]string, 0, len(conflicts)) - for key := range conflicts { - pr.ConflictedFiles = append(pr.ConflictedFiles, key) - } - } - - return nil - }). - Run(gitRepo.Ctx) - - // 9. Check if the found conflicted files is non-zero, "err" could be non-nil, so we should ignore it if we found conflicts. - // Note: `"err" could be non-nil` is due that if enable 3-way merge, it doesn't return any error on found conflicts. - if len(pr.ConflictedFiles) > 0 { - if conflict { - pr.Status = issues_model.PullRequestStatusConflict - log.Trace("Found %d files conflicted: %v", len(pr.ConflictedFiles), pr.ConflictedFiles) - - return true, nil - } - } else if err != nil { - return false, fmt.Errorf("git apply --check: %w", err) - } - return false, nil + pr.Status = issues_model.PullRequestStatusConflict + pr.ConflictedFiles = conflictFiles + log.Trace("Found %d files conflicted: %v", len(pr.ConflictedFiles), pr.ConflictedFiles) + return true, nil } // ErrFilePathProtected represents a "FilePathProtected" kind of error. diff --git a/tests/integration/pull_merge_test.go b/tests/integration/pull_merge_test.go index c2859e2e16..53709e6ff4 100644 --- a/tests/integration/pull_merge_test.go +++ b/tests/integration/pull_merge_test.go @@ -39,7 +39,6 @@ import ( pull_service "code.gitea.io/gitea/services/pull" repo_service "code.gitea.io/gitea/services/repository" commitstatus_service "code.gitea.io/gitea/services/repository/commitstatus" - files_service "code.gitea.io/gitea/services/repository/files" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -549,86 +548,6 @@ func TestCantFastForwardOnlyMergeDiverging(t *testing.T) { }) } -func TestConflictChecking(t *testing.T) { - onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) { - user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) - - // Create new clean repo to test conflict checking. - baseRepo, err := repo_service.CreateRepository(t.Context(), user, user, repo_service.CreateRepoOptions{ - Name: "conflict-checking", - Description: "Tempo repo", - AutoInit: true, - Readme: "Default", - DefaultBranch: "main", - }) - assert.NoError(t, err) - assert.NotEmpty(t, baseRepo) - - // create a commit on new branch. - _, err = files_service.ChangeRepoFiles(t.Context(), baseRepo, user, &files_service.ChangeRepoFilesOptions{ - Files: []*files_service.ChangeRepoFile{ - { - Operation: "create", - TreePath: "important_file", - ContentReader: strings.NewReader("Just a non-important file"), - }, - }, - Message: "Add a important file", - OldBranch: "main", - NewBranch: "important-secrets", - }) - assert.NoError(t, err) - - // create a commit on main branch. - _, err = files_service.ChangeRepoFiles(t.Context(), baseRepo, user, &files_service.ChangeRepoFilesOptions{ - Files: []*files_service.ChangeRepoFile{ - { - Operation: "create", - TreePath: "important_file", - ContentReader: strings.NewReader("Not the same content :P"), - }, - }, - Message: "Add a important file", - OldBranch: "main", - NewBranch: "main", - }) - assert.NoError(t, err) - - // create Pull to merge the important-secrets branch into main branch. - pullIssue := &issues_model.Issue{ - RepoID: baseRepo.ID, - Title: "PR with conflict!", - PosterID: user.ID, - Poster: user, - IsPull: true, - } - - pullRequest := &issues_model.PullRequest{ - HeadRepoID: baseRepo.ID, - BaseRepoID: baseRepo.ID, - HeadBranch: "important-secrets", - BaseBranch: "main", - HeadRepo: baseRepo, - BaseRepo: baseRepo, - Type: issues_model.PullRequestGitea, - } - prOpts := &pull_service.NewPullRequestOptions{Repo: baseRepo, Issue: pullIssue, PullRequest: pullRequest} - err = pull_service.NewPullRequest(t.Context(), prOpts) - assert.NoError(t, err) - - issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{Title: "PR with conflict!"}) - assert.NoError(t, issue.LoadPullRequest(t.Context())) - conflictingPR := issue.PullRequest - - // Ensure conflictedFiles is populated. - assert.Len(t, conflictingPR.ConflictedFiles, 1) - // Check if status is correct. - assert.Equal(t, issues_model.PullRequestStatusConflict, conflictingPR.Status) - // Ensure that mergeable returns false - assert.False(t, conflictingPR.Mergeable(t.Context())) - }) -} - func TestPullRetargetChildOnBranchDelete(t *testing.T) { onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) { session := loginUser(t, "user1") // FIXME: don't use admin user for testing