review: rename shadowed closure params and check exit codes in e2e

jdx · claude · jdx · commit acf5798e39ab · 2026-04-26T14:08:39.000-05:00
- runtime_symlinks: rename closure params `from_name`/`to_name` to `f`/`t` so the outer `from_name: String` used by `concrete_installs.contains` is obviously distinct from the closure's `OsStr` args. - test_install_system_readonly: extract `assert_silent_install` helper that fails on a non-zero exit code instead of just inspecting stdout. Catches the case where mise crashes with an unrelated error message. Review feedback from gemini-code-assist and greptile-apps on #9410. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
diff --git a/e2e/cli/test_install_system_readonly b/e2e/cli/test_install_system_readonly
@@ -24,25 +24,27 @@ assert "readlink $SYSTEM_INSTALLS/tiny/latest" "./2.1.0"
 chmod -R a-w "$SYSTEM_INSTALLS"
 trap 'chmod -R u+w "$SYSTEM_INSTALLS" 2>/dev/null || true' EXIT
 
+assert_silent_install() {
+	local out
+	if ! out="$($1 2>&1)"; then
+		echo "$out"
+		fail "[$1] command failed"
+	fi
+	echo "$out"
+	if [[ $out == *"skipping symlink update"* || $out == *"Permission denied"* || $out == *"failed rm"* ]]; then
+		fail "[$1] unexpected system-dir warning during install"
+	fi
+}
+
 # Re-installing a tool that lives only in the user dir must succeed without
 # any warning about the system dir — the system-side runtime symlink already
 # matches the desired state, so nothing should be written or attempted there.
-out="$(mise install --force tiny@1.0.0 2>&1)"
-echo "$out"
-if [[ $out == *"skipping symlink update"* || $out == *"Permission denied"* || $out == *"failed rm"* ]]; then
-	echo "FAIL: unexpected system-dir warning during install"
-	exit 1
-fi
+assert_silent_install "mise install --force tiny@1.0.0"
 assert "readlink $USER_INSTALLS/tiny/latest" "./1.0.0"
 assert "readlink $SYSTEM_INSTALLS/tiny/latest" "./2.1.0"
 
 # Installing a different tool that doesn't touch the system dir at all must
 # also succeed silently.
-out="$(mise install dummy@1.0.0 2>&1)"
-echo "$out"
-if [[ $out == *"skipping symlink update"* || $out == *"Permission denied"* || $out == *"failed rm"* ]]; then
-	echo "FAIL: unexpected system-dir warning during install"
-	exit 1
-fi
+assert_silent_install "mise install dummy@1.0.0"
 assert_directory_exists "$USER_INSTALLS/dummy/1.0.0"
 assert "readlink $SYSTEM_INSTALLS/tiny/latest" "./2.1.0"
diff --git a/src/runtime_symlinks.rs b/src/runtime_symlinks.rs
@@ -72,7 +72,7 @@ fn rebuild_symlinks_in_dir(
             } else if from
                 .file_name()
                 .zip(to.file_name())
-                .is_some_and(|(from_name, to_name)| from_name != to_name)
+                .is_some_and(|(f, t)| f != t)
                 && !concrete_installs.contains(&from_name)
             {
                 // Real (non-symlink) directory at a runtime-symlink slot —

Original file line number	Diff line number	Diff line change
`@@ -72,7 +72,7 @@ fn rebuild_symlinks_in_dir(`
`72`	`72`	`} else if from`
`73`	`73`	`.file_name()`
`74`	`74`	`.zip(to.file_name())`
`75`		`- .is_some_and(\|(from_name, to_name)\| from_name != to_name)`
	`75`	`+ .is_some_and(\|(f, t)\| f != t)`
`76`	`76`	`&& !concrete_installs.contains(&from_name)`
`77`	`77`	`{`
`78`	`78`	`// Real (non-symlink) directory at a runtime-symlink slot —`