files: batch link creation and target checks - #9718
Open
andrewgazelka wants to merge 1 commit into
Open
Conversation
The linkGeneration and checkLinkTargets activation steps forked several processes per managed file: a dirname command substitution plus mkdir and ln for every link, and a readlink per existing target during the collision check. On platforms where fork+exec costs a few milliseconds (notably darwin) this dominates activation time: a profile with 365 links spent about 3.4s in linkGeneration and 1.1s in checkLinkTargets. Classify targets with bash builtins, resolve all existing symlinks with a single readlink -z call, create missing parent directories with one mkdir -p, and group new links into one ln -sfn -t call per target directory. Only targets occupied by a regular file or directory fall back to the original per-file path, preserving the backup command, extension and overwrite handling, the identical-content skip, the failure on a real directory in the way (previously via ln -T), and dry-run output. On the same 365-link profile the two steps now take about 0.2s on a full relink and under 0.05s when the generation is unchanged, with an identical resulting tree.
andrewgazelka
force-pushed
the
upstream/files-batch-link-creation-and-target-checks
branch
from
July 28, 2026 00:53
6e30169 to
1161352
Compare
andrewgazelka
marked this pull request as ready for review
July 28, 2026 00:58
andrewgazelka
added a commit
to indexable-inc/index
that referenced
this pull request
Jul 28, 2026
Opening the first PR through this machinery found a bug in it, and reviewing home-manager for the opt-in gate found four things in our patch. ### The tool called its own new PR someone else's duplicate `upstream-pr --open` run by hand opens the PR without going through the sync loop, so the status file never learns of it. On the next `upstream-sync` the fuzzy duplicate search found that very PR, reported it as a competitor, and left `pr: null` with the patch recorded as blocked by itself. "Have we already sent this?" and "did someone else propose this?" are different questions. The first is now asked directly, before the search, with `gh pr list --head <branch>` on our own branch. The head repo owner is checked in the tool, because `gh pr list --head` matches on the bare branch name and silently returns nothing for `owner:branch`; without the owner check a stranger's same-named branch would be adopted as ours. The live failure is pinned as a regression test. ### home-manager is opted in, and this change is the review that earned it The gate said "out pending review: nobody has checked our commits against their conventions". Doing that check found: - Subject was 61 characters against their documented 50, and five body lines exceeded 72. Fixed in the fork, message only, tree byte-identical. The intent key follows the commit subject, so it moved with it. - A `Refs #3689` trailer pointing at a repo their reviewers cannot open. Dropped; that context is in the registry now. - Their treefmt reports both changed files clean, and `tests/modules/files` passes on the patched tree, `target-conflict` included, which is the collision path this patch rewrites. - Our base was 45 commits behind master and both files we touch had moved, so the first submission conflicted and their CI failed on it. Rebasing produced one conflict, and resolving it took upstream's own fix (deeb6b7e, unintended subshell expansion in a backup message) into the block our patch had moved into a function. We would otherwise have reintroduced that bug. ### Result [nix-community/home-manager#9718](nix-community/home-manager#9718) is open and not a draft, with all seven of their checks green. Known gap: the fork's `ix-patched` branch still sits on the old base, so it and the PR have diverged. Rebasing the maintained branch changes what workstation configs consuming it get, so it is a deliberate act rather than something to slip into this change. Assisted-by: Claude Opus 4.5 via Claude Code
khaneliman
reviewed
Jul 28, 2026
Comment on lines
+229
to
+235
| while IFS= read -r -d "" currentSource ; do | ||
| if [[ "$currentSource" != "''${symlinkSources[i]}" ]] ; then | ||
| linkSources+=("''${symlinkSources[i]}") | ||
| linkDirs+=("''${symlinkTargets[i]%/*}") | ||
| fi | ||
| i=$(( i + 1 )) | ||
| done < <(readlink -z -- "''${symlinkTargets[@]}") |
Collaborator
There was a problem hiding this comment.
issue (blocking): reject partial readlink batches before linking
readlink omits a record when an operand vanishes, so a later result shifts onto the wrong source; I reproduced the first target disappearing, the helper exiting 0, and the second link staying stale. Minimum safe fix is an exact-count guard immediately after this loop, before the later mkdir/ln calls:
if [[ $i -ne ''${#symlinkTargets[@]} ]] ; then
errorEcho "A link target changed while resolving symlinks; retry activation."
exit 1
fiPlease mirror the equivalent guard in check-link-targets.sh; reclassifying or falling back per target instead of exiting is also valid.
andrewgazelka
added a commit
to indexable-inc/home-manager
that referenced
this pull request
Jul 31, 2026
flake.lock in indexable-inc/index pinned d27be2a while this bookmark held 6e30169, so the activation patch that shipped was a different revision from the one on the branch (ENG-11646). The two commits carry an identical tree: 6e30169 is the same change with the subject shortened to home-managers 50-character limit and the internal issue trailer dropped for the upstream submission (nix-community#9718). Merging the pinned revision in makes the previously locked rev an ancestor of this bookmark, so both revisions describe one tree and neither is orphaned.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The linkGeneration and checkLinkTargets activation steps forked several
processes per managed file: a dirname command substitution plus mkdir
and ln for every link, and a readlink per existing target during the
collision check. On platforms where fork+exec costs a few milliseconds
(notably darwin) this dominates activation time: a profile with 365
links spent about 3.4s in linkGeneration and 1.1s in checkLinkTargets.
Classify targets with bash builtins, resolve all existing symlinks with
a single readlink -z call, create missing parent directories with one
mkdir -p, and group new links into one ln -sfn -t call per target
directory. Only targets occupied by a regular file or directory fall
back to the original per-file path, preserving the backup command,
extension and overwrite handling, the identical-content skip, the
failure on a real directory in the way (previously via ln -T), and
dry-run output.
On the same 365-link profile the two steps now take about 0.2s on a
full relink and under 0.05s when the generation is unchanged, with an
identical resulting tree.
Contributed from a maintained fork; the patch of record is indexable-inc@1161352.
Prepared with AI assistance (Claude); directed and reviewed by a human maintainer.
/cc @andrewgazelka @Axionize @harivansh-afk (indexable-inc), who maintain the fork this patch is carried in, so review comments reach someone who can act on them.