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

Do not allow moving a folder into itself #1173

Merged
merged 11 commits into from Jul 11, 2018

Conversation

Projects
None yet
5 participants
@50Wliu
Member

50Wliu commented Sep 4, 2017

Description of the Change

Do not allow any recursive copying, such as when attempting to copy test/a into test/a/b. Before this PR, such actions would be allowed and would result in a a/b/a/b/a/b/a/b/... infinite directory structure. Drag-and-dropping previously emitted a non-obvious warning (cannot rename a to a/b). Both now emit a "Cannot paste/move a folder into itself" warning and return without performing any additional actions.

As a side effect of this change, Tree View no longer allows copying a directory directly inside itself, such as from a/ to a/ (resulting in a/a/). This is consistent with behavior on Windows.

Alternate Designs

Carefully copy the folder structure once instead of recursively. This is much harder to implement and is also not what major OSes do.

Benefits

This fixes a usability issue where you would be unintentionally left with a massive folder structure (unable to be deleted on Windows through conventional means) that freezes Atom.

Possible Drawbacks

The only negative impact I can think of is pasting folders directly inside themselves no longer works.

Applicable Issues

Fixes #509
Refs #656, but I do not believe that this PR fixes that issue
Supersedes and closes #952

/cc @Ben3eeE @ungb: I'm interested in seeing whether or not pasting folders directly inside themselves works on macOS and Linux. Also, I haven't tested this with symlinks.

@ungb

This comment has been minimized.

Show comment
Hide comment
@ungb

ungb Sep 8, 2017

Contributor

Tested it on mac without symlinks and things look good.

With symlink does you get a weird behavior where it attempts to do it but fails after the first folder. none of the contents in that folder are copied either.

Attempt to copy a into syma\c
image

you get the following:

Creates the folder A but nothing else under it.
image

Moving:
image

Contributor

ungb commented Sep 8, 2017

Tested it on mac without symlinks and things look good.

With symlink does you get a weird behavior where it attempts to do it but fails after the first folder. none of the contents in that folder are copied either.

Attempt to copy a into syma\c
image

you get the following:

Creates the folder A but nothing else under it.
image

Moving:
image

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Sep 8, 2017

Member

Found a regression: You can't copy a into aa, even if they're on the same level as each other. Probably due to my startsWith matching.

Member

50Wliu commented Sep 8, 2017

Found a regression: You can't copy a into aa, even if they're on the same level as each other. Probably due to my startsWith matching.

@ungb

This comment has been minimized.

Show comment
Hide comment
@ungb

ungb Sep 8, 2017

Contributor

good find @50Wliu! on mac, if you try pasting into a symlink you get the same thing as if you try to paste a parent into a child or inside itself

image

Contributor

ungb commented Sep 8, 2017

good find @50Wliu! on mac, if you try pasting into a symlink you get the same thing as if you try to paste a parent into a child or inside itself

image

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Sep 8, 2017

Member

Ok, good to know. That's different than the behavior that Windows exhibits (which is not surprising, considering its symlink support). I think I will defer to the macOS implementation for symlinks.

Member

50Wliu commented Sep 8, 2017

Ok, good to know. That's different than the behavior that Windows exhibits (which is not surprising, considering its symlink support). I think I will defer to the macOS implementation for symlinks.

@BinaryMuse BinaryMuse self-assigned this Jan 5, 2018

@jasonrudolph

This comment has been minimized.

Show comment
Hide comment
@jasonrudolph

jasonrudolph Jan 16, 2018

Member

@50Wliu: I can take a look at this pull request this week and try to get it merged. Would you be willing to summarize the verification process that you've used to verify that these changes have the desired effects?

Member

jasonrudolph commented Jan 16, 2018

@50Wliu: I can take a look at this pull request this week and try to get it merged. Would you be willing to summarize the verification process that you've used to verify that these changes have the desired effects?

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Jan 26, 2018

Member

@jasonrudolph Sorry for the delay! Here's the test plan I came up with.
Create a test folder with the following subfolders: a/, a/b/, aa/

  • Attempt to copy test/a/ into test/a/b/ using the tree-view:copy and tree-view:paste commands.
    • Notification should appear and the folder structure should remain unchanged.
  • Attempt to move test/a/ into test/a/b/ using drag-and-drop.
    • Notification should appear and the folder structure should remain unchanged.
  • Attempt to copy test/a/ into test/aa/.
    • Operation should complete successfully and both test/a/ as well as test/aa/a/ should exist.
  • Attempt to move test/a/ into test/aa/ using drag-and-drop.
    • Operation should complete successfully and only test/aa/a/ should exist.

Now, create a symlink syma that points to the a directory.

  • Attempt to copy test/a/ into /test/syma/b/.
    • Notification should appear and there should not be a a folder under b.
  • Attempt to move test/a/ into /test/syma/b/.
    • Notification should appear and there should not be a a folder under b.

To the best of my recollection, that's how I tested this PR when I first created it. If you'd like to see some other tests, please let me know.

Member

50Wliu commented Jan 26, 2018

@jasonrudolph Sorry for the delay! Here's the test plan I came up with.
Create a test folder with the following subfolders: a/, a/b/, aa/

  • Attempt to copy test/a/ into test/a/b/ using the tree-view:copy and tree-view:paste commands.
    • Notification should appear and the folder structure should remain unchanged.
  • Attempt to move test/a/ into test/a/b/ using drag-and-drop.
    • Notification should appear and the folder structure should remain unchanged.
  • Attempt to copy test/a/ into test/aa/.
    • Operation should complete successfully and both test/a/ as well as test/aa/a/ should exist.
  • Attempt to move test/a/ into test/aa/ using drag-and-drop.
    • Operation should complete successfully and only test/aa/a/ should exist.

Now, create a symlink syma that points to the a directory.

  • Attempt to copy test/a/ into /test/syma/b/.
    • Notification should appear and there should not be a a folder under b.
  • Attempt to move test/a/ into /test/syma/b/.
    • Notification should appear and there should not be a a folder under b.

To the best of my recollection, that's how I tested this PR when I first created it. If you'd like to see some other tests, please let me know.

@jasonrudolph

This comment has been minimized.

Show comment
Hide comment
@jasonrudolph

jasonrudolph Feb 8, 2018

Member

@50Wliu: When you have a moment, would you mind merging master into this branch and resolving the merge conflicts?

Member

jasonrudolph commented Feb 8, 2018

@50Wliu: When you have a moment, would you mind merging master into this branch and resolving the merge conflicts?

@jasonrudolph

This comment has been minimized.

Show comment
Hide comment
@jasonrudolph

jasonrudolph Feb 8, 2018

Member

Here's the test plan I came up with. ... To the best of my recollection, that's how I tested this PR when I first created it. If you'd like to see some other tests, please let me know.

Thanks, @50Wliu. Those tests all seems useful, and I'd like to recommend a few additional ones:

  • Attempt to move test/a/ into test/a/b/ using the tree-view:cut and tree-view:paste commands.

    • Notification should appear and the folder structure should remain unchanged.
  • Attempt to copy move test/syma/ into /test/a.

    • Operation should complete successfully and only test/a/syma should exist.

Do those tests seem reasonable? If so, I recommend the following next steps:

  • Merge master into this branch and resolve the merge conflicts (#1173 (comment))
  • Run through the test plan on Windows
  • Run through the test plan on either Linux or macOS
  • If the test plan reveals that everything is functioning correctly, merge this pull request 👍

Edit by @rsese: copy ➡️ move for the second test per #1173 (comment)

Member

jasonrudolph commented Feb 8, 2018

Here's the test plan I came up with. ... To the best of my recollection, that's how I tested this PR when I first created it. If you'd like to see some other tests, please let me know.

Thanks, @50Wliu. Those tests all seems useful, and I'd like to recommend a few additional ones:

  • Attempt to move test/a/ into test/a/b/ using the tree-view:cut and tree-view:paste commands.

    • Notification should appear and the folder structure should remain unchanged.
  • Attempt to copy move test/syma/ into /test/a.

    • Operation should complete successfully and only test/a/syma should exist.

Do those tests seem reasonable? If so, I recommend the following next steps:

  • Merge master into this branch and resolve the merge conflicts (#1173 (comment))
  • Run through the test plan on Windows
  • Run through the test plan on either Linux or macOS
  • If the test plan reveals that everything is functioning correctly, merge this pull request 👍

Edit by @rsese: copy ➡️ move for the second test per #1173 (comment)

50Wliu added some commits Feb 9, 2018

Merge branch 'master' into wl-no-recursive-copying
Some specs are still failing.
@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Feb 23, 2018

Member

Assuming for the last bullet point you meant "attempt to move", then the test plan passes on Windows.

Member

50Wliu commented Feb 23, 2018

Assuming for the last bullet point you meant "attempt to move", then the test plan passes on Windows.

@rsese

This comment has been minimized.

Show comment
Hide comment
@rsese

rsese Jul 9, 2018

Member

Went through the verification process on macOS 10.12.6 and Ubuntu 16.04 - only 1 of the tests didn't have the expected result:

  • Attempt to move test/syma/ into /test/a.
    • Operation should complete successfully and only test/a/syma should exist.

On both macOS and Ubuntu, the operation completes without an error notification or anything in devtools console, but test/a/syma doesn't exist and test/syma no longer exists either.

GIF-age on macOS (behavior is the same on Ubuntu):

test-tree-view-1173

All the other tests had the expected results but let me know if I misunderstood anything with this last test @50Wliu.

Member

rsese commented Jul 9, 2018

Went through the verification process on macOS 10.12.6 and Ubuntu 16.04 - only 1 of the tests didn't have the expected result:

  • Attempt to move test/syma/ into /test/a.
    • Operation should complete successfully and only test/a/syma should exist.

On both macOS and Ubuntu, the operation completes without an error notification or anything in devtools console, but test/a/syma doesn't exist and test/syma no longer exists either.

GIF-age on macOS (behavior is the same on Ubuntu):

test-tree-view-1173

All the other tests had the expected results but let me know if I misunderstood anything with this last test @50Wliu.

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Jul 11, 2018

Member

@rsese instead of using a relative path for the symlink, can you try specifying the absolute path to a?

Member

50Wliu commented Jul 11, 2018

@rsese instead of using a relative path for the symlink, can you try specifying the absolute path to a?

@rsese

This comment has been minimized.

Show comment
Hide comment
@rsese

rsese Jul 11, 2018

Member

Just gave that a try on macOS and Ubuntu and on both, syma shows up in the tree-view after using an absolute path to a/:

test-tree-view-1173-2

Member

rsese commented Jul 11, 2018

Just gave that a try on macOS and Ubuntu and on both, syma shows up in the tree-view after using an absolute path to a/:

test-tree-view-1173-2

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Jul 11, 2018

Member

I've talked to rsese and the reason the symlink disappeared when it was a relative path is because of #562. If we ignore #562, then the test plan passes (and indeed, it does when an absolute path is used).

Member

50Wliu commented Jul 11, 2018

I've talked to rsese and the reason the symlink disappeared when it was a relative path is because of #562. If we ignore #562, then the test plan passes (and indeed, it does when an absolute path is used).

@50Wliu 50Wliu merged commit 90f1fc0 into master Jul 11, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@50Wliu 50Wliu deleted the wl-no-recursive-copying branch Jul 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment