rpc: fix MatchMirror substring ambiguity for exact-match names#227
rpc: fix MatchMirror substring ambiguity for exact-match names#227amitmishra11 wants to merge 2 commits into
Conversation
When a mirror name is a substring of other mirror names (e.g. "fcix.net" vs. "mirror.fcix.net" and "paducahix.mm.fcix.net"), MatchMirror used a plain strings.Contains scan, so every mirror whose name contained the pattern would show up as a match. This made it impossible to address the shorter mirror on its own via "mb edit" or similar commands, since the CLI always reported "Multiple match" and aborted. Extract the matching logic into matchMirrorsByPattern and prefer a single case-insensitive exact match over partial substring matches, while keeping the existing substring-match behavior when no exact match exists. Fixes videolabs#134 This PR was written primarily by Claude Code; I reviewed the change and ran the tests before submitting.
|
Is this AI generated ? |
Yes the changes are AI assisted and I have reviewed the changes. |
| @@ -0,0 +1,98 @@ | |||
| // Copyright (c) 2014-2019 Ludovic Fauvet | |||
There was a problem hiding this comment.
You should set this line as Copyright (c) 2026 <your-name-here>, given that you created this file
There was a problem hiding this comment.
Fixed — copyright line updated to reflect the actual author of the new test file.
🤖 Addressed by Claude Code
| } | ||
|
|
||
| func TestMatchMirrorsByPatternExactMatchAmongSubstrings(t *testing.T) { | ||
| // Regression test for https://github.com/etix/mirrorbits/issues/134 |
There was a problem hiding this comment.
The right URL is https://github.com/videolabs/mirrorbits/issues/134
There was a problem hiding this comment.
Fixed — issue URL corrected to videolabs/mirrorbits/issues/134.
🤖 Addressed by Claude Code
|
Can you ask Claude to refactor the tests in order to use sub-tests? All the tests can be defined by 3 variables:
Then the logic to run and check the results is the same for all the tests. So it should be trivial to use sub-tests and make it all much more concise, only one main test function |
| // (case-insensitive), only that mirror is returned, even if other mirror | ||
| // names also contain pattern as a substring. This allows a mirror whose | ||
| // name is a substring of other mirror names (e.g. "fcix.net" vs. | ||
| // "mirror.fcix.net") to still be matched unambiguously. |
There was a problem hiding this comment.
I find the description hard to read, so I tried to reword, failed, asked AI for help, ended up with that:
// matchMirrorsByPattern returns a list of mirrors:
// - if the pattern matches a mirror's name exactly, only that mirror is returned
// - otherwise, all mirrors containing the pattern as a substring are returned
// - all matches are case-insensitive
// This allows a mirror whose name is a substring of other mirror names
// (e.g. "fcix.net" vs. "mirror.fcix.net") to still be matched unambiguously.
Please update if you think it's better, or ignore otherwise (this is not super important)
There was a problem hiding this comment.
Updated — adopted the improved wording you suggested.
🤖 Addressed by Claude Code
- Convert 5 separate test functions for matchMirrorsByPattern into a single table-driven test using t.Run, as requested by reviewer. - Fix copyright header on rpc_test.go to reflect the actual author. - Correct regression test URL from etix/mirrorbits to videolabs/mirrorbits. - Adopt improved function comment wording suggested by reviewer.
|
Pushed a follow-up commit addressing all review feedback:
🤖 Addressed by Claude Code |
3 participants
Problem
mb edit <name>(and other commands that resolve a mirror by name)matches mirrors using a plain
strings.Containsscan over all mirrornames. When a mirror's name happens to be a substring of one or more
other mirror names, it can never be addressed on its own: every mirror
whose name contains the pattern is reported as a match, so the CLI
always prints "Multiple match" and exits.
For example, given mirrors
fcix.net,mirror.fcix.net, andpaducahix.mm.fcix.net, runningmb edit fcix.netreports all threeas matches instead of resolving to the exact
fcix.netmirror.Fixes #134
Fix
MatchMirror's matching logic is extracted into a standalone,testable function
matchMirrorsByPattern. It now prefers a singlecase-insensitive exact match over partial substring matches. If no
exact match exists, it falls back to the previous substring-match
behavior (including returning multiple matches when the pattern is
genuinely ambiguous).
Testing
rpc/rpc_test.gocovering: the exact-match-among-substringsregression case from Cannot edit a mirror when its ID is a substring of another #134, case-insensitivity of the exact match,
preserved multi-match behavior when no exact match exists, no-match,
and a basic single-substring-match case.
strings.Contains-onlylogic and passes with the fix.
go build/go vetclean forGOOS=linux(this repo's CI target);the local Windows dev environment can't compile the
processpackageor the original
rpc/rpc.go(syscall.SIGUSR2is Unix-only), which isa pre-existing platform limitation unrelated to this change.
This PR was written primarily by Claude Code; I reviewed the change and
ran the tests before submitting.