Loading ...
Sorry, an error occurred while loading the content.

Re: [PATCH] Fix windows console has("filterpipe"), and 'binary' option handling w.r.t. EOL

Expand Messages
  • David Pope
    Hi Bram, ... test11 itself breaks starting with patch 124; it works on patch 123 and previous. The break doesn t show up as a nonzero diff result. Instead it
    Message 1 of 5 , Apr 22, 2012
    • 0 Attachment
      Hi Bram,

      >> I've been trying to get the tests running on Windows; the latest roadblock
      >> 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?

      test11 itself breaks starting with patch 124; it works on patch 123
      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,
      >> 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?

      In os_win32.c, the piping code is compiled out of the console version.
      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
      the console.
      - 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 if
      >> 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()?

      You're right, when I add a FEAT_GUI_W32 check to the WIN3264 check in
      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 the
      >> Windows console.
      > If there are problems that get fixed, we should have a test to verify
      > that.

      Agreed. The confusing part is that I assume that other folks ran the
      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.

      -- Dave

      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
    Your message has been successfully submitted and would be delivered to recipients shortly.