fix: maxdepth doesn't limit total number of copies#549
Conversation
Prevent maxdepth from limiting the total number of copies, only allow it to limit the maximum depth Fixes #547
|
Apparently the maxdepth feature added in #232 wasn't implemented correctly. It was compared against the total number of calls to the function, not to the depth of the function call stack. We incremented the value, but we forgot to decrement it. Because objects in JS are pass-by-reference, all invocations of the function reference the same depth counter. |
|
@freitagbr will you have time to review in the next couple days? If not, I can merge myself. |
|
Yes, I will review this soon. |
|
@freitagbr changes are finished |
| opts.depth++; | ||
| // Ensure there is not a run away recursive copy | ||
| if (currentDepth >= common.config.maxdepth) return; | ||
| currentDepth++; |
There was a problem hiding this comment.
Well, ok. I was thinking that depth would be the last function parameter, so that you wouldn't have to explicitly call cpdirSyncRecursive with it. To initialize the depth:
if (!depth) depth = 0;
just like we initialize opts. Then you can depth++ or call cpdirSyncRecursive(..., depth + 1), whichever you think is clearer.
There was a problem hiding this comment.
As for the order, I usually leave options as the final parameter. cpdirSyncRecursive is totally internal, so requiring the depth parameter isn't so awful.
If we allow depth and opts to both be optional, then we have to account for the case when depth is missing but opts isn't, and vice-versa. Sounds very bug-prone to me, for very little external benefit.
There was a problem hiding this comment.
Ok, I can see that. As long as it's internal, it shouldn't be a problem.
There was a problem hiding this comment.
Ok, just give it an LGTM when ready to merge
|
LGTM |
|
LGTM (forgot that you don't have lgtm permission yet) |
Prevent maxdepth from limiting the total number of copies, only allow it to
limit the maximum depth
Fixes #547