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

Update VisualStudio.gitignore #1131

Merged
merged 2 commits into from Jul 8, 2014

Conversation

Projects
None yet
4 participants
@OsirisTerje
Contributor

OsirisTerje commented Jul 3, 2014

The current pattern for NuGet doesn't work for sub-folders, also commented elsewhere here, and since by default Visual Studio creates the packages folder one level below the root, the current exclude here doesn't work in those cases.
Fixed pattern for excluding nuget packages so that it works both for top level package folder and for any lower level package folders. Re-include patterns fixed the same way.
I have written a blogpost here http://geekswithblogs.net/terje/archive/2014/07/03/gitignorendashhow-to-exclude-nuget-packages-at-any-level-and-make.aspx with all the details, also what effect the different patterns have.
I have also discussed this with the product group at MS, and got affirmative from them.

Update VisualStudio.gitignore
Fixed pattern for excluding nuget packages so that it works both for top level package folder and for any lower level package folders.  Re-include patterns fixed the same way.
@cammerman

This comment has been minimized.

Show comment
Hide comment
@cammerman

cammerman Jul 4, 2014

Glad someone more knowledgeable than me was able to contribute a solid fix! :)

cammerman commented Jul 4, 2014

Glad someone more knowledgeable than me was able to contribute a solid fix! :)

@arcresu

This comment has been minimized.

Show comment
Hide comment
@arcresu

arcresu Jul 4, 2014

Collaborator

Thanks for this. The reason we didn't have this before is that it creates an annoying conflict with the Umbraco template if they are used together (see #1013; /cc @bbodenmiller).

Also I'm not sure I agree with your assessment that we would need both **/packages/* and packages/*. Here's my test:

$ git --version
git version 2.0.1

$ cat .gitignore
**/packages/*
!**/packages/build/
!**/packages/repositories.config

$ cat tests
packages/pkg/IGNORE
packages/build/KEEP
packages/repositories.config
x/packages/pkg/IGNORE
x/packages/build/KEEP
x/packages/repositories.config

$ git check-ignore --verbose --non-matching --stdin < tests
.gitignore:1:**/packages/*  packages/pkg/IGNORE
::  packages/build/KEEP
.gitignore:3:!**/packages/repositories.config   packages/repositories.config
.gitignore:1:**/packages/*  x/packages/pkg/IGNORE
::  x/packages/build/KEEP
.gitignore:3:!**/packages/repositories.config   x/packages/repositories.config

It looks to me like all the test files are kept or ignored by the rules as desired without the duplicated top-level rules.

I think if we're making a change, then it should be replacing the existing rules, not adding new ones, and the Umbraco template needs to be updated to compensate.

Collaborator

arcresu commented Jul 4, 2014

Thanks for this. The reason we didn't have this before is that it creates an annoying conflict with the Umbraco template if they are used together (see #1013; /cc @bbodenmiller).

Also I'm not sure I agree with your assessment that we would need both **/packages/* and packages/*. Here's my test:

$ git --version
git version 2.0.1

$ cat .gitignore
**/packages/*
!**/packages/build/
!**/packages/repositories.config

$ cat tests
packages/pkg/IGNORE
packages/build/KEEP
packages/repositories.config
x/packages/pkg/IGNORE
x/packages/build/KEEP
x/packages/repositories.config

$ git check-ignore --verbose --non-matching --stdin < tests
.gitignore:1:**/packages/*  packages/pkg/IGNORE
::  packages/build/KEEP
.gitignore:3:!**/packages/repositories.config   packages/repositories.config
.gitignore:1:**/packages/*  x/packages/pkg/IGNORE
::  x/packages/build/KEEP
.gitignore:3:!**/packages/repositories.config   x/packages/repositories.config

It looks to me like all the test files are kept or ignored by the rules as desired without the duplicated top-level rules.

I think if we're making a change, then it should be replacing the existing rules, not adding new ones, and the Umbraco template needs to be updated to compensate.

@OsirisTerje

This comment has been minimized.

Show comment
Hide comment
@OsirisTerje

OsirisTerje Jul 4, 2014

Contributor

That is interesting. I see you use the 2.0.1 version, and I used the 1.9.2 version. If I repeat your tests I get this output

.gitignore:1:*/packages/ "packages/pkg/IGNORE\r"
:: "packages/build/KEEP\r"

.gitignore:1:*/packages/ "packages/repositories.config\r"

.gitignore:1:*/packages/ "x/packages/pkg/IGNORE\r"
:: "x/packages/build/KEEP\r"

.gitignore:1:*/packages/ "x/packages/repositories.config\r"

I was a bit curious why the first pattern didn't cover the top level, and it seems that might be a bug in 1.9.2 that has now been fixed in or before 2.0.1.

Since the use of the top level is less used, and not a default, I guess it would be best to keep only the */packages/ filter and skip the duplicates.

Do you want me to make that change ?

About the Umbraco issue : I think it is important the the Visual Studio pattern is correct alone, and that the Umbraco issue is dealt with separately. Right now the pattern doesn't work for the default VS case, and that is sort of not good.
Could this change go in and the Umbraco template be done afterwards, or should they be done simultaneously ?

Contributor

OsirisTerje commented Jul 4, 2014

That is interesting. I see you use the 2.0.1 version, and I used the 1.9.2 version. If I repeat your tests I get this output

.gitignore:1:*/packages/ "packages/pkg/IGNORE\r"
:: "packages/build/KEEP\r"

.gitignore:1:*/packages/ "packages/repositories.config\r"

.gitignore:1:*/packages/ "x/packages/pkg/IGNORE\r"
:: "x/packages/build/KEEP\r"

.gitignore:1:*/packages/ "x/packages/repositories.config\r"

I was a bit curious why the first pattern didn't cover the top level, and it seems that might be a bug in 1.9.2 that has now been fixed in or before 2.0.1.

Since the use of the top level is less used, and not a default, I guess it would be best to keep only the */packages/ filter and skip the duplicates.

Do you want me to make that change ?

About the Umbraco issue : I think it is important the the Visual Studio pattern is correct alone, and that the Umbraco issue is dealt with separately. Right now the pattern doesn't work for the default VS case, and that is sort of not good.
Could this change go in and the Umbraco template be done afterwards, or should they be done simultaneously ?

@bbodenmiller

This comment has been minimized.

Show comment
Hide comment
@bbodenmiller

bbodenmiller Jul 5, 2014

Contributor

I agree this issue needs to be fixed but I think this and a PR for the Umbraco issues should be merged concurrently. Other than the solution I proposed in #1013 I'm not sure of a better way to address the Umbraco issues.

Contributor

bbodenmiller commented Jul 5, 2014

I agree this issue needs to be fixed but I think this and a PR for the Umbraco issues should be merged concurrently. Other than the solution I proposed in #1013 I'm not sure of a better way to address the Umbraco issues.

@OsirisTerje

This comment has been minimized.

Show comment
Hide comment
@OsirisTerje

OsirisTerje Jul 5, 2014

Contributor

Ok, I can add a commit here with the removal of the separate toplevel lines for the VS gitignore. If you (@bbodenmiller) add a corresponding new PR for Umbraco, @arcresu should be able to merge both at the same time.

Contributor

OsirisTerje commented Jul 5, 2014

Ok, I can add a commit here with the removal of the separate toplevel lines for the VS gitignore. If you (@bbodenmiller) add a corresponding new PR for Umbraco, @arcresu should be able to merge both at the same time.

Update VisualStudio.gitignore
Removed the separate top-level patterns for NuGet in accordance with comments in PR. Will now match version 2.0.1.
@OsirisTerje

This comment has been minimized.

Show comment
Hide comment
@OsirisTerje

OsirisTerje Jul 5, 2014

Contributor

Added a commit with the top-level removals.

Contributor

OsirisTerje commented Jul 5, 2014

Added a commit with the top-level removals.

@bbodenmiller

This comment has been minimized.

Show comment
Hide comment
@bbodenmiller

bbodenmiller Jul 6, 2014

Contributor

I'm a bit confused about your comment with repositories.config. Do all version of Visual Studio regenerate repositories.config? Also the if the tool you use piece seems a bit out of place since this gitignore is targeted at one tool.

@arcresu are you good with reverting 5b168d4 or do you want to solve the Umbraco issues a different way?

Contributor

bbodenmiller commented Jul 6, 2014

I'm a bit confused about your comment with repositories.config. Do all version of Visual Studio regenerate repositories.config? Also the if the tool you use piece seems a bit out of place since this gitignore is targeted at one tool.

@arcresu are you good with reverting 5b168d4 or do you want to solve the Umbraco issues a different way?

@arcresu

This comment has been minimized.

Show comment
Hide comment
@arcresu

arcresu Jul 6, 2014

Collaborator

I'm not really in a position to know how useful the comment is, but if (either of) you have better wording for the comment, we can take care of that here too.

@bbodenmiller yep, I'll revert that commit at the same time as I merge this (no need to open another PR). It's not ideal but it seems like the only reasonable way to do it.

Collaborator

arcresu commented Jul 6, 2014

I'm not really in a position to know how useful the comment is, but if (either of) you have better wording for the comment, we can take care of that here too.

@bbodenmiller yep, I'll revert that commit at the same time as I merge this (no need to open another PR). It's not ideal but it seems like the only reasonable way to do it.

@OsirisTerje

This comment has been minimized.

Show comment
Hide comment
@OsirisTerje

OsirisTerje Jul 6, 2014

Contributor

@bbodenmiller : It is the NuGet process that regenerate the repositories.config file. It only contains pointers to the packages config files in an solution, - it is an optimization, see David Ebbo's answer here: http://stackoverflow.com/questions/7286261/nuget-repositories-config/7297465#7297465 . If you delete it in your project it will be regenerated when you somehow touch nuget (NuGet Solution Manager f.e.) I find it ok to keep it in the file, but commented out as a default. Then if people find they need it, they can enable it easily. I am not sure if there is any version of Nuget that doesn't restore this. Agree that the comment referring to the "the tool you use" might seem a bit odd, VS with Nuget will restore it when you open the Manager or open the Nuget powershell command line, but you might use different CI build tools, TFS will not require it, but I have read comments that at least earlier versions of TeamCity did require that file.
Can the comment be just "If you require repositores.config, uncomment the following line" ?

Contributor

OsirisTerje commented Jul 6, 2014

@bbodenmiller : It is the NuGet process that regenerate the repositories.config file. It only contains pointers to the packages config files in an solution, - it is an optimization, see David Ebbo's answer here: http://stackoverflow.com/questions/7286261/nuget-repositories-config/7297465#7297465 . If you delete it in your project it will be regenerated when you somehow touch nuget (NuGet Solution Manager f.e.) I find it ok to keep it in the file, but commented out as a default. Then if people find they need it, they can enable it easily. I am not sure if there is any version of Nuget that doesn't restore this. Agree that the comment referring to the "the tool you use" might seem a bit odd, VS with Nuget will restore it when you open the Manager or open the Nuget powershell command line, but you might use different CI build tools, TFS will not require it, but I have read comments that at least earlier versions of TeamCity did require that file.
Can the comment be just "If you require repositores.config, uncomment the following line" ?

@bbodenmiller

This comment has been minimized.

Show comment
Hide comment
@bbodenmiller

bbodenmiller Jul 7, 2014

Contributor

I'm fine with the comment either way. It would be beneficial if the comment was updated to explain why you might need repositores.config though and/or why you don't really need it.

Contributor

bbodenmiller commented Jul 7, 2014

I'm fine with the comment either way. It would be beneficial if the comment was updated to explain why you might need repositores.config though and/or why you don't really need it.

@arcresu arcresu merged commit bf70315 into github:master Jul 8, 2014

arcresu added a commit that referenced this pull request Jul 8, 2014

Revert "Remove redundant Umbraco exceptions to VS rules"
This reverts commit 5b168d4.

Amends #1131 - fixes Umbraco template to compensate for the change in the
VisualStudio template which would lead to issues if the two were used together.
These rules were originally added by @bbodenmiller in #1013
@arcresu

This comment has been minimized.

Show comment
Hide comment
@arcresu

arcresu Jul 8, 2014

Collaborator

Okay, so I merged the PR, reverted 5b168d4, and had a go at fixing up the comment (see the commits referenced above).

It seems like the repositories.config file is only needed if using an older version of Package Restore which is no longer recommended but is still supported. I think that's what the comment was originally getting at, but I've made it explicit. I also cut back the verbiage since I prefer to keep the comments to the point to make the files easier to read.

Thanks to both of you for this

Collaborator

arcresu commented Jul 8, 2014

Okay, so I merged the PR, reverted 5b168d4, and had a go at fixing up the comment (see the commits referenced above).

It seems like the repositories.config file is only needed if using an older version of Package Restore which is no longer recommended but is still supported. I think that's what the comment was originally getting at, but I've made it explicit. I also cut back the verbiage since I prefer to keep the comments to the point to make the files easier to read.

Thanks to both of you for this

bbodenmiller added a commit to bbodenmiller/NuGetDocs that referenced this pull request Jul 10, 2014

Clarify when repositories.config must be in DVCS
Clarify when repositories.config must be included in source control. Based on experience, https://stackoverflow.com/questions/7286261/nuget-repositories-config, & github/gitignore#1131.

Fixes #175, #13, NuGet#92 (comment).
@bbodenmiller

This comment has been minimized.

Show comment
Hide comment
@bbodenmiller

bbodenmiller Jul 10, 2014

Contributor

@arcresu looks good other than the issue I addressed in #1136. Additionally I think https://github.com/github/gitignore/blob/master/VisualStudio.gitignore#L113-L120 probably also need **/ prepended to them.

Contributor

bbodenmiller commented Jul 10, 2014

@arcresu looks good other than the issue I addressed in #1136. Additionally I think https://github.com/github/gitignore/blob/master/VisualStudio.gitignore#L113-L120 probably also need **/ prepended to them.

linh-kaiserhl pushed a commit to kaiserhl/gitignore that referenced this pull request Jul 8, 2017

Revert "Remove redundant Umbraco exceptions to VS rules"
This reverts commit 5b168d4.

Amends #1131 - fixes Umbraco template to compensate for the change in the
VisualStudio template which would lead to issues if the two were used together.
These rules were originally added by @bbodenmiller in #1013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment