Fix merge related issues (#2012)

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)

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

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
 		}

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())

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)
 	}
 }

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 updates the size of a specific repository in the database (size is in KiB).
-		UpdateSize(ctx context.Context, id int64, repoSize int64) error
+		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
+		// ResetMergeCheckStatus resets the pull request's mergeability status to unchecked
-		UpdateMergeCheckStatus(ctx context.Context, targetRepo int64, targetBranch string, status enum.MergeCheckStatus) error
+		// 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

app/store/database/pullreq.go

@@ -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

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.
+// UpdateSize updates the size of a specific repository in the database (size is in KiB).
-func (s *RepoStore) UpdateSize(ctx context.Context, id int64, size int64) error {
+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)
 

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
 }
 

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

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,

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.

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)
 	}
 

git/repo.go

@@ -97,6 +97,7 @@ type GetRepositorySizeParams struct {
 }
 
 type GetRepositorySizeOutput struct {
+	// Total size of the repository in KiB.
 	Size int64
 }
 

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,

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"`

types/repo.go

@@ -35,7 +35,9 @@ type Repository struct {
 	Updated     int64  `json:"updated" yaml:"updated"`
 	Deleted     *int64 `json:"deleted,omitempty" yaml:"deleted"`
 
+	// 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:"-"`
@@ -83,7 +85,9 @@ func (r Repository) MarshalJSON() ([]byte, error) {
 type RepositorySizeInfo struct {
 	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"`
 }