Split srch flat collector into interactive builder and reusable mover#16
Split srch flat collector into interactive builder and reusable mover#16ib-bsb-br wants to merge 1 commit into
Conversation
Reviewer's GuideRefactors the srch-based flat file collector into an interactive builder script and a reusable per-file mover, with explicit logging, deterministic collision handling, concurrency-safe locking, and enforced destination ownership/permissions, and documents the design in README.md. Flow diagram for srch_move_one per-file move logicflowchart TD
A["Start srch_move_one"] --> B["Validate environment and required vars
DESTDIR, LOGFILE, LOCKFILE or LOCKDIR"]
B --> C["Normalize source path to absolute"]
C --> D{"Source exists and is regular file?"}
D -->|no| E["Log error to LOGFILE"]
E --> Z["End"]
D -->|yes| F["Acquire lock
(flock on LOCKFILE
or mkdir LOCKDIR)"]
F --> G["Derive base name and extension"]
G --> H["Select collision-free name
with _NNN suffix if needed"]
H --> I{"DRYRUN enabled?"}
I -->|yes| J["Log planned move
source -> DESTDIR/target"]
J --> M["Release lock"]
M --> Z
I -->|no| K["Copy source to
DESTDIR/target"]
K --> L{"Copy succeeded?"}
L -->|no| E
L -->|yes| N["Delete original source file"]
N --> O["Log successful move
original path, final name"]
O --> M
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The README now reads more like an internal specification/template than user-facing documentation; consider adding a concise, task-oriented section that shows how an operator actually runs
srch_flat_collectand interpretsoperation_log.txtwithout needing to parse the full spec. - For the locking implementation (
flockwith mkdir-based fallback), ensure that all early-exit and error paths reliably release the lockfile/lockdir so that interrupted runs cannot block subsequent invocations indefinitely. - The heavy reliance on interactive prompts in
srch_flat_collectmay make automation difficult; consider supporting a non-interactive mode (e.g., flags or environment variables) so the same logic can be reused in scripted workflows without TTY prompts.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The README now reads more like an internal specification/template than user-facing documentation; consider adding a concise, task-oriented section that shows how an operator actually runs `srch_flat_collect` and interprets `operation_log.txt` without needing to parse the full spec.
- For the locking implementation (`flock` with mkdir-based fallback), ensure that all early-exit and error paths reliably release the lockfile/lockdir so that interrupted runs cannot block subsequent invocations indefinitely.
- The heavy reliance on interactive prompts in `srch_flat_collect` may make automation difficult; consider supporting a non-interactive mode (e.g., flags or environment variables) so the same logic can be reused in scripted workflows without TTY prompts.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="README.md">
<violation number="1" location="README.md:35">
P2: The README contains unresolved template placeholders (`[[...]]`) and prompt-structure tags, which makes the documentation non-actionable for users.</violation>
</file>
<file name="usr/local/libexec/srch_move_one">
<violation number="1" location="usr/local/libexec/srch_move_one:21">
P2: `_lock_release` never releases the flock lock, so each worker holds the lock for the entire script run and unintentionally serializes concurrent moves.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| </context> | ||
|
|
||
| <variables> | ||
| <variable name="[[task_request]]" required="true"> |
There was a problem hiding this comment.
P2: The README contains unresolved template placeholders ([[...]]) and prompt-structure tags, which makes the documentation non-actionable for users.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At README.md, line 35:
<comment>The README contains unresolved template placeholders (`[[...]]`) and prompt-structure tags, which makes the documentation non-actionable for users.</comment>
<file context>
@@ -1 +1,137 @@
+ </context>
+
+ <variables>
+ <variable name="[[task_request]]" required="true">
+ <description>Refactor the srch-based collection workflow into a builder and a reusable mover with consistent logging and collision handling.</description>
+ </variable>
</file context>
| if command -v flock >/dev/null 2>&1; then | ||
| exec 9>>"$LOCKFILE" | ||
| flock -x 9 | ||
| return 0 |
There was a problem hiding this comment.
P2: _lock_release never releases the flock lock, so each worker holds the lock for the entire script run and unintentionally serializes concurrent moves.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At usr/local/libexec/srch_move_one, line 21:
<comment>`_lock_release` never releases the flock lock, so each worker holds the lock for the entire script run and unintentionally serializes concurrent moves.</comment>
<file context>
@@ -0,0 +1,124 @@
+ if command -v flock >/dev/null 2>&1; then
+ exec 9>>"$LOCKFILE"
+ flock -x 9
+ return 0
+ fi
+ if [[ -z "$LOCKDIR" ]]; then
</file context>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a great step forward in refactoring the collection workflow into a more modular and maintainable two-script design. The separation of the interactive builder from the file mover is well-executed. However, I've identified a few high-severity issues that should be addressed. The builder script contains bugs related to parsing user input with spaces, which will cause incorrect behavior for paths or arguments containing spaces. Additionally, the mover script has an issue with its flock locking mechanism, where locks are not released promptly, leading to unnecessary serialization of file moves and reduced concurrency. I've also included a suggestion to improve the readability of the README.md file by using standard Markdown formatting.
| # shellcheck disable=SC2206 | ||
| SRCH_ROOTS=( $roots_raw ) |
There was a problem hiding this comment.
Unquoted expansion of $roots_raw will cause issues with paths that contain spaces, even if they are quoted in the input. The shellcheck disable=SC2206 indicates awareness, but this is a bug that should be fixed. Use read -r -a with a here-string to correctly parse space-separated arguments into an array, which respects quotes.
read -r -a SRCH_ROOTS <<< "$roots_raw"
| # shellcheck disable=SC2206 | ||
| extra_arr=( $extra_raw ) |
There was a problem hiding this comment.
Unquoted expansion of $extra_raw will cause issues with arguments that contain spaces, even if they are quoted in the input. The shellcheck disable=SC2206 indicates awareness, but this is a bug that should be fixed. Use read -r -a with a here-string to correctly parse space-separated arguments into an array.
read -r -a extra_arr <<< "$extra_raw"
|
|
||
| _lock_release() { | ||
| if command -v flock >/dev/null 2>&1; then | ||
| return 0 |
There was a problem hiding this comment.
The flock lock is not being released explicitly. _lock_release returns without action if flock is available, meaning the lock is held until the script exits. This serializes all file move operations, hurting performance, and is inconsistent with the mkdir fallback which releases the lock after name reservation. The lock should be released after name reservation to allow concurrent copy operations.
flock -u 9
| [[ | ||
| <purpose> | ||
| Provide a formal, comprehensive specification for a two-script refactor that separates an interactive srch command builder from a reusable per-file mover, while preserving flat-destination moves, deterministic collision handling, and auditable logging. | ||
| </purpose> | ||
|
|
||
| <context> | ||
| <environment> | ||
| <execution_user>root (invoked via sudo by user linaro)</execution_user> | ||
| <script_locations> | ||
| <builder>/usr/local/bin/srch_flat_collect</builder> | ||
| <mover>/usr/local/libexec/srch_move_one</mover> | ||
| </script_locations> | ||
| <destination_root>/home/linaro/</destination_root> | ||
| <log_location>operation_log.txt inside the destination folder</log_location> | ||
| </environment> | ||
|
|
||
| <constraints> | ||
| <constraint>All matched files are moved into a single flat directory under /home/linaro.</constraint> | ||
| <constraint>srch -r must invoke the per-file mover script for each matched path.</constraint> | ||
| <constraint>Collision handling must use deterministic numeric suffixes before extensions.</constraint> | ||
| <constraint>Moves must use copy+delete semantics to support cross-filesystem operations.</constraint> | ||
| <constraint>Ownership must be linaro:linaro and permissions must allow read/write for owner, group, and others (a+rwX).</constraint> | ||
| <constraint>No archive or zip operation is permitted.</constraint> | ||
| <constraint>Interactive prompts must cover feasible srch flags and filters.</constraint> | ||
| </constraints> | ||
|
|
||
| <domain_notes> | ||
| <note>Three draft approaches were provided and must be merged into a single, reusable two-script design.</note> | ||
| <note>Reducing heredocs is required to improve versioning and reuse of the per-file mover.</note> | ||
| <note>Logging must capture original paths, normalized paths, renames, and errors.</note> | ||
| </domain_notes> | ||
| </context> | ||
|
|
||
| <variables> | ||
| <variable name="[[task_request]]" required="true"> | ||
| <description>Refactor the srch-based collection workflow into a builder and a reusable mover with consistent logging and collision handling.</description> | ||
| </variable> | ||
| <variable name="[[builder_path]]" required="true"> | ||
| <description>/usr/local/bin/srch_flat_collect</description> | ||
| </variable> | ||
| <variable name="[[mover_path]]" required="true"> | ||
| <description>/usr/local/libexec/srch_move_one</description> | ||
| </variable> | ||
| <variable name="[[destination_folder]]" required="true"> | ||
| <description>Single-level folder under /home/linaro used as the collection target.</description> | ||
| </variable> | ||
| </variables> | ||
|
|
||
| <instructions> | ||
| <instruction>1. Restate the goal: split the workflow into a builder script and a reusable mover while keeping flat moves, collision-safe naming, and audit logging.</instruction> | ||
|
|
||
| <instruction>2. Implement /usr/local/bin/srch_flat_collect as an interactive srch option builder that: | ||
| (a) collects srch start points; | ||
| (b) prompts for name matching (-N/-n/-i with optional negation); | ||
| (c) prompts for path matching (-a), exclusions (-e/-E/-Z), depth (-m), and filesystem boundary (-x); | ||
| (d) prompts for size and age filters (-s, -o/-O/-P, -y/-Y/-W); | ||
| (e) prompts for ownership filters (-u/-U/-g/-G); | ||
| (f) prompts for performance options (-t/-I/-q/-Q/-X) and progress/stats (-v/-S/-T/-C); | ||
| (g) always enforces -f for regular files. | ||
| </instruction> | ||
|
|
||
| <instruction>3. Implement /usr/local/libexec/srch_move_one as a standalone mover that: | ||
| (a) normalizes paths to absolute form; | ||
| (b) allocates collision-free names using numeric suffixes; | ||
| (c) performs copy+delete moves into the destination; | ||
| (d) logs every action to a .txt file in the destination. | ||
| </instruction> | ||
|
|
||
| <instruction>4. Ensure the builder exports DESTDIR, LOGFILE, LOCKFILE/LOCKDIR, and DRYRUN so the mover can operate consistently across runs.</instruction> | ||
|
|
||
| <instruction>5. Preserve auditability by rotating existing logs and recording the full srch invocation context.</instruction> | ||
| </instructions> | ||
|
|
||
| <output_format_specification> | ||
| <format>Plain text</format> | ||
| <requirements> | ||
| <requirement>Provide both scripts as versionable files rather than large heredocs embedded in one script.</requirement> | ||
| <requirement>Maintain formal, impersonal language and the structure of this template.</requirement> | ||
| <requirement>Include a complete example of each script.</requirement> | ||
| </requirements> | ||
| </output_format_specification> | ||
|
|
||
| <examples> | ||
| <example> | ||
| <input_data> | ||
| <task_request>Provide the builder and mover scripts as two separate files for reuse and testing.</task_request> | ||
| </input_data> | ||
| <output> | ||
| Builder: /usr/local/bin/srch_flat_collect | ||
| Mover: /usr/local/libexec/srch_move_one | ||
|
|
||
| The builder prompts for srch options, sets DESTDIR/LOGFILE/LOCKFILE/DRYRUN, and invokes: | ||
| srch <args> -r /usr/local/libexec/srch_move_one <roots> | ||
|
|
||
| The mover validates each match, resolves collisions with suffixes, copies to the destination, | ||
| deletes the source, and writes audit lines to operation_log.txt. | ||
| </output> | ||
| </example> | ||
| </examples> | ||
|
|
||
| <self_check> | ||
| <checklist> | ||
| <item>The builder and mover are separated into /usr/local/bin and /usr/local/libexec.</item> | ||
| <item>srch -r invokes the mover without embedding a large heredoc in the builder.</item> | ||
| <item>Collision handling uses deterministic numeric suffixes before extensions.</item> | ||
| <item>Moves use copy+delete semantics and are logged in operation_log.txt.</item> | ||
| <item>Destination ownership and permissions are set for linaro with a+rwX.</item> | ||
| </checklist> | ||
| </self_check> | ||
|
|
||
| <evaluation_notes> | ||
| <test_cases> | ||
| <case>Multiple identical basenames across different source paths.</case> | ||
| <case>Parallel srch -r runs with lock-guarded name allocation.</case> | ||
| <case>Files disappearing between traversal and move.</case> | ||
| <case>Paths containing spaces or special characters.</case> | ||
| </test_cases> | ||
| <success_definition>All matched files land in the flat destination with correct naming, permissions, and complete audit logs.</success_definition> | ||
| </evaluation_notes> | ||
|
|
||
| <documentation> | ||
| <usage> | ||
| <step>Install /usr/local/bin/srch_flat_collect and /usr/local/libexec/srch_move_one, then run the builder with sudo.</step> | ||
| <step>Follow prompts to build the srch command, confirm the operation, and review operation_log.txt.</step> | ||
| </usage> | ||
| <known_limitations> | ||
| <limitation>Large traversals can be I/O intensive; filters should be tuned before execution.</limitation> | ||
| <limitation>Locking uses flock when available and a mkdir fallback otherwise; behavior may vary on some filesystems.</limitation> | ||
| </known_limitations> | ||
| </documentation> | ||
|
|
||
| <observations> | ||
| <item>Drafts differed on whether to allow existing destinations; this version prompts for confirmation if the destination exists.</item> | ||
| <item>Drafts used both three- and four-digit suffixes; this version standardizes on three-digit suffixes for collisions.</item> | ||
| <item>Drafts varied between embedded mover heredocs and external helpers; this version externalizes the mover for reuse.</item> | ||
| </observations> | ||
| ]] |
There was a problem hiding this comment.
The document uses a custom format with [[...]] and XML-like tags. This format is not standard Markdown and will not render correctly in most Markdown viewers (like on GitHub). Consider using standard Markdown syntax (e.g., headers, lists, bold text) to improve readability and maintainability.
1 participant
Motivation
srchcommand builder from the per-match mover so the mover can be versioned, unit-tested, and reused across runs./home/linaro, provide deterministic collision handling, maintain an auditable centralized log, and support safe cross-filesystem moves.flockwith a mkdir-lock fallback), provide aDRYRUNmode, and enforce destination ownership/permissions for operational consistency.Description
/usr/local/bin/srch_flat_collect, an interactive builder that collectssrchstart points and options, rotates and initializesoperation_log.txt, exportsDESTDIR/LOGFILE/LOCKFILE/LOCKDIR/DRYRUN, and invokessrch ... -r /usr/local/libexec/srch_move_onewith all output wrapped viafoldto avoid long unbroken lines./usr/local/libexec/srch_move_one, a standalone per-file mover that normalizes incoming paths, reserves a destination name under lock, applies deterministic numeric suffixes (_NNN) before extensions for collisions, performs copy+delete semantics to support cross-filesystem moves, and writes folded audit lines to the centralized log.flockwhen available with amkdir-based lockdir fallback, and implemented aDRYRUNpath that logs intended actions without creating files.chown -R linaro:linaroandchmod -R a+rwXon the destination and ensured both scripts check for required tooling (srch,fold) and run as root.Testing
Codex Task
Summary by Sourcery
Document the design and intended behavior of a two-script refactor that separates an interactive srch-based collector from a reusable per-file mover for flat file collection into /home/linaro with deterministic naming and audit logging.
New Features:
Documentation:
Summary by cubic
Split the srch flat collector into an interactive builder and a reusable per-file mover to simplify the workflow and make moves safer and testable. Preserves flat collection under /home/linaro with deterministic collision handling and centralized logging.
Refactors
/usr/local/bin/srch_flat_collectto buildsrchoptions interactively, rotate/init the log, exportDESTDIR/LOGFILE/LOCKFILE/LOCKDIR/DRYRUN, and invokesrch -r /usr/local/libexec/srch_move_one.fold, regular-file matching is enforced (-f), and root/tool checks are performed.New Features
/usr/local/libexec/srch_move_one: a standalone per-file mover that normalizes paths and writes auditable log lines.flockwith amkdir-based fallback; collision suffixes_NNNare applied before extensions.DRYRUNmode that logs intended actions only.linaro:linaro,a+rwX).Written for commit 02ef961. Summary will update on new commits.