Alright! You know where to find me or this patch if interest arises :)

@epriestley is the best course of action for this diff to abandon it?

@chad - How can I be involved with any progress on this front then since now it is planned?

I wasn't trying to create a whole new discussion. I respect your priorities and I also understand the engineers vs. money problem. The only major problem I see with the contribution guidelines is this: to be able to contribute more or affect change, you need clout and to get clout you need to contribute and for those contributions to go into the code or even get some feedback we need time from you guys.

@chad - You know how can you get more developers? Being more open to contributions :)

rebase

rebase

rebase

rebase

rebase

rebase

rebase

rebase

rebase

I'd love to get this merged or concluded instead of trying to keep up with rebases and push it forward. I have been using this branch for quite a while locally and as far as I can tell, there are no problems.

rebase

@eadler - oops, sorry about that then :) I'm still behind what I said though ;)

@eadler - I'd like to remind you that there are 3 patches waiting for review right now. They have been sitting there for a few months. That said we are not paying Phabricator. If the only way to get these patches which are already worked on and in pretty good shape merged in is to pay you or maintain a fork I'm really starting to think about crowdsourcing this on Kickstarter or Indiegogo to collect those few hundred bucks to get already working patches landed or simply create a fork and point people there loudly. I also think I almost qualify for "Rise to prominence" sub-section mentioned in https://secure.phabricator.com/book/phabcontrib/article/contributing_code/#alternatives but of course that also relies on you guys letting me contribute more. Making patches sit for months without any feedback and then making me rebase them multiple times is definitely not a great way to encourage people to "Rise to prominence" if you really value that.

• Feedback

I've decided to just drop the CMD.exe support since it is crazy and unreliable when it comes to escaping edge cases. Also it needs double escaping anyways (ArgV + Crazy-ass CMD escaping).

• Remove CMD.exe support and fix edge cases

If there's no way to escape it, we should make it throw, and have a test case to make sure it throws. It's OK if we don't handle it, I just want to have no cases where we silently do the wrong thing.

@epriestley ping

rebase

@epriestley if you are willing to give this another shot I'll try to fix the test error. Otherwise all my hard work seems to be going into a blackhole.

rebase

rebase

rebase

Bump

Bump

(I'm looking forward to reviewing this but probably won't get to it for at least a few days since I want to test it thoroughly myself.)

The flag was initially added to minimize the impact of fixing the broken streaming that Windows has by default. Without this flag on, you can't actually stream data from a process's standard output to PHP in realtime; you have to wait for the output to be fully buffered.

Anything I can do to move this one forward?

lgtm (but I haven't tested it)

• Actually add comment about taskkill change
• Safer PHP invocation for edge case test

@epriestley, @hach-que - I think this is in a very good state for a review now. The only thing I'm unsure is whether to enable bypass_shell by default since I don't see a reason to use the cmd.exe proxy. Its commands are not universal and it requires another layer of escaping which is confusing and probably prone to errors, especially when things get nested.

• Rebase
• Add comment about taskkill change
• Slightly better php invocations in tests
• Enable one more test for Windows

rebase/bump

Okay, the patch was good. It was arc trying to overcompensate for Windows mania so another patch to arc is needed and it is on its way.

I've noticed an issue with passthru mode. I'll investigate and update the patch accordingly.

I guess the author of the article I referenced is @dancol ?

• Fix broken test case
• Fix Windows escaping once and for all
• Fix test failures
• Add test for edge cases

@epriestley - great test cases. I've already found a great article on MSDN which explains the escaping rules on Windows, both for CreateProcess and CMD.exe and implemented them. They seem to work well. I'm working fixing somewhat unrelated test failures due to stdin pipe getting clogged. I'll add these strings as test cases. Good news is, based on the information I got from the article, there doesn't seem any unescapable inputs.

@hach-que - Okay for the sake of progress I'm gonna leave the specifics of the argument or how these are parsed. I think you basically want me to come up with a "proper CMD escape" (has nothing to do with CreateProcess) instead of reusing PowerShell escape since that is not the correct way and are not asking me to make all instantiations of this class Windows-aware.

But not everything is running through Powershell? When Arcanist runs (which is using this function), it escapes for the Command Prompt, not Powershell.

@hach-que - I strongly disagree since with that solution every single instance of the class needs to to this exact same thing. It doesn't make any sense to me. There's no point using the built in escaping function on Windows because it is broken beyond repair.

• Fix test failures

This should fix T10468.

• Fix broken test case

This is another reason for me to dislike PHP but I really appreciate the time you've taken to explain the issue in addition to a promising fix for this task. Couldn't have asked for a better new year's gift :)

Abandoning per T9724#151731

Oh wow, sorry. I just thought my system was broken and was hoping CI would
run tests. Will see if I can fix this.

If so, I'd generally claim this is an issue with the client you're using: I think clients generally should not do parameter validation themselves. They will always be less able to do validation than the server is, even though server validation is not especially sophisticated right now. If/when we have a formal Python API client, it is very unlikely to perform client validation. Does client validation provide any advantage?

I'm fairly certain since it seems to be using the CreateProcess API as others do. I'm also using xonsh (the project using that Python code) for quite some time now and it works without any issues.

@epriestley - okay, may be the name mislead me. Anyways, I'm basically proposing making bypass_shell on by default in execx and implementing a mechanism that automatically finds the full path of the executable on Windows by implementing that Python function I referred to. Does this make sense?

How does that obsolete PhutilExecPassthru?

Okay, proposal: how about we implement the equivalent of this: https://github.com/scopatz/xonsh/blob/fa356d728b2852274f8aa0260dc7da3b7b0f08ab/xonsh/built_ins.py#L268

Oh god, and I thought Python's subprocess module was pretty bad.

bypass_shell (windows only): bypass cmd.exe shell when set to TRUE
5.2.1 Added the bypass_shell option to the other_options parameter.

Oh, thanks a lot for the pointers. I'll see how I can help with those.

• Fix some tests

This is a "does this really work?" sort of diff. Will make it proper after some feedback. Thanks! :)

Thank you guys for the pointer and quick turn around!

• Do not cache workflow

@epriestley - so are you saying I should simply revert rARC0e0957872083daa38309204ad9774568c9fcdd5c? I mean that looks like a nice patch and given all the context you and @avivey shared (thanks a ton btw for that), I may be able to prevent caching "properly".

@avivey - When arc patch runs itself recursively. The thing I realized is arc patch first tries to get the patch without using credentials (but it still requires a conduit), and then when it realizes the request is rejected due to missing credentials, it manually adds them by calling authenticateConduit. After that it calls run, which in turn calls buildChildWorkflow. Since the parent buildChildWorkflow doesn't know about the already established conduit which has its credentials already set ($workflow->conduit being set) it simply tries to reset it causing $workflow->setConduitCredentials($this->conduitCredentials); call to throw the You may not set new credentials after authenticating conduit. error since credentials were already set manually by arc patch.

Ping?

• don't skip lint and unit