Allow fast-forward-only merge when signed commits are required

[krjakbrjak]

Fast-forward-only creates no Gitea commit, so skip the "can Gitea sign" precheck for it. Pre-check head-commit verification for styles that preserve user commits on the target (merge, fast-forward-only) so a PR with unsigned commits surfaces a localized error instead of a 500 at the pre-receive hook. The dropdown still shows every configured style; the avatar and signing warning toggle per selection via data-pull-merge-style.

Fixes #12272

Note: Admin force-merge does not bypass the new head-commits check. This matches the existing isSignedIfRequired behavior.

test-merge.webm

[silverwind]

Some suggestions from a review pass:

1. Extract a shared helper for the commit-walk — checkHeadCommitsVerifiedIfRequired in services/pull/check.go duplicates the commitsSigned logic in services/asymkey/sign.go:340-359 (same base/head/merge-base/CommitsBeforeUntil/ParseCommitWithSignature pattern). Factor both call sites to one helper to prevent drift.
2. Split the unrelated fix in web_src/js/features/common-fetch-action.ts (JSON.parse(respText) instead of a second await resp.text()) into its own PR — it's a real bug but unrelated to the feature, and bisectability suffers from bundling.
3. Reduce template repetition in templates/repo/issue/view_content/pull_merge_box.tmpl — {{if \$signingBlocksMerge}}data-pull-merge-style=\"fast-forward-only\"{{end}} is injected on multiple items. Compute it once into a variable (e.g. \$ffOnlyAttr) and reuse.
4. The avatar cascade now relies on \$avatarBaseClass + \$avatarColorFromSigning and a conditional double-render. It's correct but a future branch added to the cascade is easy to forget — a short comment on the cascade explaining the coupling would help.
5. Parse data-pull-merge-style once in web_src/js/components/PullRequestMergeForm.vue instead of split(',').map(s => s.trim()) on every style change. The attribute is static; parse at mount or normalize at template emission so plain split(',') suffices.
6. Add a one-line comment in the template explaining why manually-merged appears in the red-avatar list but not in the green fast-forward-only case — the intent isn't obvious at a glance.
7. In tests/integration/pull_merge_test.go, the inner t.Run closures shadow the outer req. Distinct names (ffReq, apiReq, …) would read clearer and avoid the shadow.
8. Confirm the intent that admin force-merge does not bypass ErrHeadCommitsNotAllVerified. It's consistent with today's behavior around isSignedIfRequired, but worth being explicit about in the PR description so reviewers don't have to reason about it.
9. Consider whether the API should return 400 (like the web router does) rather than 405 for ErrHeadCommitsNotAllVerified — 405 "Method Not Allowed" is a stretch semantically for a precondition failure, though it matches the neighboring ErrWontSign mapping.

---

Review written with the help of Claude Opus 4.7.

[krjakbrjak]

Some suggestions from a review pass:

1. Extract a shared helper for the commit-walk — checkHeadCommitsVerifiedIfRequired in services/pull/check.go duplicates the commitsSigned logic in services/asymkey/sign.go:340-359 (same base/head/merge-base/CommitsBeforeUntil/ParseCommitWithSignature pattern). Factor both call sites to one helper to prevent drift.
2. Split the unrelated fix in web_src/js/features/common-fetch-action.ts (JSON.parse(respText) instead of a second await resp.text()) into its own PR — it's a real bug but unrelated to the feature, and bisectability suffers from bundling.
3. Reduce template repetition in templates/repo/issue/view_content/pull_merge_box.tmpl — {{if \$signingBlocksMerge}}data-pull-merge-style=\"fast-forward-only\"{{end}} is injected on multiple items. Compute it once into a variable (e.g. \$ffOnlyAttr) and reuse.
4. The avatar cascade now relies on \$avatarBaseClass + \$avatarColorFromSigning and a conditional double-render. It's correct but a future branch added to the cascade is easy to forget — a short comment on the cascade explaining the coupling would help.
5. Parse data-pull-merge-style once in web_src/js/components/PullRequestMergeForm.vue instead of split(',').map(s => s.trim()) on every style change. The attribute is static; parse at mount or normalize at template emission so plain split(',') suffices.
6. Add a one-line comment in the template explaining why manually-merged appears in the red-avatar list but not in the green fast-forward-only case — the intent isn't obvious at a glance.
7. In tests/integration/pull_merge_test.go, the inner t.Run closures shadow the outer req. Distinct names (ffReq, apiReq, …) would read clearer and avoid the shadow.
8. Confirm the intent that admin force-merge does not bypass ErrHeadCommitsNotAllVerified. It's consistent with today's behavior around isSignedIfRequired, but worth being explicit about in the PR description so reviewers don't have to reason about it.
9. Consider whether the API should return 400 (like the web router does) rather than 405 for ErrHeadCommitsNotAllVerified — 405 "Method Not Allowed" is a stretch semantically for a precondition failure, though it matches the neighboring ErrWontSign mapping.

Review written with the help of Claude Opus 4.7.

Thanks for the review @silverwind . Went through the list:

1. Pulled the commit walk out into AllHeadCommitsVerified.
2. Moved the ff-only attribute into one variable, $ffOnlyStyleAttr.
3. Added a note on the cascade: $avatarColorFromSigning should stay
   false for any new branch.
4. Dropped the .map(s => s.trim()). The template emits the attribute
   without spaces, so plain split(',') does the job.
5. Added a short comment on why manually-merged is in the red list
   but not the green one.
6. Renamed the req variables in the test so they don't shadow each
   other: createPRReq, mergeReq, apiReq.
7. Mentioned in the PR description: admin force-merge does not skip
   the new check, same as isSignedIfRequired.

A couple I'd like to push back on:

2. I kept the fetch-action fix in this PR. The "body stream already
   read" error was exactly what hid the new signing message in the UI,
   so shipping them together makes sense to me. Happy to split if you
   still want me to — it's one line.

3. Left the API status as 405. The other merge errors in the same
   handler already return 405, and changing only this one to 400
   would make the API less predictable. A full cleanup feels like a
   separate PR.

[silverwind]

I'll push a few cleanups.

[wxiaoguang]

The template is a mess and unmaintainable.

Need to refactor and clean up the related code first

-> Refactor pull request view #37380

[krjakbrjak]

The template is a mess and unmaintainable.

Need to refactor and clean up the related code first

-> Refactor pull request view #37380

Sounds good. Maybe it would make sense to drop the template changes from this PR and keep only the backend parts (the shared AllHeadCommitsVerified helper, the signing precheck consolidation, ErrHeadCommitsNotAllVerified with its API mapping, and the integration tests)? That way this one could move forward independently of your refactor, and I'd revisit the merge-box UI bits once the refactor lands. What do you think @wxiaoguang ?

[wxiaoguang]

Maybe it would make sense to drop the template changes from this PR and keep only the backend parts (the shared AllHeadCommitsVerified helper, the signing precheck consolidation, ErrHeadCommitsNotAllVerified with its API mapping, and the integration tests)

It sounds good, thank you!

[krjakbrjak]

Maybe it would make sense to drop the template changes from this PR and keep only the backend parts (the shared AllHeadCommitsVerified helper, the signing precheck consolidation, ErrHeadCommitsNotAllVerified with its API mapping, and the integration tests)

It sounds good, thank you!

Reverted the templates. only the backend part is kept. Should anything else be done here in your opinion?