bpo-37936: Systematically distinguish rooted vs. unrooted in .gitignore. by gnprice · Pull Request #15823 · python/cpython

gnprice · 2019-09-10T07:10:09Z

A root cause of bpo-37936 is that it's easy to write a .gitignore
rule that's intended to apply to a specific file (e.g., the
pyconfig.h generated by ./configure) but actually applies to all
similarly-named files in the tree (e.g., PC/pyconfig.h.)

Specifically, any rule with no non-trailing slashes is applied in an
"unrooted" way, to files anywhere in the tree. This means that if we
write the rules in the most obvious-looking way, then

for specific files we want to ignore that happen to be in
subdirectories (like Modules/config.c), the rule will work
as intended, staying "rooted" to the top of the tree; but
when a specific file we want to ignore happens to be at the root of
the repo (like platform), then the obvious rule (platform) will
apply much more broadly than intended: if someone tries to add a
file or directory named platform somewhere else in the tree, it
will unexpectedly get ignored.

That's surprising behavior that can make the .gitignore file's
behavior feel finicky and unpredictable.

To avoid it, we can simply always give a rule "rooted" behavior when
that's what's intended, by systematically using leading slashes.

Further, to help make the pattern obvious when looking at the file and
minimize any need for thinking about the syntax when adding new rules:
separate the rules into one group for each type, with brief comments
identifying them.

For most of these rules it's clear whether they're meant to be rooted
or unrooted, but in a handful of cases I've only guessed. In that
case the safer default (the choice that won't hide information) is the
narrower, rooted meaning, with a leading slash. If for some of these
the unrooted meaning is desired after all, it'll be easy to move them
to the unrooted section at the top.

While here, also made some rules more general where that seems
most natural, and deleted some rules that appear long obsolete.

Co-Authored-By: Zachary Ware zach@python.org

https://bugs.python.org/issue37936

A root cause of bpo-37936 is that it's easy to write a .gitignore rule that's intended to apply to a specific file (e.g., the `pyconfig.h` generated by `./configure`) but actually applies to all similarly-named files in the tree (e.g., `PC/pyconfig.h`.) Specifically, any rule with no non-trailing slashes is applied in an "unrooted" way, to files anywhere in the tree. This means that if we write the rules in the most obvious-looking way, then * for specific files we want to ignore that happen to be in subdirectories (like `Modules/config.c`), the rule will work as intended, staying "rooted" to the top of the tree; but * when a specific file we want to ignore happens to be at the root of the repo (like `platform`), then the obvious rule (`platform`) will apply much more broadly than intended: if someone tries to add a file or directory named `platform` somewhere else in the tree, it will unexpectedly get ignored. That's surprising behavior that can make the .gitignore file's behavior feel finicky and unpredictable. To avoid it, we can simply always give a rule "rooted" behavior when that's what's intended, by systematically using leading slashes. Further, to help make the pattern obvious when looking at the file and minimize any need for thinking about the syntax when adding new rules: separate the rules into one group for each type, with brief comments identifying them. For most of these rules it's clear whether they're meant to be rooted or unrooted, but in a handful of cases I've only guessed. In that case the safer default (the choice that won't hide information) is the narrower, rooted meaning, with a leading slash. If for some of these the unrooted meaning is desired after all, it'll be easy to move them to the unrooted section at the top.

zware

This mostly looks good, but I have several suggestions for further cleanup.

zware · 2019-09-10T07:22:45Z

.gitignore

+/config.status
+/config.status.lineno
+/db_home
+/.hg/


This can safely be ignored globally; we're not going to check in a mercurial repository anywhere :)

Solid prediction, I think :)

zware · 2019-09-10T09:34:19Z

.gitignore

+/python-gdb.py
+/python.exe-gdb.py
+/reflog.txt
+/.svn/


This should also be globally ignored.

zware · 2019-09-10T09:34:22Z

.gitignore

+/python.exe-gdb.py
+/reflog.txt
+/.svn/
+/.coverage


zware · 2019-09-10T09:40:09Z

.gitignore

+/config.status.lineno
+/db_home
+/.hg/
+/ipch/


I don't think this was meant to be rooted at /; IIRC it's intermediate files in the Windows build rooted in PCbuild/<something>, and thus can probably just go away since those entire directories are ignored now.

zware · 2019-09-10T09:42:30Z

.gitignore

+/libpython*.a
+/libpython*.so*
+/libpython*.dylib
+/libpython*.dll


Each of these 4 can be generalized to ignore anything with these extensions; we don't have anything with these extensions checked in and likely never will.

.gitignore

zware · 2019-09-10T09:55:40Z

.gitignore

-pybuilddir.txt
+/autom4te.cache
+/build/
+/buildno


I believe this is an ancient leftover from before the buildno section of sys.version referred to a VCS hash; it can just go away.

zware · 2019-09-10T10:00:03Z

.gitignore

+/python-config
+/python-config.py
+/python.bat
+/python.exe


We could globally ignore *.exe, with a specific exception for !Lib/distutils/command/*.exe.

Good idea, thanks!

zware · 2019-09-10T10:13:51Z

.gitignore

+/config.log
+/config.status
+/config.status.lineno
+/db_home


This is a holdover from the bsddb package, and can go away (so can the explicit callout of db_home in Lib/test/libregrtest/runtest.py).

Ah, indeed!

Looks like that disappeared in b98eb87 , in the Python 3.0 period (and the bit that made a db_home directory went away just before that, in d098ff2 .)

.gitignore

bedevere-bot · 2019-09-10T10:18:49Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be put in the comfy chair!

…bdirs

gnprice

Thanks @zware for the many helpful suggestions!

Just pushed an update making all your suggested changes, with the exception of the one on config.status; details below.

.gitignore

gnprice · 2019-09-11T03:47:56Z

.gitignore

+/config.log
+/config.status
+/config.status.lineno
+/db_home


Ah, indeed!

Looks like that disappeared in b98eb87 , in the Python 3.0 period (and the bit that made a db_home directory went away just before that, in d098ff2 .)

gnprice · 2019-09-11T03:51:07Z

.gitignore

+/libpython*.a
+/libpython*.so*
+/libpython*.dylib
+/libpython*.dll


gnprice · 2019-09-11T03:53:57Z

.gitignore

+/python-config
+/python-config.py
+/python.bat
+/python.exe


Good idea, thanks!

.gitignore

gnprice · 2019-09-11T04:13:18Z

.gitignore

+/config.status
+/config.status.lineno
+/db_home
+/.hg/


Solid prediction, I think :)

gnprice · 2019-09-11T04:23:56Z

@zware Also just edited the PR description (aka draft commit message) to credit you as a coauthor, because that seems appropriate given all the new substantive changes you contributed 🙂

(If you disagree, though, please feel free to take it out when merging.)

zware

LGTM, thanks!

miss-islington · 2019-09-11T09:25:29Z

Thanks @gnprice for the PR, and @zware for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7, 3.8.
🐍🍒⛏🤖

miss-islington · 2019-09-11T09:25:35Z

Sorry, @gnprice and @zware, I could not cleanly backport this to 3.8 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 455122a0094c8cfdf7e062eccc5e5b5885c75e8b 3.8

miss-islington · 2019-09-11T09:25:37Z

Sorry @gnprice and @zware, I had trouble checking out the 3.7 backport branch.
Please backport using cherry_picker on command line.
cherry_picker 455122a0094c8cfdf7e062eccc5e5b5885c75e8b 3.7

…itignore (pythonGH-15823) A root cause of bpo-37936 is that it's easy to write a .gitignore rule that's intended to apply to a specific file (e.g., the `pyconfig.h` generated by `./configure`) but actually applies to all similarly-named files in the tree (e.g., `PC/pyconfig.h`.) Specifically, any rule with no non-trailing slashes is applied in an "unrooted" way, to files anywhere in the tree. This means that if we write the rules in the most obvious-looking way, then * for specific files we want to ignore that happen to be in subdirectories (like `Modules/config.c`), the rule will work as intended, staying "rooted" to the top of the tree; but * when a specific file we want to ignore happens to be at the root of the repo (like `platform`), then the obvious rule (`platform`) will apply much more broadly than intended: if someone tries to add a file or directory named `platform` somewhere else in the tree, it will unexpectedly get ignored. That's surprising behavior that can make the .gitignore file's behavior feel finicky and unpredictable. To avoid it, we can simply always give a rule "rooted" behavior when that's what's intended, by systematically using leading slashes. Further, to help make the pattern obvious when looking at the file and minimize any need for thinking about the syntax when adding new rules: separate the rules into one group for each type, with brief comments identifying them. For most of these rules it's clear whether they're meant to be rooted or unrooted, but in a handful of cases I've only guessed. In that case the safer default (the choice that won't hide information) is the narrower, rooted meaning, with a leading slash. If for some of these the unrooted meaning is desired after all, it'll be easy to move them to the unrooted section at the top.. (cherry picked from commit 455122a) Co-authored-by: Greg Price <gnprice@gmail.com>

bedevere-bot · 2019-09-11T09:32:42Z

GH-15900 is a backport of this pull request to the 3.8 branch.

zware · 2019-09-11T09:36:43Z

The 3.7 backport is just complicated enough that I'm going to leave it alone.

Thanks for the patch, @gnprice!

…itignore (GH-15823) (GH-15900) A root cause of bpo-37936 is that it's easy to write a .gitignore rule that's intended to apply to a specific file (e.g., the `pyconfig.h` generated by `./configure`) but actually applies to all similarly-named files in the tree (e.g., `PC/pyconfig.h`.) Specifically, any rule with no non-trailing slashes is applied in an "unrooted" way, to files anywhere in the tree. This means that if we write the rules in the most obvious-looking way, then * for specific files we want to ignore that happen to be in subdirectories (like `Modules/config.c`), the rule will work as intended, staying "rooted" to the top of the tree; but * when a specific file we want to ignore happens to be at the root of the repo (like `platform`), then the obvious rule (`platform`) will apply much more broadly than intended: if someone tries to add a file or directory named `platform` somewhere else in the tree, it will unexpectedly get ignored. That's surprising behavior that can make the .gitignore file's behavior feel finicky and unpredictable. To avoid it, we can simply always give a rule "rooted" behavior when that's what's intended, by systematically using leading slashes. Further, to help make the pattern obvious when looking at the file and minimize any need for thinking about the syntax when adding new rules: separate the rules into one group for each type, with brief comments identifying them. For most of these rules it's clear whether they're meant to be rooted or unrooted, but in a handful of cases I've only guessed. In that case the safer default (the choice that won't hide information) is the narrower, rooted meaning, with a leading slash. If for some of these the unrooted meaning is desired after all, it'll be easy to move them to the unrooted section at the top. (cherry picked from commit 455122a) Co-authored-by: Greg Price <gnprice@gmail.com>

asottile · 2019-11-21T17:24:05Z

.gitignore

 *.iml
 *.o
+*.a
+*.so*


fwiw, this is causing a minor inconvenience in packaging for debian as this now matches debian/README.source

the original pattern was libpython*.so* could that be restored?

debian/README.source does not exist in the cpython repo, so presumably you can unignore it wherever you're patching it in. I would think you would want to explicitly unignore anything added to avoid issues like this.

I prefer to keep *.so*, because we we don't want to accidentally check in any compiled extension modules (even though that shouldn't happen anyway and any *.so other than libpython* should theoretically only appear in the ignored build directory, ignoring them anyway seems better to me).

However, I could be persuaded to expand that rule to

*.so *.so.*

If that works for you @asottile, please submit a PR and ping me.

will send a PR 👍

See python/cpython#15823 (comment)

In python#15823 the pattern was changed from `libpython*.so*` to `*.so*` which matches a bit too greedily for some packagers. For instance this trips up `debian/README.source`. A more specific pattern fixes this issue.

In GH-15823 the pattern was changed from `libpython*.so*` to `*.so*` which matches a bit too greedily for some packagers. For instance this trips up `debian/README.source`. A more specific pattern fixes this issue.

In pythonGH-15823 the pattern was changed from `libpython*.so*` to `*.so*` which matches a bit too greedily for some packagers. For instance this trips up `debian/README.source`. A more specific pattern fixes this issue.

…re (pythonGH-15823) A root cause of bpo-37936 is that it's easy to write a .gitignore rule that's intended to apply to a specific file (e.g., the `pyconfig.h` generated by `./configure`) but actually applies to all similarly-named files in the tree (e.g., `PC/pyconfig.h`.) Specifically, any rule with no non-trailing slashes is applied in an "unrooted" way, to files anywhere in the tree. This means that if we write the rules in the most obvious-looking way, then * for specific files we want to ignore that happen to be in subdirectories (like `Modules/config.c`), the rule will work as intended, staying "rooted" to the top of the tree; but * when a specific file we want to ignore happens to be at the root of the repo (like `platform`), then the obvious rule (`platform`) will apply much more broadly than intended: if someone tries to add a file or directory named `platform` somewhere else in the tree, it will unexpectedly get ignored. That's surprising behavior that can make the .gitignore file's behavior feel finicky and unpredictable. To avoid it, we can simply always give a rule "rooted" behavior when that's what's intended, by systematically using leading slashes. Further, to help make the pattern obvious when looking at the file and minimize any need for thinking about the syntax when adding new rules: separate the rules into one group for each type, with brief comments identifying them. For most of these rules it's clear whether they're meant to be rooted or unrooted, but in a handful of cases I've only guessed. In that case the safer default (the choice that won't hide information) is the narrower, rooted meaning, with a leading slash. If for some of these the unrooted meaning is desired after all, it'll be easy to move them to the unrooted section at the top.

the-knights-who-say-ni added the CLA signed label Sep 10, 2019

bedevere-bot added the awaiting review label Sep 10, 2019

gnprice mentioned this pull request Sep 10, 2019

bpo-37936: Systematically distinguish rooted vs. unrooted in .gitignore. gnprice/cpython#1

Closed

Add a NEWS entry.

96dde8c

zware requested changes Sep 10, 2019

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Sep 10, 2019

gnprice added 8 commits September 10, 2019 20:56

Cut redundant specific-.vs rule, thanks to zware's eagle eye

bdf15c8

Cut buildno rule.

9de8ab3

Cut db_home vestiges (from tests for bsddb, gone in 3.0)

e992e75

Cut rule for ipch -- believed to go inside already-ignored PCbuild su…

3ae3ab1

…bdirs

Generalize rules for *.a, *.so*, *.dylib, *.dll .

d4cac40

Generalize excluding *.exe .

19bc9e7

Ignore .hg/ and .svn/ anywhere.

2f0d018

Ignore .coverage anywhere.

175b726

gnprice commented Sep 11, 2019

View reviewed changes

zware approved these changes Sep 11, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels Sep 11, 2019

zware added needs backport to 3.7 labels Sep 11, 2019

zware merged commit 455122a into python:master Sep 11, 2019

bedevere-bot removed the awaiting merge label Sep 11, 2019

miss-islington assigned zware Sep 11, 2019

bedevere-bot removed the needs backport to 3.8 label Sep 11, 2019

zware removed the needs backport to 3.7 label Sep 11, 2019

gnprice deleted the pr-gitignore-rooted branch September 12, 2019 03:36

asottile reviewed Nov 21, 2019

View reviewed changes

asottile added a commit to deadsnakes/python3.9 that referenced this pull request Nov 21, 2019

Include un-gitignore for debian/README.source

d5ae4a3

See python/cpython#15823 (comment)

asottile added a commit to deadsnakes/python3.9 that referenced this pull request Nov 21, 2019

Include un-gitignore for debian/README.source

c336bb3

See python/cpython#15823 (comment)

asottile mentioned this pull request Nov 21, 2019

Be more specific about the .so gitignore patterns #17328

Merged

Uh oh!

Conversation

gnprice commented Sep 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zware left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bedevere-bot commented Sep 10, 2019

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnprice commented Sep 11, 2019

Uh oh!

zware left a comment

Choose a reason for hiding this comment

Uh oh!

miss-islington commented Sep 11, 2019

Uh oh!

miss-islington commented Sep 11, 2019

Uh oh!

miss-islington commented Sep 11, 2019

Uh oh!

bedevere-bot commented Sep 11, 2019

Uh oh!

zware commented Sep 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

gnprice commented Sep 10, 2019 •

edited

Loading