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

Directory tree depth limit #1067

Closed
moscicki opened this issue Oct 3, 2013 · 18 comments
Closed

Directory tree depth limit #1067

moscicki opened this issue Oct 3, 2013 · 18 comments
Assignees
Labels
ReadyToTest QA, please validate the fix/enhancement sev2-high type:bug
Milestone

Comments

@moscicki
Copy link
Contributor

moscicki commented Oct 3, 2013

Above 50 levels of directories the sync breaks however the sync client does not report an error (RC=0).

If there is a policy on a maximum allowed tree depth then it should be handled somehow properly.


Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

@karlitschek
Copy link

agreed. More than 50 directories should a) work or b) reported as proper error.
This could be a server issue too of course.

@ogoffart
Copy link
Contributor

I can reproduce the problem.

It seems that, in the update phase, the subdirectories are not seen.

@ogoffart
Copy link
Contributor

That's because csync hardcode a maximum depth.
In csync_private.h

/**
 * How deep to scan directories.
 */
#define MAX_DEPTH 50

I beleive the reason is not to be fooled by symbolic link loops.
We could mark files in this depth as IGNORED and report that to the user

@moscicki
Copy link
Contributor Author

Ignoring and reporting this clearly to the users sounds good enough.

Many thanks,

kuba

On Oct 14, 2013, at 3:01 PM, Olivier Goffart notifications@github.com wrote:

That's because csync hardcode a maximum depth.
In csync_private.h

/**

  • How deep to scan directories.
    */
    #define MAX_DEPTH 50
    I beleive the reason is not to be fooled by symbolic link loops.
    We could mark files in this depth as IGNORED and report that to the user


Reply to this email directly or view it on GitHub.

@danimo
Copy link
Contributor

danimo commented Nov 26, 2013

@dragotin Can we bubble this up the the UI easily?

@dragotin
Copy link
Contributor

We could set the define to a higher value, ie. 250 maybe. Should we do that?

@guruz
Copy link
Contributor

guruz commented Jul 17, 2014

What is the minimum of the maximum depth of all OSes we support?
Although it gets worse thinking about all the storage backend the server supports.
Maybe the client should not limit at all (e.g. 250) and we rely on server errors if it gets too deep

@guruz guruz added the Bug label Jul 17, 2014
@guruz guruz changed the title problem syncing deep directory trees Directory tree depth limit Jul 17, 2014
@hodyroff
Copy link

hodyroff commented Dec 8, 2016

Does this still exist as an issue?

@ogoffart
Copy link
Contributor

ogoffart commented Dec 8, 2016

yes

@hodyroff
Copy link

@pmaier1 @michaelstingl to define wanted behaviour and then lets implement it. In any case, when we reach the limit we should have a defined behaviour and not just break.

@michaelstingl
Copy link
Contributor

Limit 50 seems to work. We should display a error message in the UI.

@michaelstingl
Copy link
Contributor

michaelstingl commented Dec 22, 2016

@ogoffart Milestone?

@michaelstingl
Copy link
Contributor

@ogoffart Could me make this configurable with a environment variable?

@pmaier1
Copy link

pmaier1 commented Dec 22, 2016

For now we think a limit of 50 is fine - at least until we hear something else. Anyway we need proper error handling and docs that explain the reason for such limit (symlink loop limiting). In case the directory depth exceeds the limit, the client should show an error message like "Could not be synced due to exceeded directory depth (50). See ".

settermjd added a commit to settermjd/client that referenced this issue Jan 17, 2017
As requested in owncloud#1067, this commit adds an entry to the FAQ to highlight
the fact that the sync client will never scan greater than 50
sub-directories.
@hodyroff hodyroff added this to the 2.4.0 milestone Feb 9, 2017
@ogoffart ogoffart self-assigned this Apr 28, 2017
ogoffart added a commit that referenced this issue Apr 28, 2017
Before this patch, to deep folder would just be ignored, without any feedback.
This patch makes it so deep folder are properly shown as ignored in the UI.

Also increase the MAX_DEPTH

Issue: #1067
ogoffart added a commit that referenced this issue May 5, 2017
Before this patch, to deep folder would just be ignored, without any feedback.
This patch makes it so deep folder are properly shown as ignored in the UI.

Also increase the MAX_DEPTH

Issue: #1067
guruz pushed a commit that referenced this issue May 8, 2017
Before this patch, to deep folder would just be ignored, without any feedback.
This patch makes it so deep folder are properly shown as ignored in the UI.

Also increase the MAX_DEPTH

Issue: #1067
@guruz guruz added the ReadyToTest QA, please validate the fix/enhancement label May 9, 2017
@michaelstingl
Copy link
Contributor

What needs to be tested? @SamuAlfageme ? Is there already an error message as @pmaier1 suggested?

@ckamm
Copy link
Contributor

ckamm commented Oct 11, 2017

I've tested the current behavior: The too-deep directory is not synced and is shown in the "not synced" list of files. It's a silent error, similar to other files being excluded due to hiddenness or by exclude pattern.

@hodyroff
Copy link

Perfect IMO, if anybody thinks it should be more then 50, please raise your voice now! We can test more and up the limit ...

@ckamm
Copy link
Contributor

ckamm commented Oct 12, 2017

@hodyroff Note that 1370599 increased the limit to 100 and I've updated the docs accordingly in 096cd34

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ReadyToTest QA, please validate the fix/enhancement sev2-high type:bug
Projects
None yet
Development

No branches or pull requests

10 participants