Allow cloning of repositories into empty directories

[YuPeiHenry]

Ready for review
fixes#2250

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

[jcansdale]

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. ;-)

[jcansdale]

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

Thanks for the PR!

[StanleyGoldman]

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.

[StanleyGoldman]

[StanleyGoldman]

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]

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

[meaghanlewis]

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]

@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]

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]

thank you @YuPeiHenry for your contribution! 🎉