[Ignore] Fix recursion count bug #802
Conversation
rjmholt
requested review from
TylerLeonhardt,
rkeithhill and
SeeminglyScience
|
In Diff Settings, select Hide whitespace changes to make this easier to review |
| @@ -292,7 +292,9 @@ public IEnumerable<string> EnumeratePSFiles() | |||
| return Enumerable.Empty<string>(); | |||
| } | |||
|
|
|||
| return this.RecursivelyEnumerateFiles(WorkspacePath); | |||
| var foundFiles = new List<string>(); | |||
| this.RecursivelyEnumerateFiles(WorkspacePath, ref foundFiles); | |||
There was a problem hiding this comment.
ref isn't needed here, but I used it as a guide to the reader that this is effectively the result of the method.
| // If we get too deep, keep processing but go no deeper | ||
| if (currDepth >= recursionDepthLimit) | ||
| { | ||
| this.logger.Write(LogLevel.Warning, $"Recursion depth limit hit for path {folderPath}"); |
There was a problem hiding this comment.
This message will be more accurate now
|
|
||
| foreach (string subDir in subDirs) | ||
| { | ||
| RecursivelyEnumerateFiles(subDir, ref foundFiles, currDepth + 1); |
There was a problem hiding this comment.
Thoughts on currDepth: currDepth + 1 in the last arg?
|
|
||
| return; | ||
| } | ||
| catch (Exception e) when (e is SecurityException || e is UnauthorizedAccessException) |
There was a problem hiding this comment.
I don't have a better suggestion and think this is the right way to do it... but damn these catch statements take up a lot of space.
There was a problem hiding this comment.
If they start getting too wide I tend to wrap them like so:
catch (Exception e) when (e is SecurityException ||
e is UnauthorizedAccessException)
But I don't think this one is too wide.
There was a problem hiding this comment.
yeah I don't think it's too wide - I meant the 3 catches right after each other twice in this function.
There was a problem hiding this comment.
I don't like the repetition, but I think a function call around a lambda just to catch certain exceptions is a rather nasty abuse.
There was a problem hiding this comment.
(Would be more abstractable in a world where exceptions were returned as union types...)
@rkeithhill Saw that I'd implemented the recursion depth checking incorrectly (I was checking the stack size, which included all the files in parent directories).
I've rewritten this in the simpler recursive function form, which should actually be more efficient (in some ways), although there are pros and cons.
The other thing I noticed tracing this is that we create files for everything in the
.gitdirectory. We should really implement a file-ignore config and make it default to all files beginning with.or something.