Respect Cargo.toml [package.exclude] even not in a git repo.

[weihanglo]

May resolves #9054

This bug (or feature?) has been lingering for a while. #7680 fixed the cargo package part but cargo vendor is still affected by the heuristic rule of ignoring dotfiles. I propose to drop the rule and include dotfiles by default even if the package is not under git-controlled. See below.

Updated: Changes Summary

cargo vendor vendors dependencies without git-controlled but cargo package often runs under a VCS like git. These lines are where they diverges: fn list_files_walk_except_dot_files_and_dirs builds its own ignore instance, which cannot merge with other filter rules from [package.exclude]. This causes some patterns to not work as expected, such as re-including file after ignoring dotfiles [.*, !negated_file].

To make re-include (negate) rule works, this patch adds the excluding dotfiles rule directly into the package.exclude ignore instance if no include option nor git repo exists. Other old behaviors should not change in this patch.

[rust-highfive]

r? @alexcrichton

(rust-highfive has picked a reviewer for you, use r? to override)

[alexcrichton]

Thanks for the PR! Unfortunately it's been basically forever since I last looked at any of this and at this point I don't really remember much about what's going on here. I tried to read this for awhile and piece together what's going on, but I wasn't really able to get very far unfortunately.

You say that this may solve #9054, but does it?

The body of this looks like it's basically just reverting #7680, which I'm not sure why that works or the interactions it has with other parts of the systems. I'm surprised that at a high-level vendor/package are different since in theory they should both be using the same code paths now that they're both bundled in Cargo.

[weihanglo]

If I got it correctly, cargo::sources::path::list_files has these scenarios (similar to #7680 (comment)):

• In a git repo + without [package.include] field: respecting git status, .gitignore and exclude field from manifest.
• Not in a git repo + without [package.include] field: respecting .gitignore, but skipping dotfiles, which builds its own gitignorer and may accidentally overrides exclude field from manifest.
• With [package.include] field: respecting .gitignore and exclude field from manifest.

We can see that #9054 is under the second scenario (unpacked sources in registry/src without git controlled). Since an unpacked source is downloaded from a registry, and that means files in the source are all necessary for building the crate (author must exclude unnecessary files explicitly while uploading it to a registry). So, in my opinion all files in an unpacked source are safe to be vendored.

As far as I can think of, the only exception that would be affected by this change is a cargo project using version control tools other than git. Considering this is somewhat a breaking change, maybe we can disable skipping dotfiles functionality for cargo vendor instead. Though personally I prefer to remove the strong rule of skipping all dotfiles.

[alexcrichton]

I wonder if this could apply a similar change but basically try to fix just the use case mentioned? For example the default "exclude" builder could, even if not mentioned in the manifest, have a rule that says .* and then that can be overridden by subsequent rules?

[weihanglo]

@alexcrichton Updated. Please help review at your pace 😁

Note that it is still impossible to negate (include) a file inside a dot directory. This is the limitation of gitignore rule.

It is not possible to re-include a file if a parent directory of that file is excluded. Git doesn’t list excluded directories for performance reasons, so any patterns on contained files have no effect, no matter where they are defined.

[alexcrichton]

Ok sorry for the delay in getting to this, but I'm looking at this again. Would you be up for writing a summary of the change that this is now updated to doing? I think I have it in my head but I want to make sure I'm right. After that I'll get a sign-off from the rest of the team that this behavior is ok to change.

[weihanglo]

@alexcrichton Thank for you review!
A changes summary is added to PR description. 😄

[alexcrichton]

Ok thanks!

@rfcbot fcp merge

[rfcbot]

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Eh2406
• @alexcrichton
• @ehuss
• @joshtriplett

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[ehuss]

Thanks!

@bors r=alexcrichton

[bors]

📌 Commit 153146e has been approved by alexcrichton

[bors]

⌛ Testing commit 153146e with merge db741ac...

[bors]

☀️ Test successful - checks-actions
Approved by: alexcrichton
Pushing db741ac to master...