diff --git a/app/api/controller/pullreq/merge.go b/app/api/controller/pullreq/merge.go index 1573824a1..356931b80 100644 --- a/app/api/controller/pullreq/merge.go +++ b/app/api/controller/pullreq/merge.go @@ -257,7 +257,7 @@ func (c *Controller) Merge( pr.MergeCheckStatus = enum.MergeCheckStatusMergeable pr.MergeBaseSHA = mergeOutput.MergeBaseSHA.String() pr.MergeTargetSHA = ptr.String(mergeOutput.BaseSHA.String()) - pr.MergeSHA = ptr.String(mergeOutput.MergeSHA.String()) + pr.MergeSHA = nil // dry-run doesn't create a merge commit so output is empty. pr.MergeConflicts = nil } pr.Stats.DiffStats = types.NewDiffStats(mergeOutput.CommitCount, mergeOutput.ChangedFileCount) diff --git a/app/api/controller/pullreq/pr_state.go b/app/api/controller/pullreq/pr_state.go index 52091aa6d..7705c7741 100644 --- a/app/api/controller/pullreq/pr_state.go +++ b/app/api/controller/pullreq/pr_state.go @@ -147,6 +147,7 @@ func (c *Controller) State(ctx context.Context, pr.MergeCheckStatus = enum.MergeCheckStatusUnchecked pr.MergeSHA = nil pr.MergeConflicts = nil + pr.MergeTargetSHA = nil pr.Closed = &pr.Edited case changeReopen: pr.SourceSHA = sourceSHA diff --git a/app/services/pullreq/handlers_branch.go b/app/services/pullreq/handlers_branch.go index 1f817fe54..3bb84ddc6 100644 --- a/app/services/pullreq/handlers_branch.go +++ b/app/services/pullreq/handlers_branch.go @@ -51,7 +51,7 @@ func (s *Service) triggerPREventOnBranchUpdate(ctx context.Context, // and need to run mergeable check even nothing was changed on feature1, same applies to main if someone // push new commit to main then develop should merge status should be unchecked. if branch, err := getBranchFromRef(event.Payload.Ref); err == nil { - err = s.pullreqStore.UpdateMergeCheckStatus(ctx, event.Payload.RepoID, branch, enum.MergeCheckStatusUnchecked) + err = s.pullreqStore.ResetMergeCheckStatus(ctx, event.Payload.RepoID, branch) if err != nil { return err } diff --git a/app/services/pullreq/handlers_mergeable.go b/app/services/pullreq/handlers_mergeable.go index b309fd909..1d477834d 100644 --- a/app/services/pullreq/handlers_mergeable.go +++ b/app/services/pullreq/handlers_mergeable.go @@ -36,7 +36,6 @@ import ( const ( cancelMergeCheckKey = "cancel_merge_check_for_sha" - nilSHA = "0000000000000000000000000000000000000000" ) // mergeCheckOnCreated handles pull request Created events. @@ -48,7 +47,7 @@ func (s *Service) mergeCheckOnCreated(ctx context.Context, ctx, event.Payload.TargetRepoID, event.Payload.Number, - nilSHA, + sha.Nil.String(), event.Payload.SourceSHA, ) } @@ -76,7 +75,7 @@ func (s *Service) mergeCheckOnReopen(ctx context.Context, ctx, event.Payload.TargetRepoID, event.Payload.Number, - "", + sha.None.String(), event.Payload.SourceSHA, ) } @@ -134,16 +133,6 @@ func (s *Service) updateMergeData( return fmt.Errorf("failed to get pull request number %d: %w", prNum, err) } - return s.updateMergeDataInner(ctx, pr, oldSHA, newSHA) -} - -//nolint:funlen // refactor if required. -func (s *Service) updateMergeDataInner( - ctx context.Context, - pr *types.PullReq, - oldSHA string, - newSHA string, -) error { // TODO: Merge check should not update the merge base. // TODO: Instead it should accept it as an argument and fail if it doesn't match. // Then is would not longer be necessary to cancel already active mergeability checks. @@ -214,9 +203,12 @@ func (s *Service) updateMergeDataInner( CommitterDate: &now, }) if errors.AsStatus(err) == errors.StatusPreconditionFailed { - return events.NewDiscardEventErrorf("Source branch '%s' is not on SHA '%s' anymore.", + return events.NewDiscardEventErrorf("Source branch %q is not on SHA %q anymore.", pr.SourceBranch, newSHA) } + if err != nil { + return fmt.Errorf("failed to run git merge with base %q and head %q: %w", pr.TargetBranch, pr.SourceBranch, err) + } // Update DB in both cases (failure or success) _, err = s.pullreqStore.UpdateOptLock(ctx, pr, func(pr *types.PullReq) error { @@ -224,7 +216,7 @@ func (s *Service) updateMergeDataInner( return events.NewDiscardEventErrorf("PR SHA %s is newer than %s", pr.SourceSHA, newSHA) } - if mergeOutput.MergeSHA.IsEmpty() || len(mergeOutput.ConflictFiles) > 0 { + if len(mergeOutput.ConflictFiles) > 0 { pr.MergeCheckStatus = enum.MergeCheckStatusConflict pr.MergeBaseSHA = mergeOutput.MergeBaseSHA.String() pr.MergeTargetSHA = ptr.String(mergeOutput.BaseSHA.String()) diff --git a/app/services/repo/reposize.go b/app/services/repo/reposize.go index 6507e05de..2791196e3 100644 --- a/app/services/repo/reposize.go +++ b/app/services/repo/reposize.go @@ -94,7 +94,7 @@ func worker(ctx context.Context, s *SizeCalculator, wg *sync.WaitGroup, taskCh < for sizeInfo := range taskCh { log := log.Ctx(ctx).With().Str("repo_git_uid", sizeInfo.GitUID).Int64("repo_id", sizeInfo.ID).Logger() - log.Debug().Msgf("previous repo size: %d", sizeInfo.Size) + log.Debug().Msgf("previous repo size: %d KiB", sizeInfo.Size) sizeOut, err := s.git.GetRepositorySize( ctx, @@ -113,6 +113,6 @@ func worker(ctx context.Context, s *SizeCalculator, wg *sync.WaitGroup, taskCh < continue } - log.Debug().Msgf("new repo size: %d", sizeOut.Size) + log.Debug().Msgf("new repo size: %d KiB", sizeOut.Size) } } diff --git a/app/store/database.go b/app/store/database.go index 2c4415c0f..d75bf8654 100644 --- a/app/store/database.go +++ b/app/store/database.go @@ -213,8 +213,8 @@ type ( // Update the repo details. Update(ctx context.Context, repo *types.Repository) error - // Update the repo size. - UpdateSize(ctx context.Context, id int64, repoSize int64) error + // UpdateSize updates the size of a specific repository in the database (size is in KiB). + UpdateSize(ctx context.Context, id int64, sizeInKiB int64) error // Get the repo size. GetSize(ctx context.Context, id int64) (int64, error) @@ -341,8 +341,9 @@ type ( // It will set new values to the ActivitySeq, Version and Updated fields. UpdateActivitySeq(ctx context.Context, pr *types.PullReq) (*types.PullReq, error) - // Update all PR where target branch points to new SHA - UpdateMergeCheckStatus(ctx context.Context, targetRepo int64, targetBranch string, status enum.MergeCheckStatus) error + // ResetMergeCheckStatus resets the pull request's mergeability status to unchecked + // for all prs with target branch pointing to targetBranch. + ResetMergeCheckStatus(ctx context.Context, targetRepo int64, targetBranch string) error // Delete the pull request. Delete(ctx context.Context, id int64) error diff --git a/app/store/database/pullreq.go b/app/store/database/pullreq.go index 08b22105a..7c61ff4a7 100644 --- a/app/store/database/pullreq.go +++ b/app/store/database/pullreq.go @@ -290,7 +290,7 @@ func (s *PullReqStore) Update(ctx context.Context, pr *types.PullReq) error { ,pullreq_merge_base_sha = :pullreq_merge_base_sha ,pullreq_merge_sha = :pullreq_merge_sha ,pullreq_merge_conflicts = :pullreq_merge_conflicts - ,pullreq_commit_count = :pullreq_commit_count + ,pullreq_commit_count = :pullreq_commit_count ,pullreq_file_count = :pullreq_file_count WHERE pullreq_id = :pullreq_id AND pullreq_version = :pullreq_version - 1` @@ -361,20 +361,23 @@ func (s *PullReqStore) UpdateActivitySeq(ctx context.Context, pr *types.PullReq) }) } -// UpdateMergeCheckStatus updates the pull request's mergeability status +// ResetMergeCheckStatus resets the pull request's mergeability status to unchecked // for all pr which target branch points to targetBranch. -func (s *PullReqStore) UpdateMergeCheckStatus( +func (s *PullReqStore) ResetMergeCheckStatus( ctx context.Context, targetRepo int64, targetBranch string, - status enum.MergeCheckStatus, ) error { + // NOTE: keep pullreq_merge_base_sha on old value as it's a required field. const query = ` UPDATE pullreqs SET pullreq_updated = $1 - ,pullreq_merge_check_status = $2 ,pullreq_version = pullreq_version + 1 + ,pullreq_merge_check_status = $2 + ,pullreq_merge_target_sha = NULL + ,pullreq_merge_sha = NULL + ,pullreq_merge_conflicts = NULL ,pullreq_commit_count = NULL ,pullreq_file_count = NULL WHERE pullreq_target_repo_id = $3 AND @@ -385,10 +388,10 @@ func (s *PullReqStore) UpdateMergeCheckStatus( now := time.Now().UnixMilli() - _, err := db.ExecContext(ctx, query, now, status, targetRepo, targetBranch, + _, err := db.ExecContext(ctx, query, now, enum.MergeCheckStatusUnchecked, targetRepo, targetBranch, enum.PullReqStateClosed, enum.PullReqStateMerged) if err != nil { - return database.ProcessSQLErrorf(ctx, err, "Failed to update mergeable status check %s in pull requests", status) + return database.ProcessSQLErrorf(ctx, err, "Failed to reset mergeable status check in pull requests") } return nil diff --git a/app/store/database/repo.go b/app/store/database/repo.go index deb36c0ae..0ca67d6db 100644 --- a/app/store/database/repo.go +++ b/app/store/database/repo.go @@ -349,11 +349,11 @@ func (s *RepoStore) Update(ctx context.Context, repo *types.Repository) error { return nil } -// UpdateSize updates the size of a specific repository in the database. -func (s *RepoStore) UpdateSize(ctx context.Context, id int64, size int64) error { +// UpdateSize updates the size of a specific repository in the database (size is in KiB). +func (s *RepoStore) UpdateSize(ctx context.Context, id int64, sizeInKiB int64) error { stmt := database.Builder. Update("repositories"). - Set("repo_size", size). + Set("repo_size", sizeInKiB). Set("repo_size_updated", time.Now().UnixMilli()). Where("repo_id = ? AND repo_deleted IS NULL", id) diff --git a/errors/status.go b/errors/status.go index a63e03ab9..56d4ed8dc 100644 --- a/errors/status.go +++ b/errors/status.go @@ -63,6 +63,10 @@ func (e *Error) Unwrap() error { // Error implements the error interface. func (e *Error) Error() string { + if e.Err != nil { + return fmt.Sprintf("%s: %s", e.Message, e.Err) + } + return e.Message } diff --git a/git/interface.go b/git/interface.go index 6260b6bd4..90d4d5ac1 100644 --- a/git/interface.go +++ b/git/interface.go @@ -39,6 +39,7 @@ type Interface interface { GetRef(ctx context.Context, params GetRefParams) (GetRefResponse, error) PathsDetails(ctx context.Context, params PathsDetailsParams) (PathsDetailsOutput, error) + // GetRepositorySize calculates the size of a repo in KiB. GetRepositorySize(ctx context.Context, params *GetRepositorySizeParams) (*GetRepositorySizeOutput, error) // UpdateRef creates, updates or deletes a git ref. If the OldValue is defined it must match the reference value // prior to the call. To remove a ref use the zero ref as the NewValue. To require the creation of a new one and diff --git a/git/merge.go b/git/merge.go index 7f338a3d8..8e984fa27 100644 --- a/git/merge.go +++ b/git/merge.go @@ -293,10 +293,10 @@ func (s *Service) Merge(ctx context.Context, params *MergeParams) (MergeOutput, mergeMsg, mergeBaseCommitSHA, baseCommitSHA, headCommitSHA) if err != nil { - return MergeOutput{}, errors.Internal(err, "failed to merge %q to %q in %q using the %q merge method.", + return MergeOutput{}, errors.Internal(err, "failed to merge %q to %q in %q using the %q merge method", params.HeadBranch, params.BaseBranch, params.RepoUID, mergeMethod) } - if len(conflicts) != 0 { + if len(conflicts) > 0 { return MergeOutput{ BaseSHA: baseCommitSHA, HeadSHA: headCommitSHA, diff --git a/git/merge/check.go b/git/merge/check.go index 3d05df799..cf71008a3 100644 --- a/git/merge/check.go +++ b/git/merge/check.go @@ -22,6 +22,7 @@ import ( "github.com/harness/gitness/errors" "github.com/harness/gitness/git/command" + "github.com/harness/gitness/git/sharedrepo" "github.com/rs/zerolog/log" ) @@ -72,11 +73,14 @@ func FindConflicts( "Failed to find conflicts between %s and %s: Operation blocked. Status=%d", base, head, status) } + treeSHA = lines[1] if status == 1 { - return true, lines[1], nil, nil // all good, merge possible, no conflicts found + return true, treeSHA, nil, nil // all good, merge possible, no conflicts found } - return false, lines[1], lines[2:], nil // conflict found, list of conflicted files returned + conflicts = sharedrepo.CleanupMergeConflicts(lines[2:]) + + return false, treeSHA, conflicts, nil // conflict found, list of conflicted files returned } // CommitCount returns number of commits between the two git revisions. diff --git a/git/merge/merge.go b/git/merge/merge.go index 9fe0df135..7afb9cd37 100644 --- a/git/merge/merge.go +++ b/git/merge/merge.go @@ -16,6 +16,7 @@ package merge import ( "context" + "errors" "fmt" "github.com/harness/gitness/git/api" @@ -26,6 +27,11 @@ import ( "github.com/rs/zerolog/log" ) +var ( + // errConflict is used to error out of sharedrepo Run method without erroring out of merge in case of conflicts. + errConflict = errors.New("conflict") +) + // Func represents a merge method function. The concrete merge implementation functions must have this signature. type Func func( ctx context.Context, @@ -93,7 +99,7 @@ func mergeInternal( } if len(conflicts) > 0 { - return nil + return errConflict } parents := make([]sha.SHA, 0, 2) @@ -113,7 +119,7 @@ func mergeInternal( return nil }) - if err != nil { + if err != nil && !errors.Is(err, errConflict) { return sha.None, nil, fmt.Errorf("merge method=merge squash=%t: %w", squash, err) } @@ -173,7 +179,7 @@ func Rebase( return fmt.Errorf("failed to merge tree in rebase merge: %w", err) } if len(conflicts) > 0 { - return nil + return errConflict } // Drop any commit which after being rebased would be empty. @@ -203,7 +209,7 @@ func Rebase( return nil }) - if err != nil { + if err != nil && !errors.Is(err, errConflict) { return sha.None, nil, fmt.Errorf("merge method=rebase: %w", err) } diff --git a/git/repo.go b/git/repo.go index b6c96dc2f..9d3bb4d05 100644 --- a/git/repo.go +++ b/git/repo.go @@ -97,6 +97,7 @@ type GetRepositorySizeParams struct { } type GetRepositorySizeOutput struct { + // Total size of the repository in KiB. Size int64 } diff --git a/git/sharedrepo/sharedrepo.go b/git/sharedrepo/sharedrepo.go index e634ca294..5e39f3696 100644 --- a/git/sharedrepo/sharedrepo.go +++ b/git/sharedrepo/sharedrepo.go @@ -337,12 +337,27 @@ func (r *SharedRepo) MergeTree( log.Ctx(ctx).Err(err).Str("output", output).Msg("unexpected output of merge-tree in shared repo") return sha.None, nil, fmt.Errorf("unexpected output of merge-tree in shared repo: %w", err) } - return sha.Must(lines[0]), lines[1:], nil + + treeSHA := sha.Must(lines[0]) + conflicts := CleanupMergeConflicts(lines[1:]) + + return treeSHA, conflicts, nil } return sha.None, nil, fmt.Errorf("failed to merge-tree in shared repo: %w", err) } +func CleanupMergeConflicts(conflicts []string) []string { + out := make([]string, 0, len(conflicts)) + for _, conflict := range conflicts { + conflict = strings.TrimSpace(conflict) + if conflict != "" { + out = append(out, conflict) + } + } + return out +} + // CommitTree creates a commit from a given tree for the user with provided message. func (r *SharedRepo) CommitTree( ctx context.Context, diff --git a/types/pullreq.go b/types/pullreq.go index 455111121..9b0bd7d96 100644 --- a/types/pullreq.go +++ b/types/pullreq.go @@ -54,7 +54,7 @@ type PullReq struct { MergeCheckStatus enum.MergeCheckStatus `json:"merge_check_status"` MergeTargetSHA *string `json:"merge_target_sha"` MergeBaseSHA string `json:"merge_base_sha"` - MergeSHA *string `json:"merge_sha"` + MergeSHA *string `json:"-"` // TODO: either remove or ensure it's being set (merge dry-run) MergeConflicts []string `json:"merge_conflicts,omitempty"` Author PrincipalInfo `json:"author"` diff --git a/types/repo.go b/types/repo.go index ec1d19f5e..4020c40ad 100644 --- a/types/repo.go +++ b/types/repo.go @@ -35,7 +35,9 @@ type Repository struct { Updated int64 `json:"updated" yaml:"updated"` Deleted *int64 `json:"deleted,omitempty" yaml:"deleted"` - Size int64 `json:"size" yaml:"size"` + // Size of the repository in KiB. + Size int64 `json:"size" yaml:"size"` + // SizeUpdated is the time when the Size was last updated. SizeUpdated int64 `json:"size_updated" yaml:"size_updated"` GitUID string `json:"-" yaml:"-"` @@ -81,10 +83,12 @@ func (r Repository) MarshalJSON() ([]byte, error) { } type RepositorySizeInfo struct { - ID int64 `json:"id"` - GitUID string `json:"git_uid"` - Size int64 `json:"size"` - SizeUpdated int64 `json:"size_updated"` + ID int64 `json:"id"` + GitUID string `json:"git_uid"` + // Size of the repository in KiB. + Size int64 `json:"size"` + // SizeUpdated is the time when the Size was last updated. + SizeUpdated int64 `json:"size_updated"` } func (r Repository) GetGitUID() string {