This is an archive of the discontinued LLVM Phabricator instance.

Add a .gitignore file to the root that ignores any files outside of the project directories.
Needs ReviewPublic

Authored by pcc on Jan 29 2019, 10:42 AM.

Details

Reviewers
jyknight
labath
Summary

This allows users to store build directories in the root directory
(as recommended by the new monorepo instructions) without them being
considered by git to be untracked.

Event Timeline

pcc created this revision.Jan 29 2019, 10:42 AM

Mmm, I think this adding a default /* ignore is setting us up for problems. I'd be okay with a default ignore for '/build*', though.

Do note also that users can ignore whatever they wish to locally by doing e.g. 'echo /my-special-build >> .git/info/excludes'

pcc abandoned this revision.Jan 29 2019, 11:08 AM

Mmm, I think this adding a default /* ignore is setting us up for problems. I'd be okay with a default ignore for '/build*', though.

That won't work as well for me because I don't name my build directories that way (I use short 1-2 character names to minimise typing). I also tend to put scratch files in the project root which aren't named in any particular way.

Do note also that users can ignore whatever they wish to locally by doing e.g. 'echo /my-special-build >> .git/info/excludes'

I'll probably just set up the excludes then. It's slightly less convenient for me because I'd need to set that up in each of my worktrees, but I can live with that.

pcc reclaimed this revision.Feb 15 2019, 9:13 AM

Reopening. I'm mildly inclined to push for this because:

  1. I don't really see what problems this will cause, while there's evidence that it would have prevented a problem [1]
  2. others seem to be in favour as well (D57429)
  3. we always have the option of backing this out if we do discover a problem with it, just like any other patch

[1] http://lists.llvm.org/pipermail/llvm-dev/2019-February/130234.html

I'm in favor of this and I agree with @pcc's points and reasoning behind this.

It means top level projects are explicitly whitelisted and this is something that's going to change far less often and at the same time takes good care to avoid accidental commits to the root of the monorepo. The file itself is easy enough to change if a new top level project is added, while at the same time tacking accidental new top level projects being created by accident because someone happens to have their artifact directories or out-of-tree projects within their tree. But since we know what (official) top level projects there currently are (and there aren't that many), keeping a whitelist seems to make a lot more sense. There are too many possibilities to add to .gitignore to account for any accidental stray directory ending up being committed, so I don't think just blacklisting build will cut it (as mentioned in D57429).

On the other hand if someone really is adding a new top level project (assuming it's been discussed on llvm-commits etc.), they can update the .gitignore file. This doesn't stop new top level projects being added, it simply means whoever is adding it is actually doing so explicitly, combined with a relevant update to .gitignore.

Hope that makes sense.

I still don't like it...It's different, unusual, and IMO surprising to have such a wildcard ignore.

We haven't tended to have lots of random accidental file additions before, and while someone may surely mess up again, I don't think it likely to be a common occurrence.

I'd much prefer simply the targeted ignore of "/build*", at least for starters.

I still don't like it...It's different, unusual, and IMO surprising to have such a wildcard ignore.

We haven't tended to have lots of random accidental file additions before, and while someone may surely mess up again, I don't think it likely to be a common occurrence.

I'd much prefer simply the targeted ignore of "/build*", at least for starters.

IMHO the rate of someone messing up, may be higher than the rate of new toplevel projects being added, which is really not very often. Considering this has already happened at least once, and considering that some people likely still use SVN for commits and may less familiar with Git (with it, IMO, being a lot less explicit than SVN as far as commits go). This isn't stopping people from adding new toplevel projects, it's simply more or less an additional step that needs to be taken as a safety measure to ensure the addition was intentional and not a result of accidentally committing something from a worktree. I think this may also provide more safety to anyone who has private directories that are not tracked and yet may accidentally be added to a commit. So I think aside from avoiding to predict every possible build directory imaginable, this will also stop the previous mentioned issue from ever happening as well as files like .gn or anything alike (such as BUILD or WORKSPACE) also being accidentally committed (it makes sense to have that in the root of the monorepo checkout for anyone actively using GN or other build systems, but it may not be desirable to *accidentally* commit them to the root of the repository). Same applies to any CI configuration or any other scripts that may end up in the root directory.

I think the reason for lack of accidental addition was partially to do with the fact that (IMO) it's much harder to accidentally do it with SVN than with Git and also meant that Git users still needed a separate SVN worktree to actually commit from. Which for some added an additional safety barrier ensuring that only intended patches were applied to the SVN worktree and committed as opposed to Git where one can use a single worktree (well eventually, after the full migration is complete) and commit/push directly from it.

Git allows a lot of flexibility for local work, which is definitely a good thing, however, I'm not entirely sure if that degree of flexibility is going to be good when actually pushing changes to upstream repository. So I simply think keeping a whitelist like that may be a better approach than blacklisting certain directories, which is pretty much more of a hit/miss approach.

FWIW, I am convinced by @kristina's arguments. Given that we store build trees in other stuff in the root folder (a practice which I am still trying to digest), I think we should have some safeguards to prevent them from accidentally being comitted. However, I don't feel like the right person to hit "approve" here.

FWIW, I am convinced by @kristina's arguments. Given that we store build trees in other stuff in the root folder (a practice which I am still trying to digest), I think we should have some safeguards to prevent them from accidentally being comitted. However, I don't feel like the right person to hit "approve" here.

+1, this is weird and I was skeptical, but am convinced (thanks for spelling out the arguments here @kristina!)

This really seems like a natural consequence of putting build directories alongside subprojects. If we're not willing to add this gitignore it seems like we should recommend an external build directory too to avoid confusion.

What about using /*/ at the ignore pattern? This allows top-level files, and makes only new top-level *directories* require an ignore update. To my mind, that seems a bit more narrowly scoped and might be a bit less surprising. Thoughts?

What about using /*/ at the ignore pattern? This allows top-level files, and makes only new top-level *directories* require an ignore update. To my mind, that seems a bit more narrowly scoped and might be a bit less surprising. Thoughts?

I think that's reasonable, small accidental files being committed to root of the monorepo should not be a huge deal anyway and easily undoable and unlike directories won't accidentally cause hundreds of people to check out someone's accidentally committed build artifact directory. I'm happy with this solution if others are.

pcc added a comment.Jul 17 2019, 10:22 AM

Using /*/ as the ignore pattern would work for me. My main concern was that it would conflict with my use of /* in the exclude file to ignore files in the root directory, but it appears that they don't conflict. If @jyknight is happy with this then I'll update this review as suggested.

What about downstream users that have added directories in their local forks? Having git suddenly ignore them would be surprising. We are in that situation.

What about downstream users that have added directories in their local forks? Having git suddenly ignore them would be surprising. We are in that situation.

Ditto, however I am more concerned about some accidental commit bloating the size of the upstream repo (which AIUI would remain even after a revert) than tripping over something unexpected in my downstream repo.

FTR, we already have a .gitignore in our downstream repo for much the same reason as described in previous comments supporting this patch. If nothing else, it means we'd have a merge conflict when this went in, and we'd DTRT with it at that time.