Skip to content

Remove MIGRATE command dead code in ACLSelectorCheckCmd#3804

Merged
enjoy-binbin merged 1 commit into
valkey-io:unstablefrom
enjoy-binbin:rm_migrate_dead_code
May 23, 2026
Merged

Remove MIGRATE command dead code in ACLSelectorCheckCmd#3804
enjoy-binbin merged 1 commit into
valkey-io:unstablefrom
enjoy-binbin:rm_migrate_dead_code

Conversation

@enjoy-binbin

@enjoy-binbin enjoy-binbin commented May 22, 2026

Copy link
Copy Markdown
Member

In #2309, we ultimately decided not to set get_dbid_args for
MIGRATE command and removed the relevant code. But, we missed this
one, just cleanup the dead code.

@enjoy-binbin
Remove MIGRATE command dead code in ACLSelectorCheckCmd
a0c1557 
In valkey-io#2309, we ultimately decided not to set `get_dbid_args` for
MIGRATE command and removed the relevant code. But, we missed this
one, just cleanup the dead code.

Signed-off-by: Binbin <binloveplay1314@qq.com>
@coderabbitai

coderabbitai Bot commented May 22, 2026 •
edited
Loading

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 11cbca00-52a6-4c82-bbf2-880d548d34af

📥 Commits

Reviewing files that changed from the base of the PR and between 8d3e6c3 and a0c1557.

📒 Files selected for processing (1)
  • src/acl.c
💤 Files with no reviewable changes (1)
  • src/acl.c
📝 Walkthrough

Walkthrough

This change removes special-case handling for migrateCommand in the ACLSelectorCheckCmd function. When database-level access is denied, the code previously assigned a specific keyidxptr value for migrate; this special branch is deleted, unifying the logic so all commands follow the same fallback assignment path.

Changes

ACL Database Denial Logic

Layer / File(s) Summary
Remove migrateCommand special case in ACL denial
src/acl.c		 The previous special-case logic that set *keyidxptr = 4 specifically for migrateCommand when denying database-level permissions is removed. All commands now follow the uniform fallback path for keyidxptr assignment.

🎯 1 (Trivial) | ⏱️ ~3 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately and concisely describes the main change: removing dead code related to the MIGRATE command in ACLSelectorCheckCmd function.
Description check ✅ Passed The description clearly explains the context and reason for the change, referencing the prior decision in #2309 and indicating this is cleanup of missed dead code.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov

codecov Bot commented May 22, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.67%. Comparing base (fa57b3c) to head (a0c1557).
⚠️ Report is 1 commits behind head on unstable.

Additional details and impacted files 
@@             Coverage Diff              @@
##           unstable    #3804      +/-   ##
============================================
- Coverage     76.68%   76.67%   -0.01%     
============================================
  Files           162      162              
  Lines         80710    80708       -2     
============================================
- Hits          61891    61885       -6     
- Misses        18819    18823       +4
Files with missing lines Coverage Δ
src/acl.c 92.58% <ø> (+0.05%) ⬆️

... and 21 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

zuiderkwast
zuiderkwast approved these changes May 22, 2026
View reviewed changes

@zuiderkwast zuiderkwast 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.

If I remember correctly, we skipped it because the MIGRATE destination-db is on another node with potentially different ACL rules and users, right?

@dvkashapov

dvkashapov commented May 22, 2026

Copy link
Copy Markdown
Member

Exactly!

@enjoy-binbin enjoy-binbin merged commit 355d306 into valkey-io:unstable May 23, 2026
64 checks passed
@enjoy-binbin enjoy-binbin deleted the rm_migrate_dead_code branch May 23, 2026 02:33
@coderabbitai coderabbitai Bot mentioned this pull request Jun 2, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zuiderkwast zuiderkwast zuiderkwast approved these changes
+2 more reviewers
@nmvk nmvk nmvk approved these changes
@dvkashapov dvkashapov dvkashapov approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@enjoy-binbin enjoy-binbin

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@enjoy-binbin @dvkashapov @zuiderkwast @nmvk