Dev-new-fixes-21 #104
Dev-new-fixes-21 #104
Conversation
Signed-off-by: DaRK AnGeL <[email protected]>
Signed-off-by: DaRK AnGeL <[email protected]>
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a new moveEmptyFolders option (default "yes") to defaults and UI; uses it to include/exclude empty directories from find filters; fixes TESTMODE string comparisons; adds conditional rmdir cleanup for moved empty folders; and separates remaining-item counters/logs into files vs folders. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Files/areas needing extra attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @masterwishx, 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 refines 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 introduces several fixes and enhancements to the age_mover script. The changes include correctly quoting a variable in a test condition, adding functionality to clean up empty directories after they are moved, and improving log messages to differentiate between files and folders.
While the intent of the changes is good, I've found a few issues:
- There is a critical bug in the new logic for cleaning empty directories that will cause the script to fail with an error.
- The enhanced logging has introduced code duplication in a couple of places, which impacts maintainability.
I have provided detailed comments with code suggestions to address these issues. Please review them.
| if [ -e "${DIR}/.placeholder" ]; then | ||
| file_count=".placeholder" | ||
| else | ||
| file_count=$(find "$DIR" -type f | wc -l) | ||
| fi | ||
| # Count subdirectories | ||
| dir_count=$(find "$DIR" -mindepth 1 -type d | wc -l) | ||
|
|
||
| if [ "$file_count" -eq 0 ] && [ "$dir_count" -eq 0 ] && [ "$file_count" != ".placeholder" ]; then | ||
| if [ "$TESTMODE" = "no" ]; then | ||
| mvlogger "Deleting moved empty folder: ${DIR}" | ||
| rmdir "$DIR" | ||
| else | ||
| mvlogger "TEST MODE: Would delete moved empty folder: ${DIR}" | ||
| fi | ||
| else | ||
| mvlogger "Not deleting folder: contains $file_count file(s), $dir_count subfolder(s): $DIR" | ||
| fi |
There was a problem hiding this comment.
The logic for handling .placeholder files is buggy and will cause the script to fail. When a .placeholder file is found, file_count is set to the string ".placeholder". The subsequent condition [ "$file_count" -eq 0 ] then attempts an arithmetic comparison on a string, which results in an integer expression expected error. The logic should be refactored to correctly handle the .placeholder case separately to avoid this runtime error.
| if [ -e "${DIR}/.placeholder" ]; then | |
| file_count=".placeholder" | |
| else | |
| file_count=$(find "$DIR" -type f | wc -l) | |
| fi | |
| # Count subdirectories | |
| dir_count=$(find "$DIR" -mindepth 1 -type d | wc -l) | |
| if [ "$file_count" -eq 0 ] && [ "$dir_count" -eq 0 ] && [ "$file_count" != ".placeholder" ]; then | |
| if [ "$TESTMODE" = "no" ]; then | |
| mvlogger "Deleting moved empty folder: ${DIR}" | |
| rmdir "$DIR" | |
| else | |
| mvlogger "TEST MODE: Would delete moved empty folder: ${DIR}" | |
| fi | |
| else | |
| mvlogger "Not deleting folder: contains $file_count file(s), $dir_count subfolder(s): $DIR" | |
| fi | |
| if [ -e "${DIR}/.placeholder" ]; then | |
| mvlogger "Not deleting folder: contains .placeholder file: $DIR" | |
| else | |
| file_count=$(find "$DIR" -type f | wc -l) | |
| # Count subdirectories | |
| dir_count=$(find "$DIR" -mindepth 1 -type d | wc -l) | |
| if [ "$file_count" -eq 0 ] && [ "$dir_count" -eq 0 ]; then | |
| if [ "$TESTMODE" = "no" ]; then | |
| mvlogger "Deleting moved empty folder: ${DIR}" | |
| rmdir "$DIR" | |
| else | |
| mvlogger "TEST MODE: Would delete moved empty folder: ${DIR}" | |
| fi | |
| else | |
| mvlogger "Not deleting folder: contains $file_count file(s), $dir_count subfolder(s): $DIR" | |
| fi | |
| fi |
| if [[ "$ACTION" =~ "unattended to secondary" ]]; then | ||
| mvlogger "$REMAINING_UNATTENDED_FILES files remaining from unattended to secondary ($(bytes_to_human "$REMAINING_UNATTENDED_SIZE"))" | ||
| # Check if there are any files or folders left | ||
| if [[ "$FILETYPE" = "f" ]]; then | ||
| mvlogger "$REMAINING_UNATTENDED_FILES files remaining from unattended to secondary ($(bytes_to_human "$REMAINING_UNATTENDED_SIZE"))" | ||
| else | ||
| mvlogger "$REMAINING_UNATTENDED_FOLDERS folders remaining from unattended to secondary" | ||
| fi | ||
| elif [[ "$ACTION" =~ "unattended to primary" ]]; then | ||
| mvlogger "$REMAINING_UNATTENDED_FILES files remaining from unattended to primary ($(bytes_to_human "$REMAINING_UNATTENDED_SIZE"))" | ||
| # Check if there are any files or folders left | ||
| if [[ "$FILETYPE" = "f" ]]; then | ||
| mvlogger "$REMAINING_UNATTENDED_FILES files remaining from unattended to primary ($(bytes_to_human "$REMAINING_UNATTENDED_SIZE"))" | ||
| else | ||
| mvlogger "$REMAINING_UNATTENDED_FOLDERS folders remaining from unattended to primary" | ||
| fi | ||
| fi |
There was a problem hiding this comment.
This block of code contains duplicated logic for logging messages based on FILETYPE. This can be simplified by extracting the common parts and using a variable for the differing part (primary vs secondary), which improves maintainability and readability.
| if [[ "$ACTION" =~ "unattended to secondary" ]]; then | |
| mvlogger "$REMAINING_UNATTENDED_FILES files remaining from unattended to secondary ($(bytes_to_human "$REMAINING_UNATTENDED_SIZE"))" | |
| # Check if there are any files or folders left | |
| if [[ "$FILETYPE" = "f" ]]; then | |
| mvlogger "$REMAINING_UNATTENDED_FILES files remaining from unattended to secondary ($(bytes_to_human "$REMAINING_UNATTENDED_SIZE"))" | |
| else | |
| mvlogger "$REMAINING_UNATTENDED_FOLDERS folders remaining from unattended to secondary" | |
| fi | |
| elif [[ "$ACTION" =~ "unattended to primary" ]]; then | |
| mvlogger "$REMAINING_UNATTENDED_FILES files remaining from unattended to primary ($(bytes_to_human "$REMAINING_UNATTENDED_SIZE"))" | |
| # Check if there are any files or folders left | |
| if [[ "$FILETYPE" = "f" ]]; then | |
| mvlogger "$REMAINING_UNATTENDED_FILES files remaining from unattended to primary ($(bytes_to_human "$REMAINING_UNATTENDED_SIZE"))" | |
| else | |
| mvlogger "$REMAINING_UNATTENDED_FOLDERS folders remaining from unattended to primary" | |
| fi | |
| fi | |
| local direction | |
| if [[ "$ACTION" =~ "unattended to secondary" ]]; then | |
| direction="secondary" | |
| elif [[ "$ACTION" =~ "unattended to primary" ]]; then | |
| direction="primary" | |
| fi | |
| if [[ -n "$direction" ]]; then | |
| # Check if there are any files or folders left | |
| if [[ "$FILETYPE" = "f" ]]; then | |
| mvlogger "$REMAINING_UNATTENDED_FILES files remaining from unattended to $direction ($(bytes_to_human "$REMAINING_UNATTENDED_SIZE"))" | |
| else | |
| mvlogger "$REMAINING_UNATTENDED_FOLDERS folders remaining from unattended to $direction" | |
| fi | |
| fi |
| if [[ "$SOURCE" =~ $PRIMARYSTORAGENAME ]]; then | ||
| REMAINING_CACHE_SIZE=$((REMAINING_CACHE_SIZE - FILESIZE)) | ||
| # Check if there are any files or folders left | ||
| if [[ "$FILETYPE" = "f" ]]; then | ||
| REMAINING_CACHE_FILES=$((REMAINING_CACHE_FILES - NBLINKS)) | ||
| mvlogger "$REMAINING_CACHE_FILES files remaining from caches to array ($(bytes_to_human "$REMAINING_CACHE_SIZE"))" | ||
| else | ||
| REMAINING_CACHE_FOLDERS=$((REMAINING_CACHE_FOLDERS - 1)) | ||
| mvlogger "$REMAINING_CACHE_FOLDERS folders remaining from caches to array" | ||
| fi | ||
| mvlogger "$REMAINING_CACHE_FILES files remaining from caches to array ($(bytes_to_human "$REMAINING_CACHE_SIZE"))" | ||
| else | ||
| REMAINING_ARRAY_SIZE=$((REMAINING_ARRAY_SIZE - FILESIZE)) | ||
| # Check if there are any files or folders left | ||
| if [[ "$FILETYPE" = "f" ]]; then | ||
| REMAINING_ARRAY_FILES=$((REMAINING_ARRAY_FILES - NBLINKS)) | ||
| mvlogger "$REMAINING_ARRAY_FILES files remaining from array to caches ($(bytes_to_human "$REMAINING_ARRAY_SIZE"))" | ||
| else | ||
| REMAINING_ARRAY_FOLDERS=$((REMAINING_ARRAY_FOLDERS - 1)) | ||
| mvlogger "$REMAINING_ARRAY_FOLDERS folders remaining from array to caches" | ||
| fi | ||
| mvlogger "$REMAINING_ARRAY_FILES files remaining from array to caches ($(bytes_to_human "$REMAINING_ARRAY_SIZE"))" | ||
| fi |
There was a problem hiding this comment.
There is significant code duplication in this if/else block. The logic for decrementing counters and logging is repeated for both cache-to-array and array-to-cache movements. This can be refactored by using local variables to hold the context-specific values and then having a single block for logging. This will make the code more concise and easier to maintain.
local log_msg
if [[ "$SOURCE" =~ $PRIMARYSTORAGENAME ]]; then
REMAINING_CACHE_SIZE=$((REMAINING_CACHE_SIZE - FILESIZE))
# Check if there are any files or folders left
if [[ "$FILETYPE" = "f" ]]; then
REMAINING_CACHE_FILES=$((REMAINING_CACHE_FILES - NBLINKS))
log_msg="$REMAINING_CACHE_FILES files remaining from caches to array ($(bytes_to_human "$REMAINING_CACHE_SIZE"))"
else
REMAINING_CACHE_FOLDERS=$((REMAINING_CACHE_FOLDERS - 1))
log_msg="$REMAINING_CACHE_FOLDERS folders remaining from caches to array"
fi
else
REMAINING_ARRAY_SIZE=$((REMAINING_ARRAY_SIZE - FILESIZE))
# Check if there are any files or folders left
if [[ "$FILETYPE" = "f" ]]; then
REMAINING_ARRAY_FILES=$((REMAINING_ARRAY_FILES - NBLINKS))
log_msg="$REMAINING_ARRAY_FILES files remaining from array to caches ($(bytes_to_human "$REMAINING_ARRAY_SIZE"))"
else
REMAINING_ARRAY_FOLDERS=$((REMAINING_ARRAY_FOLDERS - 1))
log_msg="$REMAINING_ARRAY_FOLDERS folders remaining from array to caches"
fi
fi
mvlogger "$log_msg"![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/age_mover(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (4)
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/age_mover (4)
1651-1651: Good defensive quoting fix.Quoting
$TESTMODEprevents word splitting and handles empty values correctly.
1915-1928: LGTM!The conditional logging correctly distinguishes between files and folders, providing appropriate context (size for files, count only for folders).
1936-1939: LGTM!Separate logging for files vs. folders from cache to array is consistent with the pattern used elsewhere.
1946-1949: LGTM!Logging for array-to-cache moves is consistent with the cache-to-array pattern.
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/age_mover
Show resolved
Hide resolved
Signed-off-by: DaRK AnGeL <[email protected]>
Signed-off-by: DaRK AnGeL <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/age_mover (2)
1930-1944: Per-type logging correctly implemented.The logging now correctly differentiates between files and folders, displaying size information only for files. The code duplication noted in past reviews is a minor concern; the current structure is readable.
1946-1966: Per-type counter updates and logging are correct.The remaining counters for files and folders are correctly decremented based on
FILETYPE, and the log messages appropriately distinguish between files (with size) and folders (without size). The duplication noted in past reviews is a stylistic concern that doesn't affect functionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/Mover.tuning.page(1 hunks)source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/age_mover(5 hunks)source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/default.cfg(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (6)
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/default.cfg (1)
34-34: LGTM!The new
moveEmptyFoldersconfiguration option is correctly added with a default value of"yes", which aligns with the default handling in theage_moverscript and the new UI control.source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/Mover.tuning.page (1)
641-644: LGTM!The new "Top Folder" option for
cleanFoldersis correctly implemented with a descriptive help text explaining the behavior.source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/age_mover (4)
695-706: LGTM!The
moveEmptyFolderssetting handling follows the established pattern used by other configuration options in this function, with proper defaults and logging.
1081-1086: LGTM!The find filter correctly adjusts based on
MOVE_EMPTY_FOLDERS:
- When
"yes": includes both files and empty directories with-mindepth 1- When not
"yes": only includes filesThe logic is sound and the
-mindepth 1ensures the share root isn't inadvertently matched.
1666-1668: LGTM!The quoted string comparison
[ "$TESTMODE" = "yes" ]is correct and POSIX-compliant.
1892-1918: Previous review issue has been addressed.The
.placeholderhandling now correctly uses short-circuit evaluation: the string comparison[ "$file_count" != ".placeholder" ]is evaluated first, preventing the integer comparison[ "$file_count" -eq 0 ]from running whenfile_countis".placeholder". This resolves the "integer expression expected" error flagged in earlier reviews.
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/Mover.tuning.page
Outdated
Show resolved
Hide resolved
Signed-off-by: DaRK AnGeL <[email protected]>
Signed-off-by: DaRK AnGeL <[email protected]>
Signed-off-by: DaRK AnGeL <[email protected]>
Signed-off-by: DaRK AnGeL <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/Mover.tuning.page(29 hunks)source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/Share.tuning.page(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (4)
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/Share.tuning.page (1)
430-430: LGTM! Proper output sanitization added.The addition of
htmlspecialcharswithENT_QUOTES | ENT_SUBSTITUTEflags properly escapes user input in HTML attributes, preventing potential XSS vulnerabilities.Also applies to: 445-445
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/Mover.tuning.page (3)
626-633: New feature implementation looks good.The moveEmptyFolders option is properly structured with the label preceding the select element (not inside it), resolving the HTML structure issue from the previous review. The feature includes clear help text explaining its behavior.
239-239: LGTM! Input sanitization added.User input fields are now properly sanitized using
htmlspecialchars, preventing potential XSS vulnerabilities when rendering values in HTML attributes.Also applies to: 324-324, 343-343, 348-348, 546-546, 562-562
156-163: LGTM! Test mode warning display is well-implemented.The warning message provides immediate visual feedback when test mode is enabled, using both server-side PHP for initial state and client-side JavaScript for dynamic toggling. The implementation is clean and user-friendly.
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/Mover.tuning.page
Outdated
Show resolved
Hide resolved
Signed-off-by: DaRK AnGeL <[email protected]>
Signed-off-by: DaRK AnGeL <[email protected]>
Signed-off-by: DaRK AnGeL <[email protected]>
Signed-off-by: DaRK AnGeL <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/age_mover (1)
1819-1841: Minor bug in top-level folder logging and redundant.placeholdercondition.In the top-folder cleanup branch:
- The log line uses
countinstead oftop_folder_countfor pluralization and.placeholderchecking:mvlogger "Not deleting top-level folder containing $top_folder_count file$([ "$count" != ".placeholder" ] && echo "s"): ${top_folder}"
countrefers to the last per-folder counter, not the top-level one, so the message can be inconsistent.
- Because
top_folder_countis always overwritten byfind "$top_folder" -type f | wc -l, it will never be the string.placeholder, making the!= ".placeholder"test dead code.A minimal fix that keeps behavior the same but makes the code correct and clearer:
- if [[ "$top_folder_count" != ".placeholder" && "$top_folder_count" -eq 0 ]]; then + if [ "$top_folder_count" -eq 0 ]; then mvlogger "Deleting empty top-level folder: $top_folder" # Check if empty top-level folder exist and remove it [ -d "$top_folder" ] && find "$top_folder" -empty -type d -delete else - mvlogger "Not deleting top-level folder containing $top_folder_count file$([ "$count" != ".placeholder" ] && echo "s"): ${top_folder}" + mvlogger "Not deleting top-level folder containing $top_folder_count file$([ "$top_folder_count" != "1" ] && echo "s"): ${top_folder}" fi
🧹 Nitpick comments (2)
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/Mover.tuning.page (1)
622-638: moveEmptyFolders / cleanFolders UX is consistent with backend logic.The new
moveEmptyFoldersselect (yes/no) and existingcleanFolders(no/yes/topFolder) are correctly named to matchage_mover’sMOVE_EMPTY_FOLDERSandCLEANFOLDERSvariables and clearly describe behavior (moving vs deleting empty folders). If you want to avoid confusion for users, you might optionally mention in the help text that disabling “Move empty folders” means only folders associated with moved files are affected by “Clean empty folders”, while unrelated empty folders stay put.source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/age_mover (1)
1082-1087: Find filter for MOVE_EMPTY_FOLDERS is sound; consider symmetric-mindepth 1.The
findexpression correctly:
- Includes
-mindepth 1 \( -type f -o -type d -empty \)whenMOVE_EMPTY_FOLDERS=="yes"(files plus empty dirs, excluding the share root).- Falls back to
-type fonly when disabled (no directories in the action list).Functionally this is fine: the share root is a directory, so it’s never selected in the
-type fbranch. For consistency and to avoid ever touching the root in future changes, you might optionally mirror-mindepth 1in theelsebranch:- else - FINDSTR+=" -type f" - fi + else + FINDSTR+=" -mindepth 1 -type f" + fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/Mover.tuning.page(27 hunks)source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/age_mover(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (5)
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/Mover.tuning.page (2)
149-163: Test Mode warning toggle is correctly wired and stateful.The
testmodeselect,testmode_warningspan, andtoggleTestModeWarning()function use consistent"yes"/"no"values and correctly initialize the warning visibility from$cfg['testmode']. No functional issues seen.
169-176: Filename validation UI and invalid character input correctly match backend expectations.The
validateFilenamestoggle,invalidFileCharstext input, and their help text align with the shell-side validation (validate_filename+invalidFileCharshandling). Usinghtmlspecialchars($cfg['invalidFileChars'])prevents HTML injection, and the fixedclass='myinvalidFileChars'makes the markup valid and styleable. No further changes needed here.Also applies to: 189-206, 656-685
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/age_mover (3)
697-706: MOVE_EMPTY_FOLDERS config handling looks correct and inherits sensibly.The new
moveEmptyFoldersblock sets a persistent default of"yes"in the global config and logs it, while share override configs inherit the global value unless they define their ownmoveEmptyFolders. This matches how other settings behave and aligns with the new UI option.
1893-1921: Empty-folder cleanup after moving directories is correct and handles.placeholdersafely.The new “Clean moved empty folders” block only runs for directory actions (
FILETYPE=="d") withACTIONcontainingmoveandMOVE_EMPTY_FOLDERS="yes", and:
- Treats
.placeholderas a keep-alive marker, avoiding integer comparisons whenfile_count=".placeholder".- Counts both regular files and subdirectories to ensure the directory is truly empty.
- Respects
TESTMODEwhen deciding whether tormdiror just log.This correctly fixes the previous
.placeholderinteger-comparison issue and ensures moved empty directories are tidied without affecting placeholder-protected folders.
1933-1968: Remaining-items counters and logs are now per-type and much clearer.The updated remaining-count logic splits files vs folders for unattended/cache/array directions (
REMAINING_*_FILESvsREMAINING_*_FOLDERS) and logs them separately. This avoids the previous ambiguity where folders were folded into “files remaining” and matches the new statistics written inmoverStatusWrite().
Signed-off-by: DaRK AnGeL <[email protected]>
|
@coderabbitai review last commit |
|
@coderabbitai summarize |
|
Tip For best results, initiate chat on the files or code changes. @masterwishx, here's a summary of PR #104: Summary of ChangesThis PR introduces enhanced control and logging for the mover script with the following key improvements: Core Functionality
Configuration & UI
Styling Updates
Technical Details
Estimated Review Effort: Medium to High |
🧩 Analysis chain🏁 Script executed: Unrecoverable error: Failed to clone repository @masterwishx I'm unable to access the git repository directly to identify and review the specific last commit. However, I can help you in the following ways:
Would you like me to trigger a full review of the PR? ✅ Actions performedReview triggered.
|
After last commit it's OK? |
Signed-off-by: DaRK AnGeL <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/age_mover (2)
1823-1836: Consider clarifying .placeholder logging.The logic correctly avoids integer comparison errors by checking
"$top_folder_count" != ".placeholder"before-eq 0(line 1831). However, when.placeholderis present, line 1836 logs "Not deleting top-level folder containing .placeholder file: ..." which could be clearer.Consider handling
.placeholderseparately for clearer messaging:if [ -e "${top_folder}/.placeholder" ]; then - top_folder_count=".placeholder" + mvlogger "Not deleting top-level folder: contains .placeholder file: ${top_folder}" else # Count files in Top Folder if exist [ -d "$top_folder" ] && top_folder_count="$(find "$top_folder" -type f | wc -l)" mvDebuglogger "Current top folder: $top_folder, Count of files in Top Folder: $top_folder_count" "Clean Top Folder - Start Loop" - fi - if [[ "$top_folder_count" != ".placeholder" && "$top_folder_count" -eq 0 ]]; then + if [[ "$top_folder_count" -eq 0 ]]; then # Check if empty top-level folder exist and remove it [ -d "$top_folder" ] && find "$top_folder" -empty -type d -delete mvlogger "Deleting empty top-level folder: $top_folder" - else + else mvlogger "Not deleting top-level folder containing $top_folder_count file$([ "$top_folder_count" != ".placeholder" ] && echo "s"): ${top_folder}" + fi fi
1890-1918: LGTM! Empty folder cleanup logic is correct.The block correctly handles cleaning moved empty folders:
- ✅ Checks for
.placeholderfile before integer comparisons (avoids "integer expression expected" error noted in past reviews)- ✅ Counts both files and subdirectories
- ✅ Only deletes when both counts are 0 and no
.placeholderexists- ✅ Respects
TESTMODEThe logic is sound. Optionally, the
.placeholderhandling could be refactored similarly to the suggestion for top-level folders (lines 1823-1836) to make logging clearer when.placeholderis present.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/age_mover(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (6)
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/age_mover (6)
73-73: LGTM! Variable initialization follows existing pattern.The
LAST_FOLDER_COUNTvariable is initialized consistently withLAST_COUNTabove it and is used later for tracking remaining folder counts during cleanup operations.
696-706: LGTM! Configuration handling is consistent.The
moveEmptyFoldersconfiguration follows the established pattern for other settings, with a sensible default of "yes" and proper logging.
1082-1087: LGTM! Find filter correctly implements moveEmptyFolders feature.The conditional logic properly includes empty directories (
-type d -empty) whenMOVE_EMPTY_FOLDERSis "yes" and excludes them otherwise. The use of-mindepth 1ensures the share root itself is not included.
1667-1667: LGTM! Proper quoting of variable.Quoting
"$TESTMODE"is a good defensive practice that prevents word splitting and globbing issues.
1920-1944: LGTM! Per-type logging improves clarity.The remaining-item counters and logs are now separated by type (files vs folders), making progress messages more precise and easier to understand.
1946-1967: LGTM! Per-type counters and logging are correctly implemented.The cache/array remaining-item logs now distinguish files from folders, providing clearer progress tracking. The logic correctly decrements
REMAINING_CACHE_FILESbyNBLINKS(to handle hardlinks) while decrementingREMAINING_CACHE_FOLDERSby 1.
Signed-off-by: DaRK AnGeL [email protected]
Summary by CodeRabbit
New Features
Bug Fixes
Style/UX
✏️ Tip: You can customize this high-level summary in your review settings.