Skip to content

feat(api): support ref suffixes in compare#38148

Open
eyupcanakman wants to merge 11 commits into
go-gitea:mainfrom
eyupcanakman:feat/compare-ref-suffix
Open

feat(api): support ref suffixes in compare#38148
eyupcanakman wants to merge 11 commits into
go-gitea:mainfrom
eyupcanakman:feat/compare-ref-suffix

Conversation

@eyupcanakman

Copy link
Copy Markdown

Compare API requests with a ^ or ~N revision suffix (for example compare/main...feature^) were rejected with 400 Unsupported comparison syntax: ref with suffix. The fix resolves the suffix to a commit before comparing, so base...head^ and ~N work on either side, the same as git.

Only ^/~N navigation is resolved. Pull request creation still requires plain branch refs, and the web compare page keeps rejecting suffixes since its branch selectors need separate UI work.

Closes #33943

Assisted-by: Claude Code:claude-opus-4-8

Resolve a `^` or `~N` revision suffix to a commit before comparing, so the compare API accepts `base...head^` and `~N` on either ref.
Pull request creation still requires branch refs and the web compare page keeps rejecting suffixes.

Closes go-gitea#33943

Assisted-by: Claude Code:claude-opus-4-8
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 17, 2026

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0efa2beba6

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread routers/common/compare.go
if !validRefSuffix.MatchString(refSuffix) {
return ""
}
commit, err := gitRepo.GetCommit(oriRef + refSuffix)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Resolve suffixes after disambiguating refs

When a suffix is present this bypasses UnstableGuessRefByShortName and asks git to resolve the raw short name plus suffix. In repositories where a branch and tag share the same name, the non-suffixed compare path resolves the branch first, but compare/v1^...main feeds v1^ to git and git reports the ambiguous ref as missing, so users cannot compare the branch parent. Resolve oriRef to the branch/tag/SHA first and append the suffix to that full ref to preserve the existing branch-first semantics.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a related edge case worth mentioning. When a branch and tag share a name, the suffix path uses the default git order instead of checking branches first (UnstableGuessRefByShortName), so name^ can pick the tag. I can fix it here, or open a follow-up if you want to keep this PR focused.

@github-actions github-actions Bot added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Jun 17, 2026
Comment thread routers/common/compare.go Outdated
Comment thread routers/api/v1/repo/pull.go Outdated
Comment thread routers/web/repo/compare.go Outdated
ResolveRefWithSuffix returned an empty ref for an unsupported suffix and
for a valid suffix that did not resolve, so the compare endpoint answered
404 in both cases. Return an invalid-argument error for an unsupported
suffix so the API responds 400, and keep 404 for a ref that does not resolve.
Rename the headRef local to headOriRef to match the struct field.

Assisted-by: Claude Code:claude-opus-4-8
Route the web compare page through common.ResolveRefWithSuffix, the
resolver the API already uses, so `^` and `~N` ref suffixes resolve to
a commit instead of being rejected. A resolved suffix is a commit
rather than a branch, so the page stays diff-only and does not offer
to open a pull request.

Assisted-by: Claude Code:claude-opus-4-8
@eyupcanakman

Copy link
Copy Markdown
Author

Done. The web compare page now resolves base and head suffixes through the same common.ResolveRefWithSuffix as the API, so the guard is gone and base...head^ or ~N renders the diff. A resolved suffix is a commit rather than a branch, so the page shows the diff but does not offer to open a pull request, which matches the API staying diff-only for suffixes.

On the branch/tag name-clash from the codex thread, it lives in that shared resolver, so the web path inherits the same handling rather than adding anything new. I can fold the branch-first fix into this PR if you'd rather not leave it for a follow-up.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jun 20, 2026
Comment thread routers/common/compare.go Outdated
Comment thread routers/common/compare.go Outdated
A nil error now guarantees a usable RefName. A missing ref and a
suffix that does not resolve return a not-found error instead of an
empty RefName, so the callers no longer check for an empty ref after
a nil error. Lazy-init the suffix regexp with sync.OnceValue.

Assisted-by: Claude Code:claude-opus-4-8
Comment thread tests/integration/compare_test.go
{"does-not-exist", ""},
{"add-csv", "~50"},
} {
ref, err := common.ResolveRefWithSuffix(gitRepo, tc.oriRef, tc.suffix)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It needs to assert that ResolveRefWithSuffix won't return branch ref if the ref has suffix.

Otherwise the "New Pull Request" logic would have problems.

@wxiaoguang wxiaoguang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good. Just some nits.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jun 21, 2026
@lunny lunny added this to the 1.27.0 milestone Jun 22, 2026
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jun 22, 2026
@lunny lunny enabled auto-merge (squash) June 22, 2026 03:53
@bircni bircni disabled auto-merge June 22, 2026 03:55
@bircni

bircni commented Jun 22, 2026

Copy link
Copy Markdown
Member

@lunny why enable merge if there are some nits open

@bircni bircni removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jun 22, 2026
@bircni

bircni commented Jun 22, 2026

Copy link
Copy Markdown
Member

@wxiaoguang addressed the minor nits

bircni pushed a commit that referenced this pull request Jun 22, 2026
…38172)

This is really a follow-up to
[#38148](#35305) , instead of
having specific mappings of options for git configurations, just honor
any user-provided gitconfig. I include a test which points out the
specific config I have which was previously not honored, but more
generally this means that gitea now only *adds* new gitconfig and never
overwrites any config provided under `[git.config]`.

---------

Signed-off-by: Royce Remer <royceremer@gmail.com>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/feature Completely new functionality. Can only be merged if feature freeze is not active.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support "^" character is compare API to designate previous commits

5 participants