From 33b5a9a5077b64773a69259d12abe33ba01f9596 Mon Sep 17 00:00:00 2001 From: oppiliappan Date: Wed, 15 Oct 2025 09:05:57 +0000 Subject: [PATCH] appview: improve patch validation automatically adds a newline to patches that are missing one. --- patchutil/patchutil.go | 25 ++++++++++++++++++------- patchutil/patchutil_test.go | 25 +++++++++++++------------ appview/pulls/pulls.go | 40 ++++++++++++++++++---------------------- appview/state/router.go | 1 + appview/validator/patch.go | 25 +++++++++++++++++++++++++ knotserver/xrpc/merge_check.go | 2 ++ 6 file(s) changed, 77 insertion(s)(+), 41 deletion(s)(-) diff --git a/patchutil/patchutil.go b/patchutil/patchutil.go --- a/patchutil/patchutil.go +++ b/patchutil/patchutil.go @@ -1,6 +1,7 @@ package patchutil import ( + "errors" "fmt" "log" "os" @@ -42,14 +43,20 @@ // 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]) @@ -60,7 +67,7 @@ strings.HasPrefix(firstLine, "Index: ") || strings.HasPrefix(firstLine, "+++ ") || strings.HasPrefix(firstLine, "@@ ") { - return true + return nil } // check if it's format-patch @@ -70,12 +77,16 @@ // 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 { diff --git a/patchutil/patchutil_test.go b/patchutil/patchutil_test.go --- a/patchutil/patchutil_test.go +++ b/patchutil/patchutil_test.go @@ -1,6 +1,7 @@ package patchutil import ( + "errors" "reflect" "testing" ) @@ -9,17 +10,17 @@ 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`, @@ -31,7 +32,7 @@ -old line +new line context`, - expected: true, + expected: nil, }, { name: `valid patch starting with ---`, @@ -41,7 +42,7 @@ -old line +new line context`, - expected: true, + expected: nil, }, { name: `valid patch starting with Index`, @@ -53,7 +54,7 @@ -old line +new line context`, - expected: true, + expected: nil, }, { name: `valid patch starting with +++`, @@ -63,7 +64,7 @@ -old line +new line context`, - expected: true, + expected: nil, }, { name: `valid patch starting with @@`, @@ -72,7 +73,7 @@ +new line context `, - expected: true, + expected: nil, }, { name: `valid format patch`, @@ -90,14 +91,14 @@ +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 This is not a valid patch format`, - expected: false, + expected: FormatPatchError, }, { name: `not a patch at all`, @@ -105,14 +106,14 @@ 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) } }) diff --git a/appview/pulls/pulls.go b/appview/pulls/pulls.go --- a/appview/pulls/pulls.go +++ b/appview/pulls/pulls.go @@ -23,6 +23,7 @@ "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" @@ -47,6 +48,7 @@ notifier notify.Notifier enforcer *rbac.Enforcer logger *slog.Logger + validator *validator.Validator } func New( @@ -58,6 +60,7 @@ config *config.Config, notifier notify.Notifier, enforcer *rbac.Enforcer, + validator *validator.Validator, logger *slog.Logger, ) *Pulls { return &Pulls{ @@ -70,6 +73,7 @@ notifier: notifier, enforcer: enforcer, logger: logger, + validator: validator, } } @@ -965,7 +969,8 @@ 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 } @@ -982,7 +987,8 @@ } 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 } @@ -1073,7 +1079,8 @@ 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 } @@ -1337,7 +1344,8 @@ 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 } @@ -1754,23 +1762,6 @@ 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, @@ -1787,8 +1778,13 @@ 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 } diff --git a/appview/state/router.go b/appview/state/router.go --- a/appview/state/router.go +++ b/appview/state/router.go @@ -277,6 +277,7 @@ s.config, s.notifier, s.enforcer, + s.validator, log.SubLogger(s.logger, "pulls"), ) return pulls.Router(mw) diff --git a/appview/validator/patch.go b/appview/validator/patch.go new file mode 100644 --- /dev/null +++ b/appview/validator/patch.go @@ -0,0 +1,25 @@ +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 +} diff --git a/knotserver/xrpc/merge_check.go b/knotserver/xrpc/merge_check.go --- a/knotserver/xrpc/merge_check.go +++ b/knotserver/xrpc/merge_check.go @@ -81,6 +81,8 @@ } } + 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) -- tangled.sh