Patchwork D5981: tests: drop a few unnecessary "(glob)"

login
register
mail settings
Submitter phabricator
Date Feb. 18, 2019, 5:24 a.m.
Message ID <differential-rev-PHID-DREV-hykw6cgcaq4le7vxg3ih-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/38811/
State New
Headers show

Comments

phabricator - Feb. 18, 2019, 5:24 a.m.
martinvonz created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  I believe thise are unnecessary since https://phab.mercurial-scm.org/rHGa8d3a4be066e0d3b3d01a90a98dc1907cf4fad3b (windows: use
  util.localpath for repo-relative paths in getuipathfn(), 2019-02-11),
  but someone with a Windows machine should confirm.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D5981

AFFECTED FILES
  tests/test-status-color.t
  tests/test-status.t

CHANGE DETAILS




To: martinvonz, #hg-reviewers
Cc: mercurial-devel
phabricator - Feb. 18, 2019, 5:32 p.m.
mharbison72 added a comment.


  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).
  
  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.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D5981

To: martinvonz, #hg-reviewers, mharbison72
Cc: mercurial-devel
phabricator - Feb. 19, 2019, 5:53 a.m.
martinvonz added a comment.


  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?
  
  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?
  
  > 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.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D5981

To: martinvonz, #hg-reviewers, mharbison72
Cc: mercurial-devel
phabricator - Feb. 19, 2019, 3:14 p.m.
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.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D5981

To: martinvonz, #hg-reviewers, mharbison72
Cc: mercurial-devel

Patch

diff --git a/tests/test-status.t b/tests/test-status.t
--- a/tests/test-status.t
+++ b/tests/test-status.t
@@ -116,11 +116,11 @@ 
   ? ../b/in_b
   ? ../in_root
   $ HGPLAIN=1 hg status --cwd a --config ui.tweakdefaults=yes
-  ? a/1/in_a_1 (glob)
-  ? a/in_a (glob)
-  ? b/1/in_b_1 (glob)
-  ? b/2/in_b_2 (glob)
-  ? b/in_b (glob)
+  ? a/1/in_a_1
+  ? a/in_a
+  ? b/1/in_b_1
+  ? b/2/in_b_2
+  ? b/in_b
   ? in_root
   $ HGPLAINEXCEPT=tweakdefaults hg status --cwd a --config ui.tweakdefaults=yes
   ? 1/in_a_1
@@ -163,11 +163,11 @@ 
   ? ../b/in_b
   ? ../in_root
   $ HGPLAIN=1 hg status --cwd a
-  ? a/1/in_a_1 (glob)
-  ? a/in_a (glob)
-  ? b/1/in_b_1 (glob)
-  ? b/2/in_b_2 (glob)
-  ? b/in_b (glob)
+  ? a/1/in_a_1
+  ? a/in_a
+  ? b/1/in_b_1
+  ? b/2/in_b_2
+  ? b/in_b
   ? in_root
 
 if relative paths are explicitly off, tweakdefaults doesn't change it
diff --git a/tests/test-status-color.t b/tests/test-status-color.t
--- a/tests/test-status-color.t
+++ b/tests/test-status-color.t
@@ -31,11 +31,11 @@ 
   [status.unknown|? ][status.unknown|in_root]
 HGPLAIN disables color
   $ HGPLAIN=1 hg status --color=debug
-  ? a/1/in_a_1 (glob)
-  ? a/in_a (glob)
-  ? b/1/in_b_1 (glob)
-  ? b/2/in_b_2 (glob)
-  ? b/in_b (glob)
+  ? a/1/in_a_1
+  ? a/in_a
+  ? b/1/in_b_1
+  ? b/2/in_b_2
+  ? b/in_b
   ? in_root
 HGPLAINEXCEPT=color does not disable color
   $ HGPLAINEXCEPT=color hg status --color=debug