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

Add a lint to catch adding files that would otherwise be ignored by git. #9902

Merged
merged 1 commit into from Mar 8, 2018

Conversation

Projects
None yet
5 participants
@jdm
Copy link
Contributor

jdm commented Mar 7, 2018

This addresses the case where downstream syncing breaks because the sync operation excludes the ignored files.


This change is Reviewable

@w3c-bots

This comment has been minimized.

Copy link

w3c-bots commented Mar 7, 2018

Build PASSED

Started: 2018-03-08 19:22:36
Finished: 2018-03-08 19:43:15

View more information about this build on:

@jdm jdm force-pushed the jdm:ignored branch 2 times, most recently from 261dc0b to 3ffec91 Mar 7, 2018

@jdm

This comment has been minimized.

Copy link
Contributor Author

jdm commented Mar 7, 2018

It would be nicer to pass all paths to check-ignore at the same time, but that yielded mysterious errors when invoking the subprocess. Perhaps it was a command line argument length limit?

@jgraham

jgraham approved these changes Mar 7, 2018

Copy link
Contributor

jgraham left a comment

This seems fine as is, but I have a couple of questions/ideas that are optional to address.

@@ -134,6 +134,21 @@ def check_ahem_copy(repo_root, path):
return []


def check_git_ignore(repo_root, path):

This comment has been minimized.

Copy link
@jgraham

jgraham Mar 7, 2018

Contributor

Is there a way to make this work calling multiple paths at once? This seems like it isn't going to help performance.

This comment has been minimized.

Copy link
@jdm

jdm Mar 8, 2018

Author Contributor

I discovered a way to pass in filenames from a file, so this now does one check for all paths.

subprocess.check_output(["git", "--version"])
path_lints += [check_git_ignore]
except subprocess.CalledProcessError:
pass

This comment has been minimized.

Copy link
@jgraham

jgraham Mar 7, 2018

Contributor

This could print a warning or something? I'm not sure if that would break anything.

@jdm jdm force-pushed the jdm:ignored branch from 3ffec91 to cefd0bb Mar 8, 2018

@jgraham

jgraham approved these changes Mar 8, 2018

all_paths_lints += [check_git_ignore]
except subprocess.CalledProcessError:
print('No git present; skipping .gitignore lint.')
pass

This comment has been minimized.

Copy link
@jgraham

jgraham Mar 8, 2018

Contributor

This pass should be deleted, right?

This comment has been minimized.

Copy link
@jdm

jdm Mar 8, 2018

Author Contributor

Fixed.

@jdm jdm force-pushed the jdm:ignored branch from cefd0bb to 00f0e10 Mar 8, 2018

@jdm jdm force-pushed the jdm:ignored branch from 00f0e10 to d501958 Mar 8, 2018

@jgraham

jgraham approved these changes Mar 8, 2018

@jdm jdm merged commit b3b4332 into web-platform-tests:master Mar 8, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gsnedders

This comment has been minimized.

Copy link
Contributor

gsnedders commented Mar 13, 2018

This caused a regression: #10012

testharness_runner.html: testharness_runner.html matches an ignore filter in .gitignore - please add a .gitignore exception (IGNORED PATH)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.