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.

Sign up for GitHub

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

Jump to bottom

align semantics of generated vcs ignore files #11855

Merged
merged 1 commit into from
Mar 15, 2023
Merged

align semantics of generated vcs ignore files #11855

merged 1 commit into from
Mar 15, 2023

Conversation

robjtede
Copy link
Contributor

@robjtede robjtede commented Mar 15, 2023 •
edited
Loading

What does this PR try to resolve?

The currently generated ^target/ spec in a hg ignore will only ignore dirs of that name at the root.

This change matches the behavior of the gitignore spec next to it, by only ignoring both files/symlinks and dirs of name "target".

How should we test and review this PR?

Run cargo new

@rustbot
Copy link
Collaborator

rustbot commented Mar 15, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added Command-new S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 15, 2023
@robjtede robjtede force-pushed the target-ignore branch 2 times, most recently from 2a54f88 to fdd35e4 Compare March 15, 2023 05:09
weihanglo
weihanglo reviewed Mar 15, 2023
View reviewed changes
Copy link
Member

@weihanglo weihanglo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the pull request.

If I understand correctly, it is an intentional change. If we have trailing slash / and at the same time target is a symlink to another directory, it won't be ignored by git (see #4944).

It seems to be a reasonable tradeoff doing so back to that day, as people sometimes symlink target directory in order to share build artifacts. (We have a canonical build.target-dir for helping that btw)

To resolve the inconsistency between git and mercurial (and maybe fossil as well), instead of this pull request, I would recommend

  • Add a test for ignoring target dir as a symlink
  • Update generated ^target in .hgignore to ^target$, making it match file name only (if it works).
  • Same applies to fossil rules if possible.

@weihanglo
Copy link
Member

r? @weihanglo

@rustbot rustbot assigned weihanglo and unassigned ehuss Mar 15, 2023
@epage
Copy link
Contributor

epage commented Mar 15, 2023

targo is an example of a current tool that takes the symlink approach: #11156 (comment)

@robjtede robjtede force-pushed the target-ignore branch 3 times, most recently from bf37bb5 to 63f6d82 Compare March 15, 2023 14:39
@robjtede robjtede changed the title chore: fix target dir ignore in generated gitignore align semantics of generated vcs ignore files Mar 15, 2023
weihanglo
weihanglo reviewed Mar 15, 2023
View reviewed changes
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@robjtede, could you add a simple test around it as well?

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 15, 2023
@robjtede
Copy link
Contributor Author

Added. If this isn't what you meant, I don't know what test you want to see.

@weihanglo
Copy link
Member

Oops my fault 🙇🏾. You already updated UI snapshot. Maybe we don't need the newly added test. Could you drop that commit and then we can merge this? Thank you!

@weihanglo
Copy link
Member

Thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Mar 15, 2023

📌 Commit 8183c31 has been approved by weihanglo

It is now in the queue for this repository.

@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-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. labels Mar 15, 2023
@bors
Copy link
Contributor

bors commented Mar 15, 2023

⌛ Testing commit 8183c31 with merge 94394eb...

@bors
Copy link
Contributor

bors commented Mar 15, 2023

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 94394eb to master...

1 similar comment
@bors
Copy link
Contributor

bors commented Mar 15, 2023

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 94394eb to master...

@bors
Copy link
Contributor

bors commented Mar 15, 2023

👀 Test was successful, but fast-forwarding failed: 422 Changes must be made through a pull request.

@bors bors merged commit 94394eb into rust-lang:master Mar 15, 2023
@robjtede robjtede deleted the target-ignore branch March 15, 2023 18:17
@weihanglo weihanglo mentioned this pull request Mar 21, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 22, 2023
@bors
Auto merge of rust-lang#109463 - weihanglo:update-cargo, r=weihanglo
6dc3999 
Update cargo

11 commits in 4a3c588b1f0a8e2dc8dd8789dbf3b6a71b02ed49..15d090969743630bff549a1b068bcaa8174e5ee3
2023-03-14 14:05:36 +0000 to 2023-03-21 17:54:28 +0000
- docs(contrib): Move higher level resolver docs into doc comments (rust-lang/cargo#11870)
- docs(contrib): Pull impl info out of architecture (rust-lang/cargo#11869)
- Update curl-sys (rust-lang/cargo#11871)
- Poll loop fixes (rust-lang/cargo#11624)
- clippy: warn `disallowed_methods` for `std::env::var` and friends (rust-lang/cargo#11828)
- Add --ignore-rust-version flag to cargo install (rust-lang/cargo#11859)
- Handle case mismatches when looking up env vars in the Config snapshot (rust-lang/cargo#11824)
- align semantics of generated vcs ignore files (rust-lang/cargo#11855)
- Add more information to wait-for-publish (rust-lang/cargo#11713)
- docs: Address warnings (rust-lang/cargo#11856)
- docs(contrib): Create a file overview in the nightly docs (rust-lang/cargo#11850)
RalfJung pushed a commit to RalfJung/miri that referenced this pull request Mar 22, 2023
@bors
Auto merge of #109463 - weihanglo:update-cargo, r=weihanglo
0ed98a9 
Update cargo

11 commits in 4a3c588b1f0a8e2dc8dd8789dbf3b6a71b02ed49..15d090969743630bff549a1b068bcaa8174e5ee3
2023-03-14 14:05:36 +0000 to 2023-03-21 17:54:28 +0000
- docs(contrib): Move higher level resolver docs into doc comments (rust-lang/cargo#11870)
- docs(contrib): Pull impl info out of architecture (rust-lang/cargo#11869)
- Update curl-sys (rust-lang/cargo#11871)
- Poll loop fixes (rust-lang/cargo#11624)
- clippy: warn `disallowed_methods` for `std::env::var` and friends (rust-lang/cargo#11828)
- Add --ignore-rust-version flag to cargo install (rust-lang/cargo#11859)
- Handle case mismatches when looking up env vars in the Config snapshot (rust-lang/cargo#11824)
- align semantics of generated vcs ignore files (rust-lang/cargo#11855)
- Add more information to wait-for-publish (rust-lang/cargo#11713)
- docs: Address warnings (rust-lang/cargo#11856)
- docs(contrib): Create a file overview in the nightly docs (rust-lang/cargo#11850)
@ehuss ehuss added this to the 1.70.0 milestone Mar 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weihanglo weihanglo weihanglo approved these changes

Assignees

@weihanglo weihanglo

Labels
Command-new S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.70.0
Development

Successfully merging this pull request may close these issues.
6 participants
@robjtede @rustbot @weihanglo @epage @bors @ehuss