Skip to content

Fix bug the protected branch rule name is conflicted with renamed branch name#36650

Merged
lunny merged 5 commits intogo-gitea:mainfrom
lunny:lunny/fix_branch_rename
Feb 17, 2026
Merged

Fix bug the protected branch rule name is conflicted with renamed branch name#36650
lunny merged 5 commits intogo-gitea:mainfrom
lunny:lunny/fix_branch_rename

Conversation

@lunny
Copy link
Copy Markdown
Member

@lunny lunny commented Feb 17, 2026

Fix #36464

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 17, 2026
@github-actions github-actions bot added the modifies/go Pull requests that update Go code label Feb 17, 2026
@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 Feb 17, 2026
@silverwind
Copy link
Copy Markdown
Member

This comment was written by an AI assistant (Claude).


The fix correctly prevents the duplicate key value violates unique constraint "UQE_protected_branch_s" crash from #36464 by checking for an existing rule before updating. However, I have a concern about the resulting behavior:

Orphaned protection rule for the source branch: When renaming devmain and both branches have protection rules, the fix skips renaming the dev rule, leaving it in place pointing to a branch name that no longer exists. The test confirms this — after the rename, the dev protection rule still exists with RuleName: "dev".

This means:

  • Users will see a stale protection rule for the non-existent dev branch in the UI
  • The protection settings that were on dev are silently discarded rather than merged or explicitly handled

Should the orphaned from rule be deleted in this case? Something like:

if existingRule == nil || existingRule.ID == protectedBranch.ID {
    protectedBranch.RuleName = to
    if _, err = sess.ID(protectedBranch.ID).Cols("branch_name").Update(protectedBranch); err != nil {
        return err
    }
} else {
    // Delete the old rule since the branch no longer exists and the target already has a rule
    if _, err = sess.ID(protectedBranch.ID).Delete(&ProtectedBranch{}); err != nil {
        return err
    }
}

Otherwise the fix itself is sound — the conflict check is correct and the test covers the right scenario.

@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 Feb 17, 2026
@wxiaoguang wxiaoguang marked this pull request as draft February 17, 2026 07:05
@wxiaoguang
Copy link
Copy Markdown
Contributor

Does it mean that:

  • main branch protection has many complex rules
  • When test branch protection is renamed to main, then the main rules are all deleted without confirming by the operator? Even if the new name main is inputted by mistake from test?

@lunny
Copy link
Copy Markdown
Member Author

lunny commented Feb 17, 2026

  • When test branch protection is renamed to main, then the main rules are all deleted without confirming by the operator? Even if the new name main is inputted by mistake from test?

No. When test branch protection is renamed to main. the main rules will be kept but test rules will be deleted because the branch test does not exist any more. The previous logic updates the old test rule to main rule but the rules names are unique so that it returned 500.

@wxiaoguang wxiaoguang marked this pull request as ready for review February 17, 2026 07:27
@wxiaoguang
Copy link
Copy Markdown
Contributor

wxiaoguang commented Feb 17, 2026

Hmm, a second thought, why not just keep both test and main branch protection rules in this case? Then users can have the chance to choose (and rename) one by themselves later.

Deleting something silently beyond user's knowledge seems strange to me.

(Not blocker, it's up to you)

@lunny
Copy link
Copy Markdown
Member Author

lunny commented Feb 17, 2026

Hmm, a second thought, why not just keep both test and main branch protection rules in this case? Then users can have the chance to choose (and rename) one by themselves later.

Deleting something silently beyond user's knowledge seems strange to me.

(Not blocker, it's up to you)

531e82f

@lunny lunny enabled auto-merge (squash) February 17, 2026 19:30
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Feb 17, 2026
@lunny lunny merged commit 318cb85 into go-gitea:main Feb 17, 2026
24 checks passed
@GiteaBot GiteaBot added this to the 1.26.0 milestone Feb 17, 2026
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Feb 17, 2026
@lunny lunny deleted the lunny/fix_branch_rename branch February 17, 2026 20:15
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Feb 17, 2026
@GiteaBot GiteaBot added the backport/done All backports for this PR have been created label Feb 17, 2026
silverwind added a commit to silverwind/gitea that referenced this pull request Feb 17, 2026
* origin/main:
  Remove i18n backport tool at the moment because of translation format changed (go-gitea#36643)
  Fix bug the protected branch rule name is conflicted with renamed branch name (go-gitea#36650)
  Update JS deps (go-gitea#36656)
silverwind pushed a commit that referenced this pull request Feb 17, 2026
…nch name (#36650) (#36661)

Backport #36650 by @lunny

Fix #36464

Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
silverwind pushed a commit to silverwind/gitea that referenced this pull request Feb 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport/done All backports for this PR have been created backport/v1.25 lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/go Pull requests that update Go code type/bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Renaming Protected Branch Causes Duplicate Unique-Constraint Violation (UQE_protected_branch_s)

5 participants