Re: Review Request 58126: Windows: Stout: Reimplemented `stringify_args`.

[Joseph Wu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58126/#review170947
---


Ship it!




I dislike the fact we need to do this sort of escaping, but the code LGTM.


3rdparty/stout/include/stout/os/windows/shell.hpp
Lines 185-186 (original), 185-213 (patched)


So in English...

1) Any argument with a space, tab, newline, vertical tab, or quote must be 
double-quoted (").
2) Backslashes at the very end of an argument must be escaped.
3) Backslashes that precede a double-quote must be escaped.  The 
double-quote must also be escaped.

(Maybe add this as a comment.)



3rdparty/stout/include/stout/os/windows/shell.hpp
Lines 201 (patched)


s/metait/metacharacter/ ?

How did that typo come about :D ?



3rdparty/stout/include/stout/os/windows/shell.hpp
Lines 223 (patched)


s/it/character/


- Joseph Wu


On March 31, 2017, 4:49 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58126/
> ---
> 
> (Updated March 31, 2017, 4:49 p.m.)
> 
> 
> Review request for mesos, Jeff Coffler, John Kordich, Joseph Wu, Li Li, and 
> Michael Park.
> 
> 
> Bugs: MESOS-5418 and MESOS-5522
> https://issues.apache.org/jira/browse/MESOS-5418
> https://issues.apache.org/jira/browse/MESOS-5522
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This was an unused function that ended up being the correct place to
> implement proper `argv` concatenation and escaping.  It returns a
> `std::wstring` for use (speifically) by `::CreateProcessW`.  This brings
> us a bit closer to Unicode support within Mesos.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/windows/shell.hpp 
> fdce93c2146ddec6117577b538dca77c416e0c01 
> 
> 
> Diff: https://reviews.apache.org/r/58126/diff/1/
> 
> 
> Testing
> ---
> 
> Testing done later in chain.
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 58126: Windows: Stout: Reimplemented `stringify_args`.

[Andrew Schwartzmeyer]

> On April 1, 2017, 12:04 a.m., Jeff Coffler wrote:
> > 3rdparty/stout/include/stout/os/windows/shell.hpp
> > Line 179 (original), 179 (patched)
> > 
> >
> > Wow. I'd really like to see unit tests on that, so that we can make 
> > sure it works in cases we care about, and so that if we find special cases 
> > down the road, we can make sure we don't break existing cases.
> > 
> > Can you please add a unit test to call this with some basic tests as 
> > well as the specific cases that you found to fail?

Yeah, I like unit tests :) It'll just be once I've finished fixing 
`HealthCheckTest.HealthyTaskNonShell`, 
`CombinedAuthenticatorTest.MultipleAuthenticators`, and will come in a batch 
with those test fixes.


- Andrew


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58126/#review170804
---


On March 31, 2017, 11:49 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58126/
> ---
> 
> (Updated March 31, 2017, 11:49 p.m.)
> 
> 
> Review request for mesos, Jeff Coffler, John Kordich, Joseph Wu, Li Li, and 
> Michael Park.
> 
> 
> Bugs: MESOS-5418 and MESOS-5522
> https://issues.apache.org/jira/browse/MESOS-5418
> https://issues.apache.org/jira/browse/MESOS-5522
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This was an unused function that ended up being the correct place to
> implement proper `argv` concatenation and escaping.  It returns a
> `std::wstring` for use (speifically) by `::CreateProcessW`.  This brings
> us a bit closer to Unicode support within Mesos.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/windows/shell.hpp 
> fdce93c2146ddec6117577b538dca77c416e0c01 
> 
> 
> Diff: https://reviews.apache.org/r/58126/diff/1/
> 
> 
> Testing
> ---
> 
> Testing done later in chain.
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 58126: Windows: Stout: Reimplemented `stringify_args`.

[Jeff Coffler]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58126/#review170804
---




3rdparty/stout/include/stout/os/windows/shell.hpp
Line 179 (original), 179 (patched)


Wow. I'd really like to see unit tests on that, so that we can make sure it 
works in cases we care about, and so that if we find special cases down the 
road, we can make sure we don't break existing cases.

Can you please add a unit test to call this with some basic tests as well 
as the specific cases that you found to fail?


- Jeff Coffler


On March 31, 2017, 11:49 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58126/
> ---
> 
> (Updated March 31, 2017, 11:49 p.m.)
> 
> 
> Review request for mesos, Jeff Coffler, John Kordich, Joseph Wu, Li Li, and 
> Michael Park.
> 
> 
> Bugs: MESOS-5418 and MESOS-5522
> https://issues.apache.org/jira/browse/MESOS-5418
> https://issues.apache.org/jira/browse/MESOS-5522
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This was an unused function that ended up being the correct place to
> implement proper `argv` concatenation and escaping.  It returns a
> `std::wstring` for use (speifically) by `::CreateProcessW`.  This brings
> us a bit closer to Unicode support within Mesos.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/windows/shell.hpp 
> fdce93c2146ddec6117577b538dca77c416e0c01 
> 
> 
> Diff: https://reviews.apache.org/r/58126/diff/1/
> 
> 
> Testing
> ---
> 
> Testing done later in chain.
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Review Request 58126: Windows: Stout: Reimplemented `stringify_args`.

[Andrew Schwartzmeyer]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58126/
---

Review request for mesos, Jeff Coffler, John Kordich, Joseph Wu, Li Li, and 
Michael Park.


Bugs: MESOS-5418 and MESOS-5522
https://issues.apache.org/jira/browse/MESOS-5418
https://issues.apache.org/jira/browse/MESOS-5522


Repository: mesos


Description
---

This was an unused function that ended up being the correct place to
implement proper `argv` concatenation and escaping.  It returns a
`std::wstring` for use (speifically) by `::CreateProcessW`.  This brings
us a bit closer to Unicode support within Mesos.


Diffs
-

  3rdparty/stout/include/stout/os/windows/shell.hpp 
fdce93c2146ddec6117577b538dca77c416e0c01 


Diff: https://reviews.apache.org/r/58126/diff/1/


Testing
---

Testing done later in chain.


Thanks,

Andrew Schwartzmeyer

Re: Review Request 58126: Windows: Stout: Reimplemented `stringify_args`.

[Andrew Schwartzmeyer]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58126/#review170800
---




3rdparty/stout/include/stout/os/windows/shell.hpp
Lines 186-187 (original), 213-214 (patched)


What the? There's a tab here :/


- Andrew Schwartzmeyer


On March 31, 2017, 11:49 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58126/
> ---
> 
> (Updated March 31, 2017, 11:49 p.m.)
> 
> 
> Review request for mesos, Jeff Coffler, John Kordich, Joseph Wu, Li Li, and 
> Michael Park.
> 
> 
> Bugs: MESOS-5418 and MESOS-5522
> https://issues.apache.org/jira/browse/MESOS-5418
> https://issues.apache.org/jira/browse/MESOS-5522
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This was an unused function that ended up being the correct place to
> implement proper `argv` concatenation and escaping.  It returns a
> `std::wstring` for use (speifically) by `::CreateProcessW`.  This brings
> us a bit closer to Unicode support within Mesos.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/windows/shell.hpp 
> fdce93c2146ddec6117577b538dca77c416e0c01 
> 
> 
> Diff: https://reviews.apache.org/r/58126/diff/1/
> 
> 
> Testing
> ---
> 
> Testing done later in chain.
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>