fix(install): correct a false claim about shell assignment semantics
Review flagged the resolve_dir comment: it claimed `x=$(cmd) || fallback` never fires "because the assignment succeeds even when the substitution fails." That is wrong — command substitution propagates exit status and the fallback does fire, confirmed in sh, bash, dash, and zsh. Reproducing the original code shows the fallback also worked, so the second "recurrence" the Agent Note described never existed. Both real defects were the same one: comparing a resolved path against an unresolved one. The note now says that instead of inventing a mechanism. resolve_dir keeps its `|| printf` because it makes every caller a plain assignment, so no site can compare against an empty path by forgetting its own fallback — the reason is now stated accurately. Also from review: REPO_COMMON is now resolved on both branches, matching REPO_ROOT, and _repo_root notes why it is already physical without its own resolve_dir call.
This commit is contained in:
@@ -2,5 +2,5 @@
|
||||
# side as of the last confirmed-consistent state. Both languages carry equal authority;
|
||||
# after editing either side, bring the other along and re-record with:
|
||||
# pnpm run verify-translation-pairing --write .agents/notes/implemented/process/2026-07-31-installer-adopts-existing-checkout.md
|
||||
2026-07-31-installer-adopts-existing-checkout.md: 5eede5d476d21c9f2bf0b63365eab9ac705bad60
|
||||
2026-07-31-installer-adopts-existing-checkout.zh.md: a137c585de7da5b1ccc1167c2c8e9d1ca939298b
|
||||
2026-07-31-installer-adopts-existing-checkout.md: f2f4a2bf87696bc2254a352dd7568ea73f8f900b
|
||||
2026-07-31-installer-adopts-existing-checkout.zh.md: b7a545e6eb43bd8748185b26b6a7ee965353b79b
|
||||
|
||||
@@ -20,7 +20,7 @@ The installer records nothing about where that repository lives. A container who
|
||||
|
||||
Adoption branches from `HEAD`, so committed work is what runs and uncommitted changes stay in the checkout. This is not prompted or warned about: the installer builds the layout and gets out of the way. Setting `DSH_SOURCE` to a different directory remains the one documented way to opt back into cloning a separate tree.
|
||||
|
||||
Every path comparison runs on physical paths through a `resolve_dir` helper, and every compared value is resolved at assignment rather than at the comparison. macOS resolves `/var` through a symlink to `/private/var`, so comparing a git-reported path against an unresolved one misclassified an existing managed install as a foreign clone and would have built a second container beside the real one. The same defect recurred twice more during review — once where a curl install's `REPO_ROOT` stayed unresolved and so compared unequal against every resolved path, and once where `x=$(resolve_dir …) || x=$fallback` left an empty path because the assignment succeeds even when the substitution fails. `resolve_dir` therefore echoes a missing path back itself, and callers that need "does not exist" test the directory explicitly. `git rev-parse --path-format=absolute` would do the same job but requires git 2.31+.
|
||||
Every path comparison runs on physical paths through a `resolve_dir` helper, and every compared value is resolved at assignment rather than at the comparison. macOS resolves `/var` through a symlink to `/private/var`, so comparing a git-reported path against an unresolved one misclassified an existing managed install as a foreign clone and would have built a second container beside the real one. The same defect recurred twice more during review, both times as one side of a comparison left unresolved: a curl install's `REPO_ROOT`, and the container path it was compared against. `resolve_dir` therefore echoes a missing path back rather than failing, so a not-yet-created container needs no per-call fallback and no site can compare against an empty path by forgetting one; callers that need "does not exist" test the directory explicitly. `git rev-parse --path-format=absolute` would do the same job but requires git 2.31+.
|
||||
|
||||
Before `current` is repointed, the installer rejects a staging path that resolves to the repository itself, enforcing the upgrade contract that the launcher never resolves to the master clone.
|
||||
|
||||
|
||||
@@ -20,7 +20,7 @@ Status: implemented
|
||||
|
||||
接管以`HEAD`为分支起点,因此运行的是已提交的内容,未提交的更改仍留在检出中。这一点既不提示也不警告:安装器构建好布局后便不再打扰。把`DSH_SOURCE`设为其他目录,仍是唯一有文档记载的、回到克隆另一棵树的方式。
|
||||
|
||||
所有路径比较都通过`resolve_dir`辅助函数在物理路径上进行,且每个参与比较的值都在赋值时解析,而非在比较时解析。macOS 会把`/var`经符号链接解析为`/private/var`,因此拿 git 报告的路径与未解析的路径相比较,会把已有的受管安装误判为外来克隆,并在真正的容器旁再建一个容器。同一缺陷在评审过程中又出现了两次——一次是 curl 安装的`REPO_ROOT`未经解析,从而与所有已解析路径比较时均不相等;另一次是`x=$(resolve_dir …) || x=$fallback`留下了空路径,因为即使命令替换失败,赋值本身仍然成功。因此`resolve_dir`会在路径不存在时原样回显该路径,而需要判断"不存在"的调用方则显式检测该目录。`git rev-parse --path-format=absolute`能完成同样的工作,但要求 git 2.31 及以上版本。
|
||||
所有路径比较都通过`resolve_dir`辅助函数在物理路径上进行,且每个参与比较的值都在赋值时解析,而非在比较时解析。macOS 会把`/var`经符号链接解析为`/private/var`,因此拿 git 报告的路径与未解析的路径相比较,会把已有的受管安装误判为外来克隆,并在真正的容器旁再建一个容器。同一缺陷在评审过程中又出现了两次,两次都是比较的一侧未经解析:一次是 curl 安装的`REPO_ROOT`,一次是与之比较的容器路径。因此`resolve_dir`在路径不存在时原样回显该路径而非失败,这样尚未创建的容器无需在每个调用点单独兜底,也就没有调用点会因遗漏兜底而与空路径比较;需要判断"不存在"的调用方则显式检测该目录。`git rev-parse --path-format=absolute`能完成同样的工作,但要求 git 2.31 及以上版本。
|
||||
|
||||
在重指`current`之前,安装器会拒绝解析结果等于仓库自身的 staging 路径,以此落实"启动器绝不解析到 master 克隆"这一升级契约。
|
||||
|
||||
|
||||
@@ -78,9 +78,9 @@ DSH_STAGING=$DSH_SOURCE/staging-$DSH_STAMP
|
||||
# `git rev-parse --path-format=absolute` would do this, but it needs git 2.31+.
|
||||
#
|
||||
# A not-yet-created directory (the container on a fresh install) has no physical
|
||||
# path, so fall back to the literal argument here rather than at each call site:
|
||||
# `x=$(cmd) || fallback` never fires, because the assignment succeeds even when
|
||||
# the substitution fails, which would silently yield an empty path.
|
||||
# path. Falling back here rather than at each call site keeps every caller a
|
||||
# plain assignment, so no site can compare against an empty path by forgetting
|
||||
# its own fallback.
|
||||
resolve_dir() { CDPATH= cd -- "$1" 2>/dev/null && pwd -P || printf '%s\n' "$1"; }
|
||||
|
||||
# --- in-repo detection ---------------------------------------------------------
|
||||
@@ -95,6 +95,8 @@ DSH_CHECKOUT=''
|
||||
if [ -f "$0" ]; then
|
||||
_self_dir=$(resolve_dir "$(dirname -- "$0")")
|
||||
if [ -n "$_self_dir" ]; then
|
||||
# Physical without its own resolve_dir: dirname is textual, so trimming a
|
||||
# resolved path leaves one. The comparison below depends on that.
|
||||
_repo_root=$(dirname -- "$_self_dir")
|
||||
if [ "$(basename -- "$_self_dir")" = scripts ] \
|
||||
&& [ -x "$_repo_root/bin/dsh" ] && [ -f "$_repo_root/scripts/install.sh" ]; then
|
||||
@@ -277,9 +279,10 @@ else
|
||||
mkdir -p "$DSH_SOURCE"
|
||||
git clone --branch "$DSH_REF" "$DSH_REPO" "$DSH_MASTER"
|
||||
fi
|
||||
REPO_COMMON=$DSH_MASTER/.git
|
||||
# Physical, to match the adoption branch: every REPO_ROOT comparison below
|
||||
# runs against resolved paths.
|
||||
# Physical on both branches: REPO_ROOT is compared against resolved paths
|
||||
# below, and REPO_COMMON stays symmetric with it so neither can be read as
|
||||
# carrying a different kind of path.
|
||||
REPO_COMMON=$(resolve_dir "$DSH_MASTER/.git")
|
||||
REPO_ROOT=$(resolve_dir "$DSH_MASTER")
|
||||
fi
|
||||
|
||||
|
||||
Reference in New Issue
Block a user