git_pull: Fix data loss of .storage and unrelated local files on fresh clone#4356
git_pull: Fix data loss of .storage and unrelated local files on fresh clone#4356BrianTillman wants to merge 1 commit intohome-assistant:masterfrom
Conversation
📝 WalkthroughWalkthroughFixes two issues: restores non-YAML backup files correctly during git-clone by fixing the extglob copy logic, and prevents an undefined Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
| cp "${BACKUP_LOCATION}" "!(*.yaml)" /config 2>/dev/null | ||
| # try to copy non-yaml files back (use eval to avoid parse-time extglob error) | ||
| shopt -s extglob nullglob dotglob | ||
| eval 'cp -r "${BACKUP_LOCATION}"/!(*.yaml|*.yml) /config 2>/dev/null' || true |
There was a problem hiding this comment.
Maybe it would be better to use find here?
There was a problem hiding this comment.
Great suggestion! I considered both approaches:
find alternative:
find "${BACKUP_LOCATION}" -maxdepth 1 -mindepth 1 ! -name "*.yaml" ! -name "*.yml" -exec cp -r {} /config/ \; 2>/dev/null || true
Trade-offs:
- find is more portable (POSIX) and doesn't require shell option toggling
- eval + extglob is a common bash pattern and keeps it consistent with the existing rm -rf /config/{,.[!.],..?}* pattern on line 53
Fixed the bug in place rather than introducing a different approach - easier to review and less risk. Happy to switch to find if preferred, or follow up with additional PRs to refactor the backup and restore functionality.
- Use eval to wrap extglob pattern, avoiding parse-time syntax error - Initialize OLD_COMMIT variable and handle fresh clone case in validate-config - Restore subdirectories and hidden files from backup
600e47b to
4826d91
Compare
Summary
This commit fixes two issues in the git_pull addon:
Extglob syntax: Fix broken extglob copy syntax that prevented non-YAML files from being restored after git clone.
cp "${BACKUP_LOCATION}" "!(*.yaml)"was syntactically incorrect - it treated the backup location as a source file and the extglob pattern as a separate argument.!(*.yaml|*.yml)are parsed at script load time, beforeshopt -s extglobexecutes. Fixed by wrapping thecpcommand inevalto delay pattern parsing until runtime.dotglobto preserve hidden directories like.storage/nullgloband adds|| truefor graceful handling when no files matchOLD_COMMIT unbound variable: Caused the process to fail on fresh git clones.
git-cloneruns (fresh clone),OLD_COMMITwas never set, causingvalidate-configto fail. Fixed by initializingOLD_COMMIT=""and adding fresh clone handling that validates config without restart logic.Fixes #3547 - Users reported that
.storage/andcustom_components/folders were being deleted after fresh clone because the backup restore silently failed.Test plan
This PR was validated using a dedicated test fixtures repository:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.