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

proposal: reconsider allowing emacs and vim patterns in gitignore files #23800

Closed
bradfitz opened this issue Feb 13, 2018 · 11 comments
Closed

proposal: reconsider allowing emacs and vim patterns in gitignore files #23800

bradfitz opened this issue Feb 13, 2018 · 11 comments

Comments

@bradfitz
Copy link
Contributor

We seem to have a policy against adding common Emacs and vim gitignore lines.

I propose we allow them.

See recent CL where @ianlancetaylor tried to add them, to @robpike's objection:
https://go-review.googlesource.com/c/tools/+/93415

Then @ianlancetaylor reverted it, but accidentally added emacs junk files to the CL (in PS1), kinda proving that they would've been helpful in preventing garbage being added to the repo:
https://go-review.googlesource.com/c/tools/+/93437

Regarding per-user gitignore files, I wrote in the above CL comments:

We could have every Emacs & vim user add them, or we can add them once where everybody picks them up. Both options work. I prefer saving everybody the hassle.

See related bug #21458 that's about removing stuff from the "go" repo's .gitignore. But notably, the "go" repo's .gitignore contains the Emacs/etc lines.

Our current policy seems to only be about not adding them to the other repos, which is inconsistent.

I'd prefer we be consistent and ignore *~, etc in all repos.

@gopherbot gopherbot added this to the Proposal milestone Feb 13, 2018
@dsymonds
Copy link
Contributor

There's myriad editors that drop working files in all sorts of places. I'm much more in favour of people who use editor X having to do a one-off configuration to stop git trying to track editor X's temp files.

@dsymonds
Copy link
Contributor

Note also that there are oodles of git repositories floating around, not just ones for the Go project. Even if all of Go's git repositories had a suitable .gitignore file, hapless Emacsers can still find themselves in a pickle.

@josharian
Copy link
Contributor

There's myriad editors that drop working files in all sorts of places.

There's also a very heavy concentration of users in just a few editors who put stuff in a few standard places--and that's what this proposal is about.

@bradfitz
Copy link
Contributor Author

Yes. What @josharian said. This is about low hanging fruit, not solving every possible junk file.

@dmitshur
Copy link
Contributor

dmitshur commented Feb 13, 2018

One benefit of the current policy is that it makes it easy to reject CLs (and discourage them from being created) that try to add "SomeObscureAndQuestionableFile" to .gitignore.

If this proposal is accepted, will the policy be modified to match the new .gitignore contents? Something like this?

-# Add no patterns to .gitignore except for files generated by the build.
+# Add no patterns to .gitignore except for files generated by the build,
+# common Emacs and vim files.

I'm asking this because I think it'd be helpful to think through what it'll be like if that's the future we go with.

@cznic
Copy link
Contributor

cznic commented Feb 13, 2018

I prefer .gitignore files being local to the repository clone/developer's machine and never committed to the repository.

@dominikh
Copy link
Member

dominikh commented Feb 13, 2018

I prefer .gitignore files being local to the repository clone/developer's machine and never committed to the repository.

There are (at least) three ways of ignoring files in git:

  1. locally, for a specific repository: .git/info/exclude – this can be used for per-project files such as custom shell scripts or notes
  2. locally, for all repositories: configure core.excludesfile globally, otherwise the same as option 1. Can be used for stuff like Emacs backup files.
  3. distributed, for a specific repository: .gitignore – this is used for ignored files that all developers on the project will encounter, such as artifacts of the build system.

In summary, developers should configure their machines (core.excludesfile) and their local clones (.git/info/exclude). Project maintainers should maintain the project's .gitignore, with entries that will apply to all developers.

I am against the proposal.

@josharian
Copy link
Contributor

I'll try once more to make this case for this proposal.

I agreed, adding these to .gitignore is not the Right Way. It is nevertheless a reasonable, practical thing to do.

If we were a small stable team, we could put in a bit of effort to get all team members on the same page. Instead, we're a large, sprawling open source project. I'd rather spend my limited educational and review bandwidth on things other than git config.

And even folks who do things the Right Way (including me) sometimes encounter little bumps. Maybe I briefly switch editors to avoid my gofmt-on-save hooks, or I hand my keyboard to a colleague who fires up emacs, or I decide to try out Atom for a day.

We write a lot of code to prevent and fix other mistakes: pre-submit hooks, git-codereview, maintner, gerritbot, etc. That code generates bugs and feature ideas, requires maintenance, etc. This proposal is exactly the same category of fix–preventing simple, human mistakes–but with the added advantage of being extremely easy to write and maintain.

As to the slippery slope argument (of which @shurcooL's question is a variant), I have two responses. First, as Rob likes to say, the Go project is pretty good at finding traction when it is important. And second, for all the reasons listed above, I don't really think it'd be that bad if .gitignore grew organically in response to mistakes that people make, the same way git-codereview has.

And since we all have bigger fish to fry, I'll leave it here.

@robpike
Copy link
Contributor

robpike commented Feb 13, 2018

I'm OK with adding things to .gitignore if the policy of what is OK is clear. It's clear now: only things created by the build. This issue has hinted at a new policy but it's peculiar (add emacs and vim temp files - why only them? The modern world is moving away from these tools) and not well justified as the strict policy.

Actually by "I'm OK" I mean I'll go along with it if it makes sense and limits churn. I would prefer we left the policy as is, but don't want to be the lone holdout.

Define the policy. For an example of what a clear policy can do, look at how cmd/vet/README has formed the discussion about what goes into vet, with excellent results. The .gitignore story is less important but the lesson still holds.

@rsc
Copy link
Contributor

rsc commented Feb 26, 2018

.gitignore is good but doesn't have teeth.
@bradfitz points out that #18548 would be a better place for this check (also #10658).
I think it would also be fine to put this check in git-codereview, so that people using it get that benefit.
Git-codereview could also diagnose executables in file lists.

@rsc
Copy link
Contributor

rsc commented Mar 5, 2018

Closed in favor of #18548 and #24139. Let's keep the policy small and avoid syncing .gitignore files in all our repos.

@rsc rsc closed this as completed Mar 5, 2018
brho added a commit to akaros/go-akaros that referenced this issue Oct 3, 2018
Lord only knows what we'll need to do when we update to HEAD.

golang#23800

Signed-off-by: Barret Rhoden <brho@cs.berkeley.edu>
@golang golang locked and limited conversation to collaborators Mar 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

9 participants