Skip to content
This repository was archived by the owner on Oct 4, 2021. It is now read-only.

[Version Control] Avoid creating .git if it already exists #7354

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jsuarezruiz
Copy link
Contributor

When creating a new project into a folder which already has already been setup as a git repo, avoid create the .git folder.

Fixes VSTS #660470

Copy link
Member

@mrward mrward left a comment

Choose a reason for hiding this comment

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

Just wondering if we could/should make this logic available to the New Project dialog so it could disable the Git options before it reaches this code.

Copy link
Member

@mrward mrward left a comment

Choose a reason for hiding this comment

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

Currently the New Project dialog enables/disables the git checkbox here -

finalConfigurationPage.IsUseGitEnabled = IsNewSolution && (versionControlHandler != null);

Either the logic could be moved into the New Project dialog or the IVersionControlProjectTemplateHandler could be extended with a new method which the New Project dialog could call to see if git can be enabled.

I guess one problem here is that the logic is dependent on where the solution is being created. So after the location is changed the new project dialog would need to check to see if the git checkbox should be enabled/disabled. Currently this check is just done the one time.

Copy link
Member

@sevoku sevoku left a comment

Choose a reason for hiding this comment

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

I like this idea, lets's add a IVersionControlProjectTemplateHandler.CanCreateRepository (FilePath atPath). FinalProjectConfigurationPage could have an additional ParentFolderChanged event and the NewProjectController would subscribe to that and set IsUseGitEnabled based on the result of CanCreateRepository.

@jsuarezruiz
Copy link
Contributor Author

Matt, I liked your idea. Thanks guys for the feedback!. I have made changes to disable the Git option if a .git folder already exists (in the selected path).
However, I have some doubts. Should we show a warning icon with a popup and information in this case? Thus, in case of doubt the user knows why it is disabled. Another one, should we check for a .gitignore file and if exists disable the option to create the gitignore file?

Copy link
Member

@sevoku sevoku left a comment

Choose a reason for hiding this comment

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

I'm not sure about the warning, I think VS on Windows doesn't tell anything or does it?

About Gitignore: I'd say let's keep it as is. Because a gitignore can live inside a subfolder. Of course we could check if it exists or not, but this would require additional interface changes, which is already not very clean (the API pretends to be generic for all VCS systems, but the UI refers to git only, and Gitignore is git specific, hence we would really need to rewrite this whole thing).
I'd propose to do the check in ProjectTemplateHandler.cs where it actually creates that file: if it exists, just skip it, if not create it in the NEW PROJECT/SOLUTION folder (not repo root).

@jsuarezruiz jsuarezruiz force-pushed the fix-660470 branch 2 times, most recently from 91fa09a to 44dc1f8 Compare May 10, 2019 16:13
@jsuarezruiz jsuarezruiz force-pushed the fix-660470 branch 2 times, most recently from 497d524 to 77795d8 Compare May 27, 2019 06:16
@@ -179,7 +182,7 @@ public string GetValidSolutionName ()
}

public bool IsGitIgnoreEnabled {
get { return config.UseGit && IsUseGitEnabled && gitIgnoreEnabled; }
get { return config.UseGit && gitIgnoreEnabled && EnableCreateGitIgnore(); }
}
Copy link
Member

Choose a reason for hiding this comment

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

Can we add tests for this new logic?

There are some tests for the IsGitIgnoreEnabled flag in the NewProjectDialogTests.

Also should the IsUseGitEnabled check be removed here? That is used

Assert.IsFalse (controller.FinalConfiguration.IsGitIgnoreEnabled);

@jsuarezruiz
Copy link
Contributor Author

The behavior with .gitignore with this changes:

  • .gitginore exists in project folder (not parent, given the selected target folder already exists) > disable and uncheck checkbox.
  • .gitignore exists in a parent folder > enable and uncheck checkbox.
  • .gitignore does not exist (neither target nor parent folder) > enable and check the checkbox.

I have reviewed the tests too:
Screenshot 2019-06-10 at 12 42 57

Copy link
Contributor

@mkrueger mkrueger left a comment

Choose a reason for hiding this comment

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

Just the EventArgs thing :).

@jsuarezruiz
Copy link
Contributor Author

Hey @mkrueger, thanks for the feedback but, what do you mean by the EventArgs?

@sevoku
Copy link
Member

sevoku commented Jul 2, 2019

@jsuarezruiz I think @mkrueger meant to use EventArgs.Empty instead of creating a new instance

@jsuarezruiz
Copy link
Contributor Author

Ah, ok!. Changed. Thanks guys.

@Therzok Therzok added this to the 8.3 milestone Aug 21, 2019
Base automatically changed from master to main March 9, 2021 14:17
@akoeplinger akoeplinger changed the base branch from main to master March 15, 2021 17:02
Base automatically changed from master to main March 15, 2021 17:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants