Patchwork D8345: tests: look for CRLF on Windows

login
register
mail settings
Submitter phabricator
Date March 30, 2020, 1:30 a.m.
Message ID <differential-rev-PHID-DREV-dfelr57z4vxxzw7rbfvl-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/45946/
State New
Headers show

Comments

phabricator - March 30, 2020, 1:30 a.m.
indygreg created this revision.
Herald added a reviewer: durin42.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  The ui.editor code calls util.tonativeeol() to normalize line endings
  to the platform native format. So, editor files may have CRLF
  in them. For some reason, just this one specific test case was
  failing on Python 3 on Windows. (The output before this change
  was `...\r (esc)`, but only on the first line of output. I suspect
  the test harness output parser was getting confused by the specific
  byte sequences in this output or something.
  
  I wanted to hack around this by filtering output via sed to remove
  the CR. But check-code says this practice is apparently banned.
  So I just made the test conditional.

REPOSITORY
  rHG Mercurial

BRANCH
  default

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

AFFECTED FILES
  tests/test-histedit-arguments.t

CHANGE DETAILS




To: indygreg, durin42, #hg-reviewers
Cc: mercurial-devel
phabricator - March 30, 2020, 4:51 a.m.
mharbison72 added a comment.


  The test harness *should* match existing `\n` output as a fallback, which got me to wondering if it was the `(esc)` at the end screwing it up.  I tried this patch:
  
    diff --git a/tests/run-tests.py b/tests/run-tests.py
    --- a/tests/run-tests.py
    +++ b/tests/run-tests.py
    @@ -2065,6 +2065,8 @@ class TTest(Test):
                 if PYTHON3:
                     el = el[:-7].decode('unicode_escape') + '\n'
                     el = el.encode('utf-8')
    +                log('el: %s' % el)
    +                log(' l: %s' % l)
                 else:
                     el = el[:-7].decode('string-escape') + '\n'
             if el == l or os.name == 'nt' and el[:-1] + b'\r\n' == l:
  
  ... and got this output:
  
    $ time py -3 run-tests.py --local test-histedit-arguments.t#abortflag --view bcompare
    running 1 tests using 1 parallel processes
    el: b'pick 3d3ea1f3a10b 5 1234567890123456789012345678901234567890123456789012345\xc3\xa3\xc2\x81\xc2\x82...\n'
     l: b'pick 3d3ea1f3a10b 5 1234567890123456789012345678901234567890123456789012345\xe3\x81\x82...\r\n'
  
  So I'm not sure where the bytes are being lost.  If this doesn't suggest a larger problem, I've got no problem with this patch as written.  But I wonder if there's some stdio that needs to be patched up, because I thought I saw that pattern in some failing py3 tests at one point.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8345/new/

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

To: indygreg, durin42, #hg-reviewers
Cc: mharbison72, mercurial-devel
phabricator - April 17, 2020, 6:33 p.m.
marmoute added a comment.


  @mharbison72 @indygreg, IIRC Matt is saying this is the wrong fix. What's the status of this?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8345/new/

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

To: indygreg, durin42, #hg-reviewers
Cc: marmoute, mharbison72, mercurial-devel
phabricator - April 17, 2020, 6:59 p.m.
mharbison72 added a comment.


  In D8345#126157 <https://phab.mercurial-scm.org/D8345#126157>, @marmoute wrote:
  
  > @mharbison72 @indygreg, IIRC Matt is saying this is the wrong fix. What's the status of this?
  
  I'm not sure that it's *wrong*, I'm just wondering if there's an encoding and/or stdio issue lurking here somewhere.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8345/new/

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

To: indygreg, durin42, #hg-reviewers
Cc: marmoute, mharbison72, mercurial-devel
phabricator - July 31, 2020, 5:56 p.m.
Herald added a subscriber: mercurial-patches.
This revision now requires changes to proceed.
baymax added a comment.
baymax requested changes to this revision.


  There seems to have been no activities on this Diff for the past 3 Months.
  
  By policy, we are automatically moving it out of the `need-review` state.
  
  Please, move it back to `need-review` without hesitation if this diff should still be discussed.
  
  :baymax:need-review-idle:

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8345/new/

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

To: indygreg, durin42, #hg-reviewers, marmoute, baymax
Cc: mercurial-patches, marmoute, mharbison72, mercurial-devel

Patch

diff --git a/tests/test-histedit-arguments.t b/tests/test-histedit-arguments.t
--- a/tests/test-histedit-arguments.t
+++ b/tests/test-histedit-arguments.t
@@ -308,6 +308,28 @@ 
   $ echo xx >> x
   $ hg --encoding utf-8 commit --logfile logfile
 
+Editor code calls util.tonativeeol, which only inserts CRLF on Python 3 on Windows.
+#if py3 windows
+  $ HGEDITOR=cat hg --encoding utf-8 histedit tip
+  pick 3d3ea1f3a10b 5 1234567890123456789012345678901234567890123456789012345\xe3\x81\x82...\r (esc)
+  \r (esc)
+  # Edit history between 3d3ea1f3a10b and 3d3ea1f3a10b\r (esc)
+  #\r (esc)
+  # Commits are listed from least to most recent\r (esc)
+  #\r (esc)
+  # You can reorder changesets by reordering the lines\r (esc)
+  #\r (esc)
+  # Commands:\r (esc)
+  #\r (esc)
+  #  e, edit = use commit, but stop for amending\r (esc)
+  #  m, mess = edit commit message without changing commit content\r (esc)
+  #  p, pick = use commit\r (esc)
+  #  b, base = checkout changeset and apply further changesets from there\r (esc)
+  #  d, drop = remove commit from history\r (esc)
+  #  f, fold = use commit, but combine it with the one above\r (esc)
+  #  r, roll = like fold, but discard this commit's description and date\r (esc)
+  #\r (esc)
+#else
   $ HGEDITOR=cat hg --encoding utf-8 histedit tip
   pick 3d3ea1f3a10b 5 1234567890123456789012345678901234567890123456789012345\xe3\x81\x82... (esc)
   
@@ -327,6 +349,7 @@ 
   #  f, fold = use commit, but combine it with the one above
   #  r, roll = like fold, but discard this commit's description and date
   #
+#endif
 
 Test --continue with --keep