feat: unrealted histories error handling (#2642)

* comments added
* return proper status and message for unrelated history
* code cleanup
* requested changes
* feat: unrealted histories error handling

app/api/controller/repo/merge_check.go

@@ -21,6 +21,7 @@ import (
 	"github.com/harness/gitness/app/api/controller"
 	"github.com/harness/gitness/app/auth"
 	"github.com/harness/gitness/git"
+	"github.com/harness/gitness/git/api"
 	"github.com/harness/gitness/types/enum"
 )
 
@@ -57,6 +58,14 @@ func (c *Controller) MergeCheck(
 		HeadBranch:  info.HeadRef,
 	})
 	if err != nil {
+		// git.Merge works with commits and error is not user-friendly
+		// and here we are modify base-ref and head-ref with user input
+		// values.
+		if uErr := api.AsUnrelatedHistoriesError(err); uErr != nil {
+			uErr.BaseRef = info.BaseRef
+			uErr.HeadRef = info.HeadRef
+			return MergeCheck{}, uErr
+		}
 		return MergeCheck{}, fmt.Errorf("merge check execution failed: %w", err)
 	}
 	if len(mergeOutput.ConflictFiles) > 0 {

app/api/handler/repo/diff.go

@@ -23,6 +23,7 @@ import (
 	"github.com/harness/gitness/app/api/controller/repo"
 	"github.com/harness/gitness/app/api/render"
 	"github.com/harness/gitness/app/api/request"
+	"github.com/harness/gitness/app/api/usererror"
 	"github.com/harness/gitness/errors"
 	gittypes "github.com/harness/gitness/git/api"
 )
@@ -110,6 +111,13 @@ func HandleDiffStats(repoCtrl *repo.Controller) http.HandlerFunc {
 		path := request.GetOptionalRemainderFromPath(r)
 
 		output, err := repoCtrl.DiffStats(ctx, session, repoRef, path)
+		if uErr := gittypes.AsUnrelatedHistoriesError(err); uErr != nil {
+			render.JSON(w, http.StatusOK, &usererror.Error{
+				Message: uErr.Error(),
+				Values:  uErr.Map(),
+			})
+			return
+		}
 		if err != nil {
 			render.TranslatedUserError(ctx, w, err)
 			return

app/api/render/render.go

@@ -22,7 +22,9 @@ import (
 	"os"
 	"strconv"
 
+	"github.com/harness/gitness/app/api/usererror"
 	"github.com/harness/gitness/errors"
+	"github.com/harness/gitness/git/api"
 	"github.com/harness/gitness/types"
 
 	"github.com/rs/zerolog/log"
@@ -75,6 +77,15 @@ func JSONArrayDynamic[T comparable](ctx context.Context, w http.ResponseWriter,
 		}
 
 		if err != nil {
+			// based on discussion unrelated histories error should return
+			// StatusOK on diff.
+			if uErr := api.AsUnrelatedHistoriesError(err); uErr != nil {
+				JSON(w, http.StatusOK, &usererror.Error{
+					Message: uErr.Error(),
+					Values:  uErr.Map(),
+				})
+				return
+			}
 			// User canceled the request - no need to do anything
 			if errors.Is(err, context.Canceled) {
 				return

app/api/usererror/translate.go

@@ -25,6 +25,7 @@ import (
 	"github.com/harness/gitness/app/services/webhook"
 	"github.com/harness/gitness/blob"
 	"github.com/harness/gitness/errors"
+	"github.com/harness/gitness/git/api"
 	"github.com/harness/gitness/lock"
 	"github.com/harness/gitness/store"
 	"github.com/harness/gitness/types/check"
@@ -37,6 +38,7 @@ func Translate(ctx context.Context, err error) *Error {
 		rError                   *Error
 		checkError               *check.ValidationError
 		appError                 *errors.Error
+		unrelatedHistoriesErr    *api.UnrelatedHistoriesError
 		maxBytesErr              *http.MaxBytesError
 		codeOwnersTooLargeError  *codeowners.TooLargeError
 		codeOwnersFileParseError *codeowners.FileParseError
@@ -96,6 +98,12 @@ func Translate(ctx context.Context, err error) *Error {
 			appError.Message,
 			appError.Details,
 		)
+	case errors.As(err, &unrelatedHistoriesErr):
+		return NewWithPayload(
+			http.StatusBadRequest,
+			err.Error(),
+			unrelatedHistoriesErr.Map(),
+		)
 
 	// webhook errors
 	case errors.Is(err, webhook.ErrWebhookNotRetriggerable):

git/api/errors.go

@@ -19,7 +19,6 @@ import (
 	"strings"
 
 	"github.com/harness/gitness/errors"
-	"github.com/harness/gitness/git/enum"
 
 	"github.com/rs/zerolog/log"
 )
@@ -133,33 +132,64 @@ func processGitErrorf(err error, format string, args ...interface{}) error {
 		return errors.NotFound("repository not found")
 	case strings.Contains(err.Error(), "reference already exists"):
 		return errors.Conflict("reference already exists")
+	case strings.Contains(err.Error(), "no merge base"):
+		if len(args) >= 2 {
+			return &UnrelatedHistoriesError{
+				BaseRef: strings.TrimSpace(args[0].(string)),
+				HeadRef: strings.TrimSpace(args[1].(string)),
+			}
+		}
+		return &UnrelatedHistoriesError{}
 	default:
 		return fallbackErr
 	}
 }
 
-// MergeUnrelatedHistoriesError represents an error if merging fails due to unrelated histories.
-type MergeUnrelatedHistoriesError struct {
-	Method enum.MergeMethod
-	StdOut string
-	StdErr string
-	Err    error
+type UnrelatedHistoriesError struct {
+	BaseRef string
+	HeadRef string
 }
 
-func IsMergeUnrelatedHistoriesError(err error) bool {
-	return errors.Is(err, &MergeUnrelatedHistoriesError{})
+func (e *UnrelatedHistoriesError) Map() map[string]any {
+	return map[string]any{
+		"base_ref": e.BaseRef,
+		"head_ref": e.HeadRef,
+	}
 }
 
-func (e *MergeUnrelatedHistoriesError) Error() string {
-	return fmt.Sprintf("Merge UnrelatedHistories Error: %v: %s\n%s", e.Err, e.StdErr, e.StdOut)
+func (e *UnrelatedHistoriesError) Is(err error) bool {
+	var target *UnrelatedHistoriesError
+	ok := errors.As(err, &target)
+	if !ok {
+		return false
+	}
+
+	return target.BaseRef == e.BaseRef && target.HeadRef == e.HeadRef
 }
 
-func (e *MergeUnrelatedHistoriesError) Unwrap() error {
-	return e.Err
+func (e *UnrelatedHistoriesError) Error() string {
+	if e.BaseRef == "" || e.HeadRef == "" {
+		return "unrelated commit histories error"
+	}
+	// remove branch and tag prefixes, original remains in struct fields
+	// we just need to remove first occurrence.
+	baseRef := strings.TrimPrefix(e.BaseRef, BranchPrefix)
+	baseRef = strings.TrimPrefix(baseRef, TagPrefix)
+	headRef := strings.TrimPrefix(e.HeadRef, BranchPrefix)
+	headRef = strings.TrimPrefix(headRef, TagPrefix)
+	return fmt.Sprintf("%s and %s have entirely different commit histories.", baseRef, headRef)
 }
 
-//nolint:errorlint // the purpose of this method is to check whether the target itself if of this type.
-func (e *MergeUnrelatedHistoriesError) Is(target error) bool {
-	_, ok := target.(*MergeUnrelatedHistoriesError)
-	return ok
+// IsUnrelatedHistoriesError checks if an error is a UnrelatedHistoriesError.
+func IsUnrelatedHistoriesError(err error) bool {
+	return AsUnrelatedHistoriesError(err) != nil
+}
+
+func AsUnrelatedHistoriesError(err error) *UnrelatedHistoriesError {
+	var target *UnrelatedHistoriesError
+	ok := errors.As(err, &target)
+	if !ok {
+		return nil
+	}
+	return target
 }

git/api/merge.go

@@ -56,15 +56,26 @@ func (g *Git) GetMergeBase(
 		}
 	}
 
-	stdout := &bytes.Buffer{}
 	cmd := command.New("merge-base",
 		command.WithArg(base, head),
 	)
+
+	stdout, stderr := new(bytes.Buffer), new(bytes.Buffer)
 	err := cmd.Run(ctx,
 		command.WithDir(repoPath),
 		command.WithStdout(stdout),
+		command.WithStderr(stderr),
 	)
 	if err != nil {
+		// git merge-base rev1 rev2
+		// if there is unrelated history then stderr is empty with
+		// exit code 1. This cannot be handled in processGitErrorf because stderr is empty.
+		if command.AsError(err).IsExitCode(1) && stderr.Len() == 0 {
+			return sha.None, "", &UnrelatedHistoriesError{
+				BaseRef: base,
+				HeadRef: head,
+			}
+		}
 		return sha.None, "", processGitErrorf(err, "failed to get merge-base [%s, %s]", base, head)
 	}
 

git/merge.go

@@ -184,12 +184,12 @@ func (s *Service) Merge(ctx context.Context, params *MergeParams) (MergeOutput,
 
 	baseCommitSHA, err := s.git.GetFullCommitID(ctx, repoPath, params.BaseBranch)
 	if err != nil {
-		return MergeOutput{}, fmt.Errorf("failed to get merge base branch commit SHA: %w", err)
+		return MergeOutput{}, fmt.Errorf("failed to get base branch commit SHA: %w", err)
 	}
 
 	headCommitSHA, err := s.git.GetFullCommitID(ctx, repoPath, params.HeadBranch)
 	if err != nil {
-		return MergeOutput{}, fmt.Errorf("failed to get merge base branch commit SHA: %w", err)
+		return MergeOutput{}, fmt.Errorf("failed to get head branch commit SHA: %w", err)
 	}
 
 	if !params.HeadExpectedSHA.IsEmpty() && !params.HeadExpectedSHA.Equal(headCommitSHA) {