D5981: tests: drop a few unnecessary "(glob)"
mharbison72 (Matt Harbison)
phabricator at mercurial-scm.org
Tue Feb 19 10:14:26 EST 2019
mharbison72 added a comment.
In https://phab.mercurial-scm.org/D5981#87343, @martinvonz wrote:
> In https://phab.mercurial-scm.org/D5981#87341, @mharbison72 wrote:
> > The tests run clean with this.
> > But I'm a bit confused. The test harness has been doing '\' -> '/' conversion without (glob) now for a little over a year, and it complains if there's a trailing (glob) and no '\' -> '/' conversion. That's not been happening here, and made me suspicious. These globs predate that functionality slightly, so I'm not sure the meaning of the referenced commit (which seems to say nothing will change because ui.slash is set).
> You're right, and even if I go back to https://phab.mercurial-scm.org/rHGbdcaf612e75a08f20257076845510353a448b57d (where you added these globs), it seems like it should have been converted to slashes already there (because the test runner set ui.slash back then, too, and the code seemed to respect that). Do you have time to go back and see if the globbing was never needed?
It was needed. In fact the test runner itself added it on Windows. (There's logic in there to add it to the output if changing '\' to '/' results in a match.) I bisected the `HGPLAIN=1 hg status --cwd a` test back to https://phab.mercurial-scm.org/rHG7e3b145f824793ccdb5caf4f13570d4f25ab0164, where the trail went cold- it fails there too where it was added. Backing up one more and reverting the test to that also fails, so it wasn't a code change there causing it.
(As an aside, it would be awesome if bisect accepted a --force to ignore the dirty working copy check. Then I could take out the globs, and see where it started working without having to manually update to each candidate. I guess the question then is whether to merge the dirty changes on each update, or just revert to the dirty changes as-is. I guess I'd lean towards the latter for automated bisection.)
> Maybe the "no complaining from test harness" is because these lines contain a "?", which is a glob character, so maybe it doesn't warn then?
Strangely, no. I added a (glob) to a random status test that listed only the file with an unknown status in https://phab.mercurial-scm.org/rHG37a0ad669051ab70b89aaf669a7c277a908cf4b4 and https://phab.mercurial-scm.org/rHG37b33c34bf4f890857b5e8728febbc82a99368a5, and they both took away the stray (glob).
>> To be clear, these paths are still being printed with '\', and the test harness is accommodating that. But I'm not sure if that's what you intended.
> Yes, I hope that my recent changes has made it so more commands, not fewer, print paths in the native form.
To: martinvonz, #hg-reviewers, mharbison72
More information about the Mercurial-devel