This is my 4th attempt at fixing the cmd command line treatment. All patches pass the cmd tests with 0 errors both under wine and on the Test Bot, including NT. Whitespace issues are fixed, too. Apart from that, the code is the same as before.
The size of my patches has been criticized - only patch 2/5 is really big, but it has to be that way to pass the tests.
This will also be my last attempt to get this merged. If the patches don't get applied this time, I hope that the cmd Gurus (Frederic or Dan) will take up the bits they like and get them in over time. If there are any technical issues or bugs in my code, I'll of course take care (please email me privately, as I'm not monitoring wine-devel).
A private side note: I estimate that I have spent at least 10x more effort and time to get the tests right and the patches submission-ready than I spent on fixing the actual Wine code. I believe this project needs to think over its policies. I fully understand that a project this big needs to take a lot of care to maintain qualitiy standards. But the way this is currently working is discouraging and frustrating for casual contributors like myself.
Martin Wilck (5): cmd/tests: add tests for command line parsing cmd: use GetCommandline() rather than argv cmd: improve parsing of quotes in command line cmd: fix handling of special characters in command line cmd: fix parameter parsing of quotes and special chars
programs/cmd/batch.c | 82 ++++++--- programs/cmd/tests/rsrc.rc | 6 + programs/cmd/tests/test_builtins.cmd.exp | 8 +- programs/cmd/tests/test_cmdline.cmd | 158 +++++++++++++++++ programs/cmd/tests/test_cmdline.cmd.exp | 77 +++++++++ programs/cmd/wcmdmain.c | 276 ++++++++++++------------------ 6 files changed, 406 insertions(+), 201 deletions(-) create mode 100755 programs/cmd/tests/test_cmdline.cmd create mode 100755 programs/cmd/tests/test_cmdline.cmd.exp
On Thu, Sep 29, 2011 at 00:46, Martin Wilck mwilck@arcor.de wrote:
This is my 4th attempt at fixing the cmd command line treatment. All patches pass the cmd tests with 0 errors both under wine and on the Test Bot, including NT. Whitespace issues are fixed, too. Apart from that, the code is the same as before.
The size of my patches has been criticized - only patch 2/5 is really big, but it has to be that way to pass the tests.
It's just that the project leader, Alexandre Julliard, generally likes patch as small as possible. YMMV though, and if it's necessary to put it together, it's ok, just indicate it as patch commit "comment" for AJ, if necessary
This will also be my last attempt to get this merged. If the patches don't get applied this time, I hope that the cmd Gurus (Frederic or Dan) will take up the bits they like and get them in over time. If there are any technical issues or bugs in my code, I'll of course take care (please email me privately, as I'm not monitoring wine-devel).
A private side note: I estimate that I have spent at least 10x more effort and time to get the tests right and the patches submission-ready than I spent on fixing the actual Wine code.
I think that's why Dan pointed that you should probably send a single patch first to avoid reworking the whole series at once, to limit the total amout of effort needed.
I believe this project needs to think over its policies. I fully understand that a project this big needs to take a lot of care to maintain qualitiy standards. But the way this is currently working is discouraging and frustrating for casual contributors like myself.
Yes the "learning curve" is quite steep... I've been bitten by this myself (and cmd test runner wasn't even working properly when I began working on cmd...) BTW if you had issues with the doc regarding patches submissions/testbot (or other things), please tell so we can try to fix that.
Anyway, thanks for your patches. TBH I was a bit "scared" to see/fix how the whole quoting stuff is done, after people told how horrific it was. It's nice not to feel being left "alone in the dark" ;)
Now that the tests pass, you're just one step further on the way towards patch application. Just need to pass through Alexandre's filter ;) (but since there's wineconf this w-e, there won't probably be any commit rounds until next week)
Martin Wilck (5):
Hi Martin,
I reviewed your patches and have found some (minor) issues:
* Your patches email line subjects should have '(try N') appended. I *believe* this is needed for the project leader patches handling scripts You can just sed the "git format-patch" generated patches for instance
* PATCH 1/5 create mode 100755 programs/cmd/tests/test_cmdline.cmd create mode 100755 programs/cmd/tests/test_cmdline.cmd.exp
These files are added as executable. They should be marked as non-executable since they contain/can potentially contain @keyword@s (e.g. @space@, @todo_wine@ which won't be recognized by a native cmd, but need to be processed beforehand). While one might argue that test_cmdline.cmd is executable, test_cmdline.cmd.exp definitely isn't. (this may be due to the fact the files were created on windows)
* PATCH 2/5 - args = argc; + cmdline = WCMD_strdupW(GetCommandLineW()); + if (cmdline == NULL) { + SetLastError(ERROR_OUTOFMEMORY); + exit(1); + }
Using SetLastError before exit(1) is a bit pointless IMHO. You'd better use something like WINE_ERR("Could not allocate memory\n");
* PATCH 5/5 The WCMD_parameter doc should remain in front on WCMD_parameter function
That's all. Great work!
I've also had some weird test failures running "make testclean && make test" for patch 2 batch.c:302: Test succeeded inside todo block: unexpected char 0x0 position -1 in line 18 (got '0 ', wanted '0@space@') batch.c:302: Test failed: unexpected char 0x65 position 0 in line 19 (got 'error 9009', wanted '1@space@') [and some additional messages] Those are probably due to an issue with the testrunner, since the line numbers aren't correct, and the test_cmdline.cmd works nicely when test_builtins.* files are removed. I'll look into that .
Frédéric
2011/9/29 Frédéric Delanoy frederic.delanoy@gmail.com:
I've also had some weird test failures running "make testclean && make test" for patch 2 batch.c:302: Test succeeded inside todo block: unexpected char 0x0 position -1 in line 18 (got '0 ', wanted '0@space@') batch.c:302: Test failed: unexpected char 0x65 position 0 in line 19 (got 'error 9009', wanted '1@space@') [and some additional messages] Those are probably due to an issue with the testrunner, since the line numbers aren't correct, and the test_cmdline.cmd works nicely when test_builtins.* files are removed. I'll look into that .
Apparently a preliminary "make clean" seems to fix that issue. Sorry for the noise. Dan, did buildbot had those issues or does it do a 'make clean'?
Frédéric
2011/9/29 Frédéric Delanoy frederic.delanoy@gmail.com:
Apparently a preliminary "make clean" seems to fix that issue. Sorry for the noise. Dan, did buildbot had those issues or does it do a 'make clean'?
The buildbot always starts with a clean directory, so it won't catch problems like that :-(
Martin Wilck mwilck@arcor.de writes:
This will also be my last attempt to get this merged.
Saying this is the best way to ensure that they won't be merged. Why would people spend time reviewing your code if you've already decided that you are not going to address the issues they find?
This will also be my last attempt to get this merged.
Saying this is the best way to ensure that they won't be merged. Why would people spend time reviewing your code if you've already decided that you are not going to address the issues they find?
I also said that I'd address issues with my code, and I will. Anyway, I was lucky that Dan and Frédéric did review my code again, and so I'll take another shot, contrary to what I said before.
Regards Martin
-
Alexandre Julliard -
Dan Kegel -
Frédéric Delanoy -
Martin Wilck