Re: [PATCH] Fix windows console has("filterpipe"), and 'binary' option handling w.r.t. EOL
- Hi Bram,
>> I've been trying to get the tests running on Windows; the latest roadblocktest11 itself breaks starting with patch 124; it works on patch 123
>> was test11. test11 works correctly using vim.exe from the officially built
>> binary distribution (7.3.48), but not on vim.exe built from the latest
>> source (7.3.496). I finally got to play with 'hg bisect', very fun.
>> This patch does three things, all of them related and necessary:
>> 1. Revert the only not-100%-straightforward change that was in patch 124.
>> Patch 124 was mainly about moving the "did-i-have-an-eol-when-i-loaded"
>> setting from global to per-buffer, but it also changed the point at which
>> the setting was reset. This patch undoes that last part. There was a
>> comment about the line number becoming invalid due to edits, which I didn't
>> follow up on.
> You do not mention what breaks without this change. What is it? Can we
> write a test for it?
and previous. The break doesn't show up as a nonzero diff result.
Instead it just leaves vim sitting in insert mode halfway through the
test, editing a garbled test.out.
In addition to making "did-i-have-an-eol-when-i-loaded" per-buffer
instead of global, patch 124 also moved the line
curbuf->b_no_eol_lnum = 0;
from the end of buf_write() to the end of readfile(). I don't know
what the author's exact intent for this change was; it seems
inconsistent with the rest of the patch, although I admit I'm far from
100% up to speed on the vim codebase.
Keeping it the way it was written (but applying the filterpipe fix)
causes test11 to fail because gzip corrupts the output. Here's the
end of test.out when that happens:
* HEre is a new .c file
* HEre is a new .c file
Reverting the change (keeping all the other per-buffer changes in
patch 124) causes the test to complete successfully, which I assume
means it was interfering with the binary representation of the file
and confusing gzip.
>> 2. Correctly sets has("filterpipe") as "false" on the Windows console,In os_win32.c, the piping code is compiled out of the console version.
>> leaving it enabled in the GUI as intended by patch 240.
>> Almost all the code in patch 240 was guarded with FEAT_GUI_W32; for some
>> reason the change to add has("filterpipe") wasn't. I took a look at
>> generalizing the pipe code to cover the console, but it's very GUI-specific
>> as written. For the time being the console doesn't do pipes.
> This isn't quite right. In ex_cmds.c around line 1117 there is a
> condition that only checks WIN3264, not FEAT_GUI_W32. Thus pipes are
> used on the console as well. It's not clear to me where that happens
> though, perhaps resetting 'shelltemp' breaks filtering?
See mch_system() around line 3861 for the shelltemp check itself,
which is only in the GUI version. The #ifdef that enables all the
pipe-related code is around line 3250, and checks for FEAT_GUI_W32. I
ran it under the debugger, and mch_system() went straight into the
MSCVRT system() call, not the pipe-enabled vim code.
As the code is written it's clear that the console was meant to be excluded:
- There's a full Windows message loop
(PeekMessage/TranslateMessage/DispatchMessage), which couldn't work in
- On pre-W2K systems, piping creates another console window, which
would only be acceptable in the GUI.
- See the comments at the start of mch_system_classic() and
- See the end of mch_call_shell() for more GUI-only #ifdefs and
comments related to piping.
The code in ex_cmds.c:do_filter() sets SHELL_* flags that are consumed
by this GUI-only code; those flags are ignored in the console. It
also adjusts some line numbers, which would NOT be ignored in the
console; these may be the immediate cause of the test failure (see
>> 3. In test11, set shelltemp before filtering through gzip ifYou're right, when I add a FEAT_GUI_W32 check to the WIN3264 check in
>> has("filterpipe") is false.
> This should not be needed, if piping through a filter doesn't work Vim
> should automatically use temp files. Perhaps this is because of the
> problem in do_filter()?
do_filter() like I did in eval.c, the change to test11.in isn't
needed. I have updated the patch (attached).
>> I suspect this patch fixes a number of potential filtering issues on theAgreed. The confusing part is that I assume that other folks ran the
>> Windows console.
> If there are problems that get fixed, we should have a test to verify
tests without issue up to now. It's possible that either my build
choices or my environment exposed the bugs. I build using "nmake -f
Make_mvc.mak FEATURES=HUGE MBYTE=yes". If others can make the
unpatched test11 work using console vim with these settings, I'd want
to dig in to find out what's different between our systems. That
said, I do think the bugs are real; I don't see how gzip filtering
could have worked in the console without these fixes. So, I hesitate
to suggest a test without understanding this discrepancy.
You received this message from the "vim_dev" maillist.
Do not top-post! Type your reply below the text you are replying to.
For more information, visit http://www.vim.org/maillist.php