Create .gitignore even if inside existing repository cargo new

[collin5]

Creates a VCS .ignore file if it doesn't exist or adds paths to be ignored when the project is created/initialized in an already existing VCS repository.

Fixes #6357

[rust-highfive]

r? @alexcrichton

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

[alexcrichton]

Thanks for the PR! I'm not really sure I'm quite following what's going on here though. If we didn't detect a git repository why would we then inject a gitignore anyway? Additionally how come the workdir is either one level up from a directory we just created or is the directory itself? I may be a bit lost on the rationale for this...

[collin5]

Currently fn mk() is being used by both cargo init and cargo new. For cargo init, Version Control will be the one used in the current dir and vcs will be Some option if present otherwise None. But for cargo new(even when inside a git repository), the path will be the newly to be created directory which in most cases if not all will be empty (meaning no vcs). This is why we get the vcs in the parent (workdir) that's if it's a vcs repo which we then use to create the appropriate .gitignore file.

[ehuss]

This does seem to be a little too special-cased looking at the parent directory. I'm wondering how this can be done in relatively safe way. I'd imagine existing_vcs_repo could be changed to report the actual repo kind and root. It would also probably need to check the current directory, or any parent up to the repo root, for Cargo.toml files just to be on the safe side of assuming it is in a workspace.

It may not be worth the complexity, since the workaround (manually updating the ignore file) is quite easy to do.

[collin5]

Making the root part of existing_vcs_repo actually sounds reasonable to me. @alexcrichton What's your take on this?

[alexcrichton]

I'd defer to @ehuss, sounds like a plausible route but perhaps more complicated than updating gitignore

[adarshaj]

@collin5 - are you still working on this one? If not, I am willing to pick it up and update it to what @ehuss suggested. lmk.

[collin5]

Oops! Sorry had lost track of this. Will be completing it in a few. Thanks

[collin5]

Updated! Also, I think we should just update/add the .ignore file regardless of whether we are in a cargo dir/workspace or not (unless I'm missing something here of course). Honestly, I'm not exactly sure why I was checking for this before 🙂. cc @ehuss @alexcrichton

[ehuss]

There seems to be a regression here. With this PR, every time I run new or init in a repo, it modifies the root .gitignore file with:

#Added by cargo
#
#already existing elements are commented out

#/target
#**/*.rs.bk

which seems like unwanted noise. It also doesn't seem to do anything else, which I'm not sure what the intent is here.

It might be helpful to write down exactly and very precisely what the desired behavior is. There are many edge cases here, and it seems like it will be easy to cause breakage.

[bors]

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

[collin5]

Sorry I had missed this. Was introduced after 99a23c0 . I've refactored fn format_existing() to only add paths that are missing instead of commenting out. Also added a description to the PR detailing the changes @ehuss

[ehuss]

Sorry for the delay in the review.

One minor thing, each time I run cargo new, it adds a blank line to gitignore. Can that be fixed?

[ehuss]

This might be a little safer to write as PathBuf::from(root.trim()) instead.

[ehuss]

Also, I'm not entire sure I understand the intent or outcome of this change. If you do cargo new inside a git repository, it will add /target to the root .gitignore file, but that won't match the target directory if it is not a workspace. If it is a workspace, you'll probably already have a gitignore.

I guess I still don't understand what the use case is.

[collin5]

Thanks. Fixed (including the blank line to .gitignore). The intent is;
When creating a subpackage (cargo project) inside an existing git repository which itself (the parent) may not be necessarily a cargo project, we should perhaps add **/target and/or **/*.rs.bk to the parent's .gitignore if not present. @ehuss.

[ehuss]

I think we explicitly decided in #6245 to not use **/target because it is at risk of ignoring legitimate directories.

There would also be the concern that if you are creating new projects inside a workspace, you really don't want anything modified. I think there's a fundamental problem here that cargo new doesn't know if you want to be in a workspace or not. Since there is a fairly high risk of doing the wrong thing, and the fix is trivial (manually modifying .gitignore), I'm not sure this can be easily addressed.

[collin5]

Fair enough. I guess we should close this for now.