fix(cmd,csync): anchor --exclude patterns at the sync root regardless of filename#10284
fix(cmd,csync): anchor --exclude patterns at the sync root regardless of filename#10284mosandlt wants to merge 1 commit into
Conversation
46bca62 to
770e9e8
Compare
mgallien
left a comment
mgallien
left a comment
There was a problem hiding this comment.
nice idea
please have a look at my comments and let me know if things are not clear
|
the PR description is missing some parts that are in the template including things relevant for our AI policy Checklist
AI (if applicable)
|
770e9e8 to
1e0a8af
Compare
|
Added the checklist back, including the AI disclosure section and an |
thanks for the quick reply |
1e0a8af to
e5db10e
Compare
|
Addressed all three review comments:
Also rebased onto current master to pick up #10283 (which landed the missing- |
e5db10e to
a52f79a
Compare
|
Rebased onto current master. Also just double-checked against your 3 review comments — all already addressed in the current HEAD (the fixes must have landed after your review, before you could see the update):
Let me know if anything still needs another pass. |
@mosandlt |
a52f79a to
0900c00
Compare
0900c00 to
01c6791
Compare
|
Good point, thanks. Force-pushed: |
Summary nextcloudcmd --exclude <path> silently produced zero exclusions for every pattern in the file, unless <path> happened to be literally named "sync-exclude.lst" -- with no warning or error anywhere. Any other filename (a typo'd extension, a descriptive name, a per-tool name like myapp-exclude.lst) caused every single pattern to be matched against the wrong base directory, so none of them ever excluded anything under the sync root. Root cause ExcludedFiles::addExcludeFilePath() in src/csync/csync_exclude.cpp anchors patterns either at the sync root (_localPath) or at the exclude file's own containing directory, based on a filename check: literally "sync-exclude.lst" -> sync root, anything else -> own directory. That heuristic was introduced in 5788f35 (2020-12-09, "Skip sync exclude file from list of exclude files if it doesn't exist") to merge two previously-separate code paths: the GUI's global exclude list (always named sync-exclude.lst, lives outside any sync folder, needs sync-root anchoring) and a per-directory .sync-exclude.lst discovered during traversal (needs anchoring at its own directory). It was a side effect of that merge, not a deliberate choice -- the commit's own description and linked issue are unrelated to anchoring behavior. src/cmd/cmd.cpp passes the CLI's --exclude value into the same function unconditionally, so it inherited this GUI-specific heuristic even though CLI users can name their exclude file anything. Reported independently by multiple users: nextcloud#2916, nextcloud#7682, and a comment diagnosing the exact same filename dependency on nextcloud#2916. Fix Added an explicit anchorToLocalPath parameter (now the ExcludedFiles::ExcludeFileAnchor enum) to addExcludeFilePath() in csync_exclude.cpp/.h (default FileDirectory, preserving current behavior everywhere else). Per @mgallien's review, --exclude's own anchoring behavior is left unchanged (still FileDirectory / the historic filename heuristic) so existing setups relying on it, however unintentionally, don't silently change behavior. Instead cmd.cpp gets a new, separate --exclude-anchored [file] option that always anchors at the sync root (ExcludeFileAnchor::SyncRoot) regardless of the file's name -- use it instead of --exclude when patterns aren't matching because the exclude file isn't literally named "sync-exclude.lst". Test plan - [x] New unit test: testAddExcludeFilePath_syncRootAnchor_bypassesFilenameHeuristic (covers both ExcludeFileAnchor values directly on ExcludedFiles) - [x] Full ExcludedFilesTest suite: 25 passed, 0 failed - [x] Manual repro against a live Nextcloud account: an exclude file not named sync-exclude.lst had zero effect via --exclude (unchanged, by design); passed via --exclude-anchored instead, its patterns are correctly applied (FileIgnored) with no behavior change for --exclude or GUI-managed exclude lists. Signed-off-by: Thomas Mosandl <mosandl@concreativ.de> Assisted-by: ClaudeCode:claude-sonnet-5
01c6791 to
62ec8a0
Compare
|
Artifact containing the AppImage: nextcloud-appimage-pr-10284.zip Digest: To test this change/fix you can download the above artifact file, unzip it, and run it. Please make sure to quit your existing Nextcloud app and backup your data. |
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
3 participants
Summary
nextcloudcmd --exclude <path>silently produced zero exclusions for every pattern in the file, unless<path>happened to be literally namedsync-exclude.lst— with no warning or error anywhere. Any other filename (a typo'd extension, a descriptive name, a per-tool name likemyapp-exclude.lst) caused every single pattern to be matched against the wrong base directory, so none of them ever excluded anything under the sync root.Root cause
ExcludedFiles::addExcludeFilePath()insrc/csync/csync_exclude.cppanchors patterns either at the sync root (_localPath) or at the exclude file's own containing directory, based on a filename check: literallysync-exclude.lst→ sync root, anything else → own directory.That heuristic was introduced in 5788f35 (2020-12-09, "Skip sync exclude file from list of exclude files if it doesn't exist") to merge two previously-separate code paths: the GUI's global exclude list (always named
sync-exclude.lst, lives outside any sync folder, needs sync-root anchoring) and a per-directory.sync-exclude.lstdiscovered during traversal (needs anchoring at its own directory). It was a side effect of that merge, not a deliberate choice — the commit's own description and linked issue are unrelated to anchoring behavior.src/cmd/cmd.cpppasses the CLI's--excludevalue into the same function unconditionally, so it inherited this GUI-specific heuristic even though CLI users can name their exclude file anything.Reported independently by multiple users, never properly root-caused:
Fix
Add an explicit
anchorToLocalPathparameter toaddExcludeFilePath()(defaultfalse, preserving current behavior everywhere else) and set it for the CLI's--excludeoption incmd.cpp, so its patterns are always anchored at the sync root regardless of the file's name.Test plan
testAddExcludeFilePath_anchorToLocalPath_bypassesFilenameHeuristicExcludedFilesTestsuite: 25 passed, 0 failed--excludefile not namedsync-exclude.lstpreviously had zero effect (excluded directories were downloaded anyway); after this fix they are correctly skipped (FileIgnored) with no behavior change for GUI-managed exclude listsChecklist
AI (if applicable)