Respect .gitignore during `cargo new` #5733

withoutboats · 2018-07-17T06:31:49Z

When running cargo new, we check to see if you are inside a git repository. If you are, we do not initialize a new git repo for your project unless you specifically asked for it using --vcs. (See #1210 for more background).

This commit changes that behavior to also create a new repo if the project would be an ignored path in the parent repository. This way, if your home directory is a git repository, as long as you have ignored the directory you are creating a new project in, we will instantiate a git repository without you having to specifically request it.

rust-highfive · 2018-07-17T06:31:52Z

r? @matklad

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

withoutboats · 2018-07-17T06:33:02Z

cc @joshtriplett I figure you may be interested in this, since you filed #1210 way back.

alexcrichton · 2018-07-17T13:53:09Z

Looks good to me! Can you add a test for this as well?

bors · 2018-07-17T16:51:21Z

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

joshtriplett · 2018-07-17T18:52:00Z

This seems like a plausible heuristic.

withoutboats · 2018-07-19T11:33:20Z

@bors r=alexcrichton

bors · 2018-07-19T11:33:21Z

📌 Commit 4f2d55472c0d2a7f6f1931ad8441c110ea477c49 has been approved by alexcrichton

bors · 2018-07-19T11:33:28Z

⌛ Testing commit 4f2d55472c0d2a7f6f1931ad8441c110ea477c49 with merge 09f35b8c58816bd625e6125f727f9d7852229b8a...

bors · 2018-07-19T12:09:26Z

💔 Test failed - status-appveyor

withoutboats · 2018-07-19T16:30:56Z

It appears this behavior doesn't work on windows, I am unable to test locally & attempts to fix path issues have been unsuccessful.

Eh2406 · 2018-07-19T16:52:46Z

I am a windows based lifeform, I am able to reproduce locally. Investigating.

dwijnand · 2018-07-19T17:07:04Z

Does this fix it? dwijnand@ec27a6a

Eh2406 · 2018-07-19T17:23:59Z

edit: @dwijnand good thought, but no. I don't think it is the test.
(sorry @daboross)

I don't think the functionality is working. I don't have any .git anyware in target\cit at all. The reason is described here. That also means that the test subpackage_no_git is not testing anything. Looks like the proposal in this pr is so intuitive that we write tests having assume it was already inplace.

Eh2406 · 2018-07-19T17:57:22Z

looks like is_path_ignored is not working correctly. I added some println and it doesn't look correct.

code with println

pub fn existing_vcs_repo(path: &Path, cwd: &Path) -> bool {
    fn in_git_repo(path: &Path, cwd: &Path) -> bool {
        if let Ok(repo) = GitRepo::discover(path, cwd) {
            println!("repo: {:?}", repo.path().display());
            repo.is_path_ignored(path).map(|ignored| {
                println!("path: {:?} ignored: {:?}", path.display(), ignored);
                !ignored
            }).unwrap_or(true)
        } else { false }
    }

    in_git_repo(path, cwd) || HgRepo::discover(path, cwd).is_ok()
}

full output:

running 1 test
running `C:\\cargo\target\debug\cargo.exe new foo`
repo: "C:/cargo/.git/"
path: "C:\\cargo\\target\\cit\\t0" ignored: false
     Created binary (application) `foo` project
thread 'new::subpackage_no_git' panicked at '
Expected: ExistingDir
    but: C:\cargo\target\cit\t0\foo/.git was not a dir', tests\testsuite\hamcrest.rs:13:9
stack backtrace:

the kee output is:

repo: "C:/cargo/.git/"
path: "C:\\cargo\\target\\cit\\t0" ignored: false

target\\cit\\t0 is not ignored by C:/cargo/.git/ !?

daboross · 2018-07-19T18:29:17Z

Unintentional ping? Forwarding to @dwijnand.

withoutboats · 2018-07-20T11:57:36Z

It sounds like is_path_ignored does not work correctly on Windows? Probably a bug in libgit2?

Maybe for now we merge with this functionality only on unix & open a tracking issue for supporting it on windows?

dwijnand · 2018-07-20T12:20:38Z

It sounds like is_path_ignored does not work correctly on Windows? Probably a bug in libgit2?

Yup, see https://github.com/alexcrichton/git2-rs/issues/334

Eh2406 · 2018-07-20T12:32:02Z

Looks like is_path_ignored only accepts linux like paths, even though discover works with windows paths. I am working on a PR upstream to work around this bug by using to_string_lossy and replace.

We can wait for that PR and for a new version of libgit, or we can add the same workaround to this PR. Please don't leave a bunch of test broken on windows.

withoutboats · 2018-07-20T12:38:12Z

Please don't leave a bunch of test broken on windows.

I'm suggesting merging this PR by cfging out the one new test of this new functionality until the git2 patch has merged.

Eh2406 · 2018-07-20T12:43:39Z

In looking the new test to find this bug I realized that there are a lot of test that are nop, because they don't realize that no .git folders are created in target at all, and there are other test with workarounds for it.

I guess we could clean that mess up when we get it working on windows.

dwijnand · 2018-07-20T14:18:50Z

I'm with boats: let's accept the feature, with the test for Unix only, and forward fix trailing concerns.

alexcrichton · 2018-07-20T14:52:35Z

We can land this as a gated feature perhaps if it only works on Unix, but otherwise we can't land new functionality in Cargo that doesn't work on one of our tier 1 platforms

dwijnand · 2018-07-20T15:10:30Z

Oh, sorry, I'm still confused thinking the failure is the test on Windows, rather than the feature itself.

Eh2406 · 2018-07-23T23:39:45Z

This should work with git2 = "0.7.3". Thanks @alexcrichton for pushing that update so quickly!

I'd like to see the other test reviewed before this gets merged, but as long as they pass we can clean them up later.

dwijnand · 2018-07-26T07:22:25Z

This is failing CI validation because the pre-existing test fix::does_not_warn_about_dirty_ignored_files fails.

I think the failure is a git2 error (git2::Repository::discover(Path::new("/s/foo")).is_path_ignored("/s/foo") returns true with a .gitignore containing "foo"), and the upstream issue should be fixed to avoid it misfiring in cargo new and cargo fix

But, in the meanwhile, I've found you can tweak the test to make it pass by either:

making it gitignore and create a non "foo" file
move the project to a non "foo" directory (project().at("bar"))

When running `cargo new`, we check to see if you are inside a git repository. If you are, we do not initialize a new git repo for your project unless you specifically asked for it using --vcs. (See rust-lang#1210 for more background). This commit changes that behavior to *also* create a new repo if the project would be an ignored path in the parent repository. This way, if your home directory is a git repository, as long as you have ignored the directory you are creating a new project in, we will instantiate a git repository without you having to specifically request it.

alexcrichton · 2018-07-26T17:36:37Z

@bors: r+

bors · 2018-07-26T17:36:38Z

📌 Commit 064a146 has been approved by alexcrichton

alexcrichton · 2018-07-26T17:36:51Z

Thanks for the diagnosis @dwijnand!

bors · 2018-07-26T18:01:31Z

⌛ Testing commit 064a146 with merge b5a1dbd...

…excrichton Respect .gitignore during `cargo new` When running `cargo new`, we check to see if you are inside a git repository. If you are, we do not initialize a new git repo for your project unless you specifically asked for it using --vcs. (See #1210 for more background). This commit changes that behavior to *also* create a new repo if the project would be an ignored path in the parent repository. This way, if your home directory is a git repository, as long as you have ignored the directory you are creating a new project in, we will instantiate a git repository without you having to specifically request it.

bors · 2018-07-26T19:29:19Z

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing b5a1dbd to master...

now that we respect gitignore tests can be simplified There are a lot of test that used a tempfile to avoid the fact that cargo would not init a git in the test folder. (or because they were copy/pasted from one that did.) Now that #5733 landed we can remove them all.

rust-highfive assigned matklad Jul 17, 2018

withoutboats added Command-new Command-init labels Jul 17, 2018

withoutboats force-pushed the cargo-new-respects-gitignore branch 2 times, most recently from c259f89 to 4f2d554 Compare July 19, 2018 11:31

withoutboats force-pushed the cargo-new-respects-gitignore branch from 4f2d554 to a63f31d Compare July 19, 2018 12:55

Eh2406 mentioned this pull request Jul 19, 2018

is_path_ignored not working on windows rust-lang/git2-rs#334

Closed

This was referenced Jul 20, 2018

work around for windows path separator for is_path_ignored rust-lang/git2-rs#335

Merged

Don't warn about ignored files in cargo-fix #5770

Merged

Eh2406 mentioned this pull request Jul 24, 2018

Minimal versions build #5757

Merged

withoutboats force-pushed the cargo-new-respects-gitignore branch from a29dd74 to 064a146 Compare July 26, 2018 13:40

Eh2406 added a commit to Eh2406/cargo that referenced this pull request Jul 26, 2018

update test for rust-lang#5733

acf89e9

bors merged commit 064a146 into rust-lang:master Jul 26, 2018

Eh2406 mentioned this pull request Jul 26, 2018

now that we respect gitignore tests can be simplified #5810

Merged

ehuss added this to the 1.29.0 milestone Feb 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Respect .gitignore during `cargo new` #5733

Respect .gitignore during `cargo new` #5733

withoutboats commented Jul 17, 2018 •

edited

Loading

rust-highfive commented Jul 17, 2018

withoutboats commented Jul 17, 2018

alexcrichton commented Jul 17, 2018

bors commented Jul 17, 2018

joshtriplett commented Jul 17, 2018

withoutboats commented Jul 19, 2018

bors commented Jul 19, 2018

bors commented Jul 19, 2018

bors commented Jul 19, 2018

withoutboats commented Jul 19, 2018

Eh2406 commented Jul 19, 2018

dwijnand commented Jul 19, 2018

Eh2406 commented Jul 19, 2018 •

edited

Loading

Eh2406 commented Jul 19, 2018

daboross commented Jul 19, 2018

withoutboats commented Jul 20, 2018

dwijnand commented Jul 20, 2018

Eh2406 commented Jul 20, 2018

withoutboats commented Jul 20, 2018

Eh2406 commented Jul 20, 2018

dwijnand commented Jul 20, 2018

alexcrichton commented Jul 20, 2018

dwijnand commented Jul 20, 2018

Eh2406 commented Jul 23, 2018

dwijnand commented Jul 26, 2018

alexcrichton commented Jul 26, 2018

bors commented Jul 26, 2018

alexcrichton commented Jul 26, 2018

bors commented Jul 26, 2018

bors commented Jul 26, 2018

Respect .gitignore during cargo new #5733

Respect .gitignore during cargo new #5733

Conversation

withoutboats commented Jul 17, 2018 • edited Loading

rust-highfive commented Jul 17, 2018

withoutboats commented Jul 17, 2018

alexcrichton commented Jul 17, 2018

bors commented Jul 17, 2018

joshtriplett commented Jul 17, 2018

withoutboats commented Jul 19, 2018

bors commented Jul 19, 2018

bors commented Jul 19, 2018

bors commented Jul 19, 2018

withoutboats commented Jul 19, 2018

Eh2406 commented Jul 19, 2018

dwijnand commented Jul 19, 2018

Eh2406 commented Jul 19, 2018 • edited Loading

Eh2406 commented Jul 19, 2018

daboross commented Jul 19, 2018

withoutboats commented Jul 20, 2018

dwijnand commented Jul 20, 2018

Eh2406 commented Jul 20, 2018

withoutboats commented Jul 20, 2018

Eh2406 commented Jul 20, 2018

dwijnand commented Jul 20, 2018

alexcrichton commented Jul 20, 2018

dwijnand commented Jul 20, 2018

Eh2406 commented Jul 23, 2018

dwijnand commented Jul 26, 2018

alexcrichton commented Jul 26, 2018

bors commented Jul 26, 2018

alexcrichton commented Jul 26, 2018

bors commented Jul 26, 2018

bors commented Jul 26, 2018

Respect .gitignore during `cargo new` #5733

Respect .gitignore during `cargo new` #5733

withoutboats commented Jul 17, 2018 •

edited

Loading

Eh2406 commented Jul 19, 2018 •

edited

Loading