Patchwork test-merge-tools: stabilize for Windows

login
register
mail settings
Submitter Matt Harbison
Date Feb. 12, 2018, 2:39 a.m.
Message ID <29c42f65c6b609a6bc42.1518403154@Envy>
Download mbox | patch
Permalink /patch/27634/
State Accepted
Headers show

Comments

Matt Harbison - Feb. 12, 2018, 2:39 a.m.
# HG changeset patch
# User Matt Harbison <matt_harbison@yahoo.com>
# Date 1518400775 18000
#      Sun Feb 11 20:59:35 2018 -0500
# Node ID 29c42f65c6b609a6bc429fc6d0963d1eb7cc951b
# Parent  f91b7f26c68ac87961aa6ef883ba96e5a2822ad3
test-merge-tools: stabilize for Windows

This masks the Windows argument parsing insanity[1], so it needs a bit of
explanation.  (The security reference in the footnote is probably useful to keep
in mind if we ever whitelist certain in-repo config settings.)

9037c29e9f53 introduced tests that were failing on Windows with an unbalanced
double quote[2].  What ends up happening here is util.shellquote() is double
quoting the file path, but the shell script is placing this ->": "<- right next
to it.  So cmd.exe gets launched with 'lb:base": ""c:\...\f~base.xyz"', which
got interpreted as 'lb:base: "c:\...\f~base.xyz'. If the test is adjusted to
quote like "lb:$labelbase: $base", then MSYS runs interference and strips the
'\' characters.  I was able to get the expected result by dropping the quotes
from '": "', and changing the space to underscore.  But since we need to glob
away the C: part anyway, just glob away the quote and leave the test unchanged.

[1] https://blogs.msdn.microsoft.com/twistylittlepassagesallalike/2011/04/23/everyone-quotes-command-line-arguments-the-wrong-way/
[2] https://buildbot.mercurial-scm.org/builders/Win7%20x86_64%20hg%20tests/builds/441/steps/run-tests.py%20%28python%202.7.13%29/logs/stdio
Yuya Nishihara - Feb. 12, 2018, 10:27 a.m.
On Sun, 11 Feb 2018 21:39:14 -0500, Matt Harbison wrote:
> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1518400775 18000
> #      Sun Feb 11 20:59:35 2018 -0500
> # Node ID 29c42f65c6b609a6bc429fc6d0963d1eb7cc951b
> # Parent  f91b7f26c68ac87961aa6ef883ba96e5a2822ad3
> test-merge-tools: stabilize for Windows
> 
> This masks the Windows argument parsing insanity[1], so it needs a bit of
> explanation.  (The security reference in the footnote is probably useful to keep
> in mind if we ever whitelist certain in-repo config settings.)
> 
> 9037c29e9f53 introduced tests that were failing on Windows with an unbalanced
> double quote[2].  What ends up happening here is util.shellquote() is double
> quoting the file path, but the shell script is placing this ->": "<- right next
> to it.  So cmd.exe gets launched with 'lb:base": ""c:\...\f~base.xyz"', which
> got interpreted as 'lb:base: "c:\...\f~base.xyz'. If the test is adjusted to
> quote like "lb:$labelbase: $base", then MSYS runs interference and strips the
> '\' characters.  I was able to get the expected result by dropping the quotes
> from '": "', and changing the space to underscore.  But since we need to glob
> away the C: part anyway, just glob away the quote and leave the test unchanged.
> 
> [1] https://blogs.msdn.microsoft.com/twistylittlepassagesallalike/2011/04/23/everyone-quotes-command-line-arguments-the-wrong-way/
> [2] https://buildbot.mercurial-scm.org/builders/Win7%20x86_64%20hg%20tests/builds/441/steps/run-tests.py%20%28python%202.7.13%29/logs/stdio

Perhaps, we can instead fix printargs_merge_tool to take prefix:variable pairs
as separate arguments:

  echo "arg: $1:$2"
  shift 2
Matt Harbison - Feb. 12, 2018, 1:06 p.m.
> On Feb 12, 2018, at 5:27 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> 
>> On Sun, 11 Feb 2018 21:39:14 -0500, Matt Harbison wrote:
>> # HG changeset patch
>> # User Matt Harbison <matt_harbison@yahoo.com>
>> # Date 1518400775 18000
>> #      Sun Feb 11 20:59:35 2018 -0500
>> # Node ID 29c42f65c6b609a6bc429fc6d0963d1eb7cc951b
>> # Parent  f91b7f26c68ac87961aa6ef883ba96e5a2822ad3
>> test-merge-tools: stabilize for Windows
>> 
>> This masks the Windows argument parsing insanity[1], so it needs a bit of
>> explanation.  (The security reference in the footnote is probably useful to keep
>> in mind if we ever whitelist certain in-repo config settings.)
>> 
>> 9037c29e9f53 introduced tests that were failing on Windows with an unbalanced
>> double quote[2].  What ends up happening here is util.shellquote() is double
>> quoting the file path, but the shell script is placing this ->": "<- right next
>> to it.  So cmd.exe gets launched with 'lb:base": ""c:\...\f~base.xyz"', which
>> got interpreted as 'lb:base: "c:\...\f~base.xyz'. If the test is adjusted to
>> quote like "lb:$labelbase: $base", then MSYS runs interference and strips the
>> '\' characters.  I was able to get the expected result by dropping the quotes
>> from '": "', and changing the space to underscore.  But since we need to glob
>> away the C: part anyway, just glob away the quote and leave the test unchanged.
>> 
>> [1] https://blogs.msdn.microsoft.com/twistylittlepassagesallalike/2011/04/23/everyone-quotes-command-line-arguments-the-wrong-way/
>> [2] https://buildbot.mercurial-scm.org/builders/Win7%20x86_64%20hg%20tests/builds/441/steps/run-tests.py%20%28python%202.7.13%29/logs/stdio
> 
> Perhaps, we can instead fix printargs_merge_tool to take prefix:variable pairs
> as separate arguments:
> 
>  echo "arg: $1:$2"
>  shift 2

I can try that, but the glob adjustment is still necessary (the drive letter isn’t the same on all platforms), so this fix is effectively hidden.
Yuya Nishihara - Feb. 12, 2018, 2:24 p.m.
On Mon, 12 Feb 2018 08:06:12 -0500, Matt Harbison wrote:
> 
> > On Feb 12, 2018, at 5:27 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> > 
> >> On Sun, 11 Feb 2018 21:39:14 -0500, Matt Harbison wrote:
> >> # HG changeset patch
> >> # User Matt Harbison <matt_harbison@yahoo.com>
> >> # Date 1518400775 18000
> >> #      Sun Feb 11 20:59:35 2018 -0500
> >> # Node ID 29c42f65c6b609a6bc429fc6d0963d1eb7cc951b
> >> # Parent  f91b7f26c68ac87961aa6ef883ba96e5a2822ad3
> >> test-merge-tools: stabilize for Windows
> >> 
> >> This masks the Windows argument parsing insanity[1], so it needs a bit of
> >> explanation.  (The security reference in the footnote is probably useful to keep
> >> in mind if we ever whitelist certain in-repo config settings.)
> >> 
> >> 9037c29e9f53 introduced tests that were failing on Windows with an unbalanced
> >> double quote[2].  What ends up happening here is util.shellquote() is double
> >> quoting the file path, but the shell script is placing this ->": "<- right next
> >> to it.  So cmd.exe gets launched with 'lb:base": ""c:\...\f~base.xyz"', which
> >> got interpreted as 'lb:base: "c:\...\f~base.xyz'. If the test is adjusted to
> >> quote like "lb:$labelbase: $base", then MSYS runs interference and strips the
> >> '\' characters.  I was able to get the expected result by dropping the quotes
> >> from '": "', and changing the space to underscore.  But since we need to glob
> >> away the C: part anyway, just glob away the quote and leave the test unchanged.
> >> 
> >> [1] https://blogs.msdn.microsoft.com/twistylittlepassagesallalike/2011/04/23/everyone-quotes-command-line-arguments-the-wrong-way/
> >> [2] https://buildbot.mercurial-scm.org/builders/Win7%20x86_64%20hg%20tests/builds/441/steps/run-tests.py%20%28python%202.7.13%29/logs/stdio
> > 
> > Perhaps, we can instead fix printargs_merge_tool to take prefix:variable pairs
> > as separate arguments:
> > 
> >  echo "arg: $1:$2"
> >  shift 2
> 
> I can try that, but the glob adjustment is still necessary (the drive letter isn’t the same on all platforms), so this fix is effectively hidden.

Indeed. Queued the patch, thanks.

Patch

diff --git a/tests/test-merge-tools.t b/tests/test-merge-tools.t
--- a/tests/test-merge-tools.t
+++ b/tests/test-merge-tools.t
@@ -1358,7 +1358,7 @@ 
   arg: "ll:working copy"
   arg: "lo:"
   arg: "merge rev"
-  arg: "lb:base: /*/f~base.*" (glob)
+  arg: "lb:base: */f~base.*" (glob)
   0 files updated, 1 files merged, 0 files removed, 0 files unresolved
   (branch merge, don't forget to commit)
   $ rm -f 'printargs_merge_tool'
@@ -1388,7 +1388,7 @@ 
   arg: "ll:working copy: tooltmpl ef83787e2614"
   arg: "lo:"
   arg: "merge rev: tooltmpl 0185f4e0cf02"
-  arg: "lb:base: /*/f~base.*" (glob)
+  arg: "lb:base: */f~base.*" (glob)
   0 files updated, 1 files merged, 0 files removed, 0 files unresolved
   (branch merge, don't forget to commit)
   $ rm -f 'printargs_merge_tool'