8000 Allow cloning of repositories into empty directories by YuPeiHenry · Pull Request #2316 · github/VisualStudio · GitHub
[go: up one dir, main page]
Skip to content
This repository was archived by the owner on Jun 21, 2023. It is now read-only.

Allow cloning of repositories into empty directories #2316

Merged
merged 15 commits into from
Apr 8, 2019
Merged

Allow cloning of repositories into empty directories #2316

merged 15 commits into from
Apr 8, 2019

Conversation

YuPeiHenry
Copy link
Contributor
@YuPeiHenry YuPeiHenry commented Apr 4, 2019 •
edited
Loading

Ready for review
fixes #2250

The fix modifies CanClone and CanOpen on RepositoryCloneViewModel, and modified RepositoryCloneService CloneOrOpenRepository method to factor for empty directories.

@YuPeiHenry
Enable clone and disable open button on empty dir.
7c998ec 
- Currently clone button is disabled and open button is enabled
on empty directories.
- Desired behavior should support cloning to empty directories.
- Non-empty directory message is not shown when the directory is empty.
@YuPeiHenry
Add support for cloning into empty dir.
b7cd425
jcansdale
jcansdale suggested changes Apr 4, 2019
View reviewed changes
Copy link
Collaborator
@jcansdale jcansdale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking good!

Could you add some unit tests? Something similar to this...
https://github.com/github/VisualStudio/blob/master/test/GitHub.App.UnitTests/ViewModels/Dialog/Clone/RepositoryCloneViewModelTests.cs#L317

Maybe add the following tests:

Clone_Is_Enabled_When_Path_EmptyDirectoryExists
PathWarning_Is_Not_Set_When_EmptyDirectoryExists_Selected

I've just realized this is a draft PR. Sorry if I've jumped the gun reviewing it. ;-)

@YuPeiHenry YuPeiHenry marked this pull request as ready for review April 4, 2019 14:39
@YuPeiHenry YuPeiHenry force-pushed the 2250-allow-cloning-to-empty-dir branch from 80198b4 to 43b5ef6 Compare April 4, 2019 15:01
@meaghanlewis meaghanlewis requested a review from jcansdale April 4, 2019 20:05
jcansdale
jcansdale approved these changes Apr 5, 2019
View reviewed changes
Copy link
Collaborator
@jcansdale jcansdale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code for this looks good! 😄 We just need someone to confirm the functionality.

Thanks for the PR!

StanleyGoldman
StanleyGoldman suggested changes Apr 5, 2019
View reviewed changes
Copy link
Contributor
@StanleyGoldman StanleyGoldman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably need to make the error/warning messages clearer...

For instance when I'm trying to clone into a directory that is not empty and does not have a git repo
The message says "There is already a repository at this location, but it does not contain a repository".

That's not entirely accurate, I haven't looked but I think we have to re-evaluate the messages output by the view model in this and probably some other cases.

image

@StanleyGoldman
Copy link
Contributor

image

@StanleyGoldman
Copy link
Contributor

The more I look at it, it's just that one message that would no longer be accurate...
I don't even think it was right to begin with..

If the directory is not empty, and is not a git repo that could be scanned.
The error message probably should have been. "A directory exists at the destination path."
Similar to "A file exists at the destination path."

Based on the current logic in this Pull Request.
The new error message should be: "The directory at the destination path is not empty."
The chain of logic should be... (in pseudocode)

if file_exists_at_destination:
   return "A file exists at the destination path"

else:
   if directory_exists_at_destination AND directory_at_destination_not_empty:

      if directory_exists_at_destination_is_git_repo:
         if git_repo_has_no_origin:
            return "A repository already exists at this location, but it doesn't have a remote named `origin`."
         end if

         if git_repo_origin_is_different:
            return "A repository already exists at this location, but it doesn't have a remote named `origin`."
         end if

      else:
         return "The directory at the destination path is not empty."
      end if

   end if
end if

@StanleyGoldman
Copy link
Contributor

Let @meaghanlewis weigh in before you change anything...

@meaghanlewis
Copy link
Contributor

I agree with what @StanleyGoldman proposed: The directory at the destination path is not empty. Its straightforward and covers cases where there might be a git directory for the destination or not.

@YuPeiHenry
Copy link
Contributor Author

@StanleyGoldman I have updated the error message for non-empty directory. Should something be done about the translation to other languages in this PR?

@StanleyGoldman StanleyGoldman force-pushed the 2250-allow-cloning-to-empty-dir branch from a29268a to ef0ed57 Compare April 8, 2019 12:42
@StanleyGoldman
Copy link
Contributor
StanleyGoldman commented Apr 8, 2019 •
edited
Loading

Hey @YuPeiHenry I pushed some commits. It felt easier than nit picking at you. 😈

So I copied the English text to all the language files, we have a 3rd party resource that translates things for us. So we just put the English in place until they get around to it.

I also realized I reordered the logic funny in my pseudocode. You changed the flow to match my ramblings. I undid the damage I caused. Sorry to put you through my spaghetti brain.

And I tweaked DestinationDirectoryEmpty. Everywhere that the function is used, we explicitly check if the directory exists. It makes sense to check if the directory exists inside DestinationDirectoryEmpty to prevent an error, but it's also redundant to all the code that uses DestinationDirectoryEmpty. The only correct choice is to assume the function with be used properly.

All nit picks. I didn't wanna bug you with it. 😅

Since my hands have been all in this, I'm going to let @meaghanlewis test this and give us both the final approval.

@meaghanlewis
Copy link
Contributor

thank you @YuPeiHenry for your contribution! 🎉

@meaghanlewis meaghanlewis merged commit 285c8a5 into github:master Apr 8, 2019
@StanleyGoldman StanleyGoldman mentioned this pull request Apr 10, 2019
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@StanleyGoldman StanleyGoldman StanleyGoldman approved these changes

@meaghanlewis meaghanlewis meaghanlewis approved these changes

@jcansdale jcansdale jcansdale approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow cloning of repositories into empty directories
4 participants
@YuPeiHenry @StanleyGoldman @meaghanlewis @jcansdale
0