Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Selective sync #1712

Closed
wants to merge 3 commits into from

Conversation

AudriusButkevicius
Copy link
Member

Basically, request for comments/help regarding this pruner business, as it looks really bad.

Especially the fact that I can't toggle stuff on or off without having to go through the config, and relying on the order the AJAX calls are made to potentially detect that the flag has been toggled.

Alternatively I could rely on a nil slice to indicate it's on or off, but that's easy to mess up from the API consumer perspective.

return true
}

func (p *Pruner) ShouldSkipDirectory(path string) bool {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't both ShouldSkipFile and ShouldSkipDir be a simple check for filepath.HasPrefix on any of the patterns? The answer is only yes if it's a dir mentioned directly, or some subpath of a mentioned dir, no?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well a file will never be a prefix of any of the patterns.
A pattern might be a prefix to one of the files, but that does not imply that we need to sync it, as the base directory of the file has to explicitly match the pattern (minus the [^/]+) bit?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I guess I should add / to all dir patterns and to all dir paths passed in.

@calmh
Copy link
Member

calmh commented Apr 27, 2015

Looks generally sane to me, as far as the pruner stuff goes. The GetSelections() is 😱 so I avoided looking at that one so far, and the GUI stuff. :)

@calmh
Copy link
Member

calmh commented Apr 28, 2015

@st-jenkins retest this please

@AudriusButkevicius
Copy link
Member Author

@st-jenkins run integration tests please

@AudriusButkevicius
Copy link
Member Author

Need to address the comment I made, and some tests.

@AudriusButkevicius
Copy link
Member Author

So pruner now deals with \\ vs / in filepaths.

The part which builds the tree sort of does too, but doesn't do proper anchoring (ending patterns with /), hence a pattern a/b/c would still match both a/b/c and a/b/cow.
Ideally we could reuse the pruner there (to potentially make the code less ugly), but that would increase runtime, due to having iterate N patterns 3 times (for self, parent, grandparent), versus how it's now, potentially no iteration, up to a maximum of 1 iteration over N patterns.

@calmh calmh added enhancement New features or improvements of some kind, as opposed to a problem (bug) and removed pr-enhancement labels May 7, 2015
@calmh
Copy link
Member

calmh commented May 11, 2015

(I've taken over this branch and rebased it on top of current master, will close this as this is out of date; look for a new one in time.)

@calmh calmh closed this May 11, 2015
@st-review st-review added the frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion label Jun 16, 2017
@syncthing syncthing locked and limited conversation to collaborators Jun 16, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New features or improvements of some kind, as opposed to a problem (bug) frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants