Issue #15723: Skip git ignored files in update-manifest.

[kregoslup]

This patch makes "./mach update-manifest" aware of .gitignore.

---

• ./mach build -d does not report any errors
• ./mach test-tidy does not report any errors
• These changes fix #15725 (github issue number if applicable).

• There are tests for these changes OR
• These changes do not require tests because I don't know how to test it, I have tried mocking opening file and checking whether it exists but it's difficult to handle both in py2.7 and py3 and I think it would say too much about PathFilter internals. I have also tried mocking PathFilter but then I would be duplicating logic in tests.

---

This change is 

[highfive]

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @emilio (or someone else) soon.

[tigercosmos]

r? @emilio

[jdm]

r? @jgraham
This touches enough code that I'm unfamiliar with that it would be good to have your eyes on it.

[nox]

That change will get upstreamed automatically, right?

[jdm]

Yes.

[jgraham]

So I think this is the wrong fix. update() is called with a tree object from the confusingly named vcs.py and the Git tree class presumably considers .gitignore by default and the FileSystem tree class imports and uses the gitignore module. So, unless I'm missing something, if this patch changes the behaviour, it's either because the existing code is buggy and should be fixed, or because the caller from the servo mach command is passing in the wrong kind of object. In either case the fix should be closer to the source.

[jdm]

Note: update-manifest command is at https://github.com/kregoslup/servo/blob/687e6378df1b242cbaa425cc9b7686f63201d708/python/servo/testing_commands.py#L426-L433, which invokes https://github.com/kregoslup/servo/blob/687e6378df1b242cbaa425cc9b7686f63201d708/python/servo/testing_commands.py#L362-L394, which invokes https://github.com/kregoslup/servo/blob/687e6378df1b242cbaa425cc9b7686f63201d708/tests/wpt/run_wpt.py.

[kregoslup]

OK, I think I found the bug but I'm not sure, can I ask a few questions?
Issue happens only in tests/wpt/mozilla/, because there is no .gitignore file in there. When

servo/tests/wpt/web-platform-tests/tools/manifest/update.py

Lines 18 to 26 in 687e637

	def update(tests_root, manifest, working_copy=False):
	logger.info("Updating manifest")
	tree = None
	if not working_copy:
	tree = vcs.Git.for_path(tests_root, manifest.url_base)
	if tree is None:
	tree = vcs.FileSystem(tests_root, manifest.url_base)
	return manifest.update(tree)

update function calls manifest update with a tree object, either git tree or filesystem tree, PathFilter finds no .gitignore in that directory and here

servo/tests/wpt/web-platform-tests/tools/gitignore/gitignore.py

Lines 90 to 112 in 687e637

	class PathFilter(object):
	def __init__(self, root, extras=None):
	if root:
	ignore_path = os.path.join(root, ".gitignore")
	else:
	ignore_path = None
	if not ignore_path and not extras:
	self.trivial = True
	return
	self.trivial = False
	self.rules_file = []
	self.rules_dir = []
	if extras is None:
	extras = []
	if ignore_path and os.path.exists(ignore_path):
	self._read_ignore(ignore_path)
	for item in extras:
	self._read_line(item)

has no extras argument and does not read gitignore file because it does not exists and trivial attribute is set to False because ignore_path variable is not None. My question is, which of these solutions do you think would be the best? I'm not sure what's the expected behaviour in this situation.

1. Add .gitignore to tests/wpt/mozilla/ ?
2. Add looking for additional gitignore files in other directiories?
3. Change PathFilter behaviour to inform or log or set that it hasn't found any gitignore file in tests root directory or set its trivial attribute to True?

I think I'll go with 1 and 3, please let me know what do you think about that.

[jdm]

Let's add a .gitignore to tests/wpt/mozilla.

[jdm]

@kregoslup This PR now contains a single commit that only adds a .gitignore file. Where did the rest of the work go?

[kregoslup]

@jdm I figured out that the bug was only missing gitignore, so I amended previous commit and added only .gitignore file to path where it was missing. Do you mean that I should add tests?

[jdm]

@bors-servo r+
Oh, huh. That works for me!

[bors-servo]

📌 Commit 961e4a1 has been approved by jdm

[bors-servo]

⌛ Testing commit 961e4a1 with merge 84513d4...

[bors-servo]

💔 Test failed - linux-rel-css

[jdm]

@bors-servo: retry

• #13480

[bors-servo]

⚡ Previous build results for android, arm32, arm64, linux-dev, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev are reusable. Rebuilding only linux-rel-css...

[bors-servo]

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: jdm
Pushing 84513d4 to master...