Skip to content
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

don't ignore mir_dump folder #63307

Merged
merged 1 commit into from
Aug 7, 2019
Merged

don't ignore mir_dump folder #63307

merged 1 commit into from
Aug 7, 2019

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Aug 5, 2019

I dumped some MIR and wondered why git status wouldn't show the tree as dirty, reminding me to clean up after myself. Turns out this folder was explicitly gitignored. I don't think it should be.

If someone doesn't want to clean up that way, they can add it to .git/info/exclude.

(That file seems like it could need some general cleanup, honestly, but that's for another day.)

@rust-highfive
Copy link
Collaborator

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 5, 2019
@varkor
Copy link
Member

varkor commented Aug 5, 2019

This was deliberately ignored in #52615. If there's disagreement, an explicit decision should be made, rather than going back and forth with what's included. The files in mir_dump are never going to be committed, so I think it makes sense to .gitignore them.

@RalfJung
Copy link
Member Author

RalfJung commented Aug 6, 2019

The files in mir_dump constitute junk that should be cleaned up. gitignore is only for files that are expected to be created, like stuff created by a normal build. Nothing else should be ignored.

If someone's workflow involves leaving junk in the tree, that's fine, that's exactly what .git/info/exclude is for. Please don't force such a workflow on everyone.

Also see #53768, where some people agreed with that reasoning.

@davidtwco
Copy link
Member

I don’t feel strongly about this directory being removed from the .gitconfig. If we do decide to remove the directory, we should also add a comment explaining what warrants addition to .gitignore - files that are expected to be created by the compilation process, not junk files that commonly show up - so that anyone who wishes to add to .gitignore can see that, and so can reviewers of future PRs that modify .gitignore.

@varkor
Copy link
Member

varkor commented Aug 6, 2019

If it's not supported already, it would be nice to have an x.py clean mode that removed these sorts of directories.

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Aug 6, 2019

📌 Commit 7c374cf has been approved by alexcrichton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 6, 2019
Centril added a commit to Centril/rust that referenced this pull request Aug 6, 2019
don't ignore mir_dump folder

I dumped some MIR and wondered why `git status` wouldn't show the tree as dirty, reminding me to clean up after myself. Turns out this folder was explicitly gitignored. I don't think it should be.

If someone doesn't want to clean up that way, they can add it to `.git/info/exclude`.

(That file seems like it could need some general cleanup, honestly, but that's for another day.)
bors added a commit that referenced this pull request Aug 7, 2019
Rollup of 9 pull requests

Successful merges:

 - #63034 (Fix generator size regressions due to optimization)
 - #63035 (Use MaybeUninit to produce more correct layouts)
 - #63163 (add a pair of whitespace after remove parentheses)
 - #63294 (tests for async/await drop order)
 - #63307 (don't ignore mir_dump folder)
 - #63308 (PlaceRef's base is already a reference)
 - #63310 (Tests around moving parts of structs and tuples across await points)
 - #63314 (doc: the content has since been moved to the guide)
 - #63333 (Remove unnecessary features from compiler error code list)

Failed merges:

r? @ghost
@bors bors merged commit 7c374cf into rust-lang:master Aug 7, 2019
@bors
Copy link
Contributor

bors commented Aug 7, 2019

☔ The latest upstream changes (presumably #63341) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 7, 2019
@RalfJung
Copy link
Member Author

RalfJung commented Aug 8, 2019

we should also add a comment

PR opened: #63373

Centril added a commit to Centril/rust that referenced this pull request Aug 8, 2019
gitignore: add comment explaining policy

Based on rust-lang#63307 (comment), I added a comment what I think should be gitignored and what not. This is just a proposal, obviously.  Also see rust-lang#53768 for some more discussion.

The summary is that if there are junk files that you create locally and are fine leaving around (such as `mir_dump`), git has the option for you to add them to `.git/info/exclude`. Others might prefer to keep their working dir clean of those same junk files, so we shouldn't just ignore them for everyone.

I then also cleaned up a few more things, but there were many things that I had no idea where they came from so I didn't touch them.
Centril added a commit to Centril/rust that referenced this pull request Aug 8, 2019
gitignore: add comment explaining policy

Based on rust-lang#63307 (comment), I added a comment what I think should be gitignored and what not. This is just a proposal, obviously.  Also see rust-lang#53768 for some more discussion.

The summary is that if there are junk files that you create locally and are fine leaving around (such as `mir_dump`), git has the option for you to add them to `.git/info/exclude`. Others might prefer to keep their working dir clean of those same junk files, so we shouldn't just ignore them for everyone.

I then also cleaned up a few more things, but there were many things that I had no idea where they came from so I didn't touch them.
Centril added a commit to Centril/rust that referenced this pull request Oct 21, 2019
…um,Centril

keep the root dir clean from debugging

We landed this before with rust-lang#63307 but recently in rust-lang#65630 the IMO bad ignore crept back in.

If you regularly do graphviz-based debugging and you are fine leaving junk in the rustc root dir, please configure your local `.git/info/exclude`. But most people working on rustc don't work with graphciz all that often (I for once never did), and not everyone likes to have stray generated files in their source dirs.

Also Cc rust-lang#63373 rust-lang#53768 @ecstatic-morse @Mark-Simulacrum
Centril added a commit to Centril/rust that referenced this pull request Oct 21, 2019
…um,Centril

keep the root dir clean from debugging

We landed this before with rust-lang#63307 but recently in rust-lang#65630 the IMO bad ignore crept back in.

If you regularly do graphviz-based debugging and you are fine leaving junk in the rustc root dir, please configure your local `.git/info/exclude`. But most people working on rustc don't work with graphciz all that often (I for once never did), and not everyone likes to have stray generated files in their source dirs.

Also Cc rust-lang#63373 rust-lang#53768 @ecstatic-morse @Mark-Simulacrum
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants