Providing --path option to transfer-ownership#3965
Conversation
This will help user to selectively move the folders specified using --path option, instead of moving entire folder under files directory. Signed-off-by: Sujith H <sharidasan@owncloud.com> Update the integration test for transfer-ownership Update the integration test for transfer-ownership as the new option --path is introduced in the command. Signed-off-by: Sujith H <sharidasan@owncloud.com> Signed-off-by: Morris Jobke <hey@morrisjobke.de>
MorrisJobke
requested review from
LukasReschke,
blizzz,
icewind1991 and
nickvergessen
|
@MorrisJobke, thanks for your PR! By analyzing the history of the files in this pull request, we identified @nickvergessen, @rullzer and @LukasReschke to be potential reviewers. |
| Filesystem::initMountPoints($this->sourceUser); | ||
| Filesystem::initMountPoints($this->destinationUser); | ||
|
|
||
| if (strlen($this->inputPath) >= 1) { |
There was a problem hiding this comment.
if($this->inputPath) would be better, otherwise single character paths can lead to unexpected results
There was a problem hiding this comment.
I don't get this. It checks if the path is greater equal than.
| $self = $this; | ||
| $this->walkFiles($view, "$this->sourceUser/files", | ||
| $walkPath = "$this->sourceUser/files"; | ||
| if ( strlen($this->inputPath) > 0) { |
There was a problem hiding this comment.
just if($this->inputPath) would be cleaner
There was a problem hiding this comment.
but that fails directory '0'
| 'path', | ||
| null, | ||
| InputOption::VALUE_REQUIRED, | ||
| 'selectively provide the path to transfer. For example --path="folder_name"' |
There was a problem hiding this comment.
It seems to expect absolute path /$user/files/path, the cli help should reflect that
There was a problem hiding this comment.
Not really - see line 139.
| $this->walkFiles($view, "$this->sourceUser/files", | ||
| $walkPath = "$this->sourceUser/files"; | ||
| if ( strlen($this->inputPath) > 0) { | ||
| if ($this->inputPath !== "$this->sourceUser/files") { |
There was a problem hiding this comment.
This if only makes the code more complex imo
| $view->rename("$this->sourceUser/files", $this->finalTarget); | ||
| // because the files folder is moved away we need to recreate it | ||
| $view->mkdir("$this->sourceUser/files"); | ||
| $sourcePath = (strlen($this->inputPath) > 0) ? $this->inputPath : "$this->sourceUser/files"; |
There was a problem hiding this comment.
Simply setting $this->inputPath to a default value of $this->sourceUser/files would remove the need for endless if statements like this one
Signed-off-by: Morris Jobke <hey@morrisjobke.de>
MorrisJobke
force-pushed
the
downstream-27343
branch
from
0cbf3db to
fbf3bfa
Compare
|
@icewind1991 I addressed all the comments you had and refactored out most of the if statements. I tested it with and without the path option and it works fine 👍 |
5 participants
Tested and works fine here 👍