Repository navigation
fix(pipeline): skip a file named nul in the control-file walk instead of aborting the index (#2646) - #2903
Open
tanishqbaweja wants to merge 1 commit into
Open
fix(pipeline): skip a file named nul in the control-file walk instead of aborting the index (#2646)#2903tanishqbaweja wants to merge 1 commit into
tanishqbaweja wants to merge 1 commit into
Conversation
… of aborting the index (DeusData#2646) semantic_manifest_control_entry() reads the metadata of every entry the control-file walk lists, and one failed read fails the whole manifest. On Win32 the plain path <dir>\nul names the NUL device, so a real file named nul (Git Bash leaves one behind after `cmd > nul` or `2>nul`) cannot be read: GetFileAttributesExW fails with ERROR_INVALID_PARAMETER. The manifest returns CBM_NOT_FOUND and every index run on the repository ends in CBM_PIPELINE_ABORT_PRESERVE_DB (aborted_previous_preserved). Listing nul in .gitignore does not help, because the walk visits every entry. Only directories and control files can contribute to the manifest. Decide the control name first and skip an entry that the listing already shows to be neither, without reading it. A Windows listing always carries the attributes; on POSIX an entry whose d_type is DT_UNKNOWN is still read. Directories and control-named entries are read exactly as before. Signed-off-by: Tanishq <slatorgator3@gmail.com>
|
Thanks for opening this — it has been seen, and it is queued. This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence. Current review status: working through a backlog. What that means for this PR, concretely:
Things that will genuinely speed it up whenever review does happen:
If this fixes a bug, a reproduction we can run is worth more than a description of the symptom. Thanks for contributing, and sorry in advance for the wait. |
This branch has not been deployed
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
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.
What does this PR do?
Fixes #2646.
On Windows, one file named
nulin a repository makes everyindex_repositoryrun returnaborted_previous_preserved, so a project without an index never gets one. Git Bash leaves such a file behind aftercmd > nulor2>nul, and listing it in.gitignoredoes not help.The abort comes from the semantic manifest's control-file walk.
semantic_manifest_control_entry()reads the metadata of every entry it lists, and one failed read fails the whole manifest. On Win32 the plain path<dir>\nulnames the NUL device, soGetFileAttributesExWfails with error 87 on the real file. The manifest returnsCBM_NOT_FOUND, andcbm_pipeline_run()turns that intoCBM_PIPELINE_ABORT_PRESERVE_DB. Discovery is not affected:_wstat64reports a character device, and the entry is skipped.The walk records only control files and descends only into directories, so it never needs the metadata of an entry that is neither. The fix, in
pipeline_incremental.conly, checks the control name first and skips a non-control entry that the listing already reports as a non-directory, without reading it. A Windows listing always carries the attributes; on POSIX an entry whosed_typeisDT_UNKNOWNis still read. Directories and control-named entries are read exactly as before, so a failed read on one of those still fails the walk.find_alias_files()inpath_alias.calready skips!is_direntries before reading metadata.The skip does not depend on the name. On this Windows 11 build
con,aux,prn,com1,lpt1andnul.txtreach the real file, so of the names I tried onlynulis affected. The plain path of a name ending in a dot or a space does not reach the file either, and the same skip applies to it.Tests. Two Windows-only tests in
pipeline_semantic_manifest_repro, under#ifdef _WIN32like the POSIX-onlypipeline_semantic_manifest_skips_unreadable_subdirectorynext to them. Each creates<tmp>\nulthrough\\?\, next toapp.pyand a.gitignorethat listsnul, and deletes it before cleanup.pipeline_semantic_manifest_skips_nul_device_file_issue2646: the manifest builds and records.gitignore. On main it fails withrc == -1.pipeline_full_index_survives_nul_device_file_issue2646:cbm_pipeline_run()returns 0 and indexes the function inapp.py. On main it fails withrun_rc == -2.Not changed. A directory named
nulstill fails the walk, like any directory whose metadata cannot be read. The response still does not name the failing path, which is the second ask in the issue. This follows the approach of SanHsien/codebase-memory-mcp@ed6ae148 (a downstream fork, never opened here), reimplemented on current main with the check split into two helpers and an added full-index test.Verification. Windows 11 Home 23H2 (build 22631.6199), i7-10750H, 16 GB RAM, repo and TEMP on an exFAT volume. MSYS2 CLANG64 clang 23.1.4 with ASan+UBSan, built with the flags that
make -n -f Makefile.cbm build/c/test-runner CC=clang CXX=clang++prints.pipeline_semantic_manifest_repro: with the fix, the only failures are the git-ownership test below and, in some runs, the two route checks below; on main the two new tests fail as well (108 passed, 3 failed in one run). The failure both share is environmental: git exits 128 inpipeline_git_context_change_forces_full_and_refreshes_branch("detected dubious ownership"), because exFAT records no file owner. The route checks inincremental_env_edit_replaces_scoped_envvarsandincremental_compose_edit_replaces_build_edgesalso flake on this machine with and without the change: over 11 repeated runs, at least one of them failed in 10 runs on main and in 8 runs with the fix.pipeline: 447 passed, 2 failed, 1 skipped. Both failures fail the same way on main: ingit_context_linked_worktreegit also exits 128 right aftergit init, andpipeline_create_freegetsF-some-pathbecause a directory matching the test's fixed/some/pathexists on this machine's test drive.pipeline_frozen: 16 passed.index_resilience: 7 passed, 1 skipped (POSIX-only).incrementalran no tests here, because its fixture clone fails on this machine.make -f Makefile.cbm lint-ci CLANG_FORMAT=<clang-format 20.1.8>stops at cppcheck 2.21.0. It reports 18 findings, all in 9 files this change does not touch, for examplecompareValueOutOfTypeRangeErrorwherelongis 32 bits on Windows. With the same flags onpipeline_incremental.calone, cppcheck is clean. The other lint-ci targets (lint-format,lint-no-suppress,lint-memory-core) pass. CI runs cppcheck 2.20.0 on Linux.make -f Makefile.cbm lint-tidy-diff(clang-tidy 23.1.4): no findings on the changed lines.scripts/security-audit.sh: passed.make -f Makefile.cbm test(on this machine the unrelated failures above happen either way).Checklist
git commit -s)make -f Makefile.cbm test): not run in full; the suites listed above were.make -f Makefile.cbm lint-ci): on this Windows machine only cppcheck fails, on untouched files; see above.Written with an AI coding agent.