appview: improve patch validation

automatically adds a newline to patches that are missing one.

appview/pulls/pulls.go

···
 	"tangled.org/core/appview/pages"
 	"tangled.org/core/appview/pages/markup"
 	"tangled.org/core/appview/reporesolver"
+	"tangled.org/core/appview/validator"
 	"tangled.org/core/appview/xrpcclient"
 	"tangled.org/core/idresolver"
 	"tangled.org/core/patchutil"
···
 	notifier     notify.Notifier
 	enforcer     *rbac.Enforcer
 	logger       *slog.Logger
+	validator    *validator.Validator
 }
 
 func New(
···
 	config *config.Config,
 	notifier notify.Notifier,
 	enforcer *rbac.Enforcer,
+	validator *validator.Validator,
 	logger *slog.Logger,
 ) *Pulls {
 	return &Pulls{
···
 		notifier:     notifier,
 		enforcer:     enforcer,
 		logger:       logger,
+		validator:    validator,
 	}
 }
 
···
 	patch := comparison.FormatPatchRaw
 	combined := comparison.CombinedPatchRaw
 
-	if !patchutil.IsPatchValid(patch) {
+	if err := s.validator.ValidatePatch(&patch); err != nil {
+		s.logger.Error("failed to validate patch", "err", err)
 		s.pages.Notice(w, "pull", "Invalid patch format. Please provide a valid diff.")
 		return
 	}
···
 }
 
 func (s *Pulls) handlePatchBasedPull(w http.ResponseWriter, r *http.Request, f *reporesolver.ResolvedRepo, user *oauth.User, title, body, targetBranch, patch string, isStacked bool) {
-	if !patchutil.IsPatchValid(patch) {
+	if err := s.validator.ValidatePatch(&patch); err != nil {
+		s.logger.Error("patch validation failed", "err", err)
 		s.pages.Notice(w, "pull", "Invalid patch format. Please provide a valid diff.")
 		return
 	}
···
 	patch := comparison.FormatPatchRaw
 	combined := comparison.CombinedPatchRaw
 
-	if !patchutil.IsPatchValid(patch) {
+	if err := s.validator.ValidatePatch(&patch); err != nil {
+		s.logger.Error("failed to validate patch", "err", err)
 		s.pages.Notice(w, "pull", "Invalid patch format. Please provide a valid diff.")
 		return
 	}
···
 		return
 	}
 
-	if patch == "" || !patchutil.IsPatchValid(patch) {
+	if err := s.validator.ValidatePatch(&patch); err != nil {
+		s.logger.Error("faield to validate patch", "err", err)
 		s.pages.Notice(w, "patch-error", "Invalid patch format. Please provide a valid git diff or format-patch.")
 		return
 	}
···
 	s.resubmitPullHelper(w, r, f, user, pull, patch, combined, sourceRev)
 }
 
-// validate a resubmission against a pull request
-func validateResubmittedPatch(pull *models.Pull, patch string) error {
-	if patch == "" {
-		return fmt.Errorf("Patch is empty.")
-	}
-
-	if patch == pull.LatestPatch() {
-		return fmt.Errorf("Patch is identical to previous submission.")
-	}
-
-	if !patchutil.IsPatchValid(patch) {
-		return fmt.Errorf("Invalid patch format. Please provide a valid diff.")
-	}
-
-	return nil
-}
-
 func (s *Pulls) resubmitPullHelper(
 	w http.ResponseWriter,
 	r *http.Request,
···
 		return
 	}
 
-	if err := validateResubmittedPatch(pull, patch); err != nil {
+	if err := s.validator.ValidatePatch(&patch); err != nil {
 		s.pages.Notice(w, "resubmit-error", err.Error())
+		return
+	}
+
+	if patch == pull.LatestPatch() {
+		s.pages.Notice(w, "resubmit-error", "Patch is identical to previous submission.")
 		return
 	}
 

appview/state/router.go

···
 		s.config,
 		s.notifier,
 		s.enforcer,
+		s.validator,
 		log.SubLogger(s.logger, "pulls"),
 	)
 	return pulls.Router(mw)

appview/validator/patch.go

···
+package validator
+
+import (
+	"fmt"
+	"strings"
+
+	"tangled.org/core/patchutil"
+)
+
+func (v *Validator) ValidatePatch(patch *string) error {
+	if patch == nil || *patch == "" {
+		return fmt.Errorf("patch is empty")
+	}
+
+	// add newline if not present to diff style patches
+	if !patchutil.IsFormatPatch(*patch) && !strings.HasSuffix(*patch, "\n") {
+		*patch = *patch + "\n"
+	}
+
+	if err := patchutil.IsPatchValid(*patch); err != nil {
+		return err
+	}
+
+	return nil
+}

knotserver/xrpc/merge_check.go

···
 		}
 	}
 
+	l.Debug("merge check response", "isConflicted", response.Is_conflicted, "err", response.Error, "conflicts", response.Conflicts)
+
 	w.Header().Set("Content-Type", "application/json")
 	w.WriteHeader(http.StatusOK)
 	json.NewEncoder(w).Encode(response)

patchutil/patchutil.go

···
 package patchutil
 
 import (
+	"errors"
 	"fmt"
 	"log"
 	"os"
···
 // IsPatchValid checks if the given patch string is valid.
 // It performs very basic sniffing for either git-diff or git-format-patch
 // header lines. For format patches, it attempts to extract and validate each one.
-func IsPatchValid(patch string) bool {
+var (
+	EmptyPatchError   error = errors.New("patch is empty")
+	GenericPatchError error = errors.New("patch is invalid")
+	FormatPatchError  error = errors.New("patch is not a valid format-patch")
+)
+
+func IsPatchValid(patch string) error {
 	if len(patch) == 0 {
-		return false
+		return EmptyPatchError
 	}
 
 	lines := strings.Split(patch, "\n")
 	if len(lines) < 2 {
-		return false
+		return EmptyPatchError
 	}
 
 	firstLine := strings.TrimSpace(lines[0])
···
 		strings.HasPrefix(firstLine, "Index: ") ||
 		strings.HasPrefix(firstLine, "+++ ") ||
 		strings.HasPrefix(firstLine, "@@ ") {
-		return true
+		return nil
 	}
 
 	// check if it's format-patch
···
 		// it's safe to say it's broken.
 		patches, err := ExtractPatches(patch)
 		if err != nil {
-			return false
+			return fmt.Errorf("%w: %w", FormatPatchError, err)
 		}
-		return len(patches) > 0
+		if len(patches) == 0 {
+			return EmptyPatchError
+		}
+
+		return nil
 	}
 
-	return false
+	return GenericPatchError
 }
 
 func IsFormatPatch(patch string) bool {

patchutil/patchutil_test.go

···
 package patchutil
 
 import (
+	"errors"
 	"reflect"
 	"testing"
 )
···
 	tests := []struct {
 		name     string
 		patch    string
-		expected bool
+		expected error
 	}{
 		{
 			name:     `empty patch`,
 			patch:    ``,
-			expected: false,
+			expected: EmptyPatchError,
 		},
 		{
 			name:     `single line patch`,
 			patch:    `single line`,
-			expected: false,
+			expected: EmptyPatchError,
 		},
 		{
 			name: `valid diff patch`,
···
 -old line
 +new line
 	context`,
-			expected: true,
+			expected: nil,
 		},
 		{
 			name: `valid patch starting with ---`,
···
 -old line
 +new line
 	context`,
-			expected: true,
+			expected: nil,
 		},
 		{
 			name: `valid patch starting with Index`,
···
 -old line
 +new line
 	context`,
-			expected: true,
+			expected: nil,
 		},
 		{
 			name: `valid patch starting with +++`,
···
 -old line
 +new line
 	context`,
-			expected: true,
+			expected: nil,
 		},
 		{
 			name: `valid patch starting with @@`,
···
 +new line
 	context
 `,
-			expected: true,
+			expected: nil,
 		},
 		{
 			name: `valid format patch`,
···
 +new content
 --
 2.48.1`,
-			expected: true,
+			expected: nil,
 		},
 		{
 			name: `invalid format patch`,
 			patch: `From 1234567890123456789012345678901234567890 Mon Sep 17 00:00:00 2001
 From: Author <[e-mail]>
 This is not a valid patch format`,
-			expected: false,
+			expected: FormatPatchError,
 		},
 		{
 			name: `not a patch at all`,
···
 just some
 random text
 that isn't a patch`,
-			expected: false,
+			expected: GenericPatchError,
 		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			result := IsPatchValid(tt.patch)
-			if result != tt.expected {
+			if !errors.Is(result, tt.expected) {
 				t.Errorf("IsPatchValid() = %v, want %v", result, tt.expected)
 			}
 		})