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

vimgrep fails when 'autochdir' is set

Expand Messages
  • Ben Fritz
    Trying to grep an entire project tree fails with autochdir set: gvim -N -u NONE -i NONE C:/path/to/project/dir/relative/path/in/ project/somedir/file.c set
    Message 1 of 9 , Feb 1 2:29 PM
    • 0 Attachment
      Trying to grep an entire project tree fails with autochdir set:

      gvim -N -u NONE -i NONE C:/path/to/project/dir/relative/path/in/
      project/somedir/file.c
      set autochdir
      cd C:/path/to/project/dir
      vimgrep /pattern/j relative/path/in/project/**/*.[ch]

      Gives:
      C:\path\to\project\dir
      "relative\path\in\project\somedir\file.c" [New DIRECTORY]
      C:\path\to\project\dir
      "relative\path\in\project\somedir\file2.c" [New DIRECTORY]
      C:\path\to\project\dir
      "relative\path\in\project\somedir\file3.c" [New DIRECTORY]
      ...
      E480: No match: pattern

      But without the set autochdir, the vimgrep correctly finds all
      occurrences of pattern in the project.

      I'm about 95% sure this worked at some point in the past, because this
      is how I've done similar searches before.

      I actually found the problem while using :ProjectGrep in eclim, but
      discovered that even with the minimal configuration given above, Vim
      seems to use the wrong directory as the base for its grep search.



      VIM - Vi IMproved 7.3 (2010 Aug 15, compiled Dec 9 2011 16:21:55)
      MS-Windows 32-bit GUI version with OLE support
      Included patches: 1-372
      Compiled by digitectNO@...
      Huge version with GUI. Features included (+) or not (-):

      --
      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
    • Ben Fritz
      ... Actually, it turns out, the issue has been around a long time. I went back to 7.2.160 and the issue exists there as well. This patch fixes it, but probably
      Message 2 of 9 , Feb 2 7:18 PM
      • 0 Attachment
        On Feb 1, 4:29 pm, Ben Fritz <fritzophre...@...> wrote:
        > Trying to grep an entire project tree fails with autochdir set:
        >
        > gvim -N -u NONE -i NONE C:/path/to/project/dir/relative/path/in/
        > project/somedir/file.c
        > set autochdir
        > cd C:/path/to/project/dir
        > vimgrep /pattern/j relative/path/in/project/**/*.[ch]
        >
        > Gives:
        > C:\path\to\project\dir
        > "relative\path\in\project\somedir\file.c" [New DIRECTORY]
        > C:\path\to\project\dir
        > "relative\path\in\project\somedir\file2.c" [New DIRECTORY]
        > C:\path\to\project\dir
        > "relative\path\in\project\somedir\file3.c" [New DIRECTORY]
        > ...
        > E480: No match: pattern
        >
        > But without the set autochdir, the vimgrep correctly finds all
        > occurrences of pattern in the project.
        >
        > I'm about 95% sure this worked at some point in the past, because this
        > is how I've done similar searches before.
        >

        Actually, it turns out, the issue has been around a long time. I went
        back to 7.2.160 and the issue exists there as well.

        This patch fixes it, but probably a better way would be to insert the
        directory restore code in each "dummy_buffer" function. Is there a way
        to detect whether 'cd' or 'lcd' was used, as well? It seems that
        assuming the user used :lcd is an assumption which might bite somebody
        at some point.

        Based off v7.3.393.


        # HG changeset patch
        # User Ben Fritz <fritzophrenic@...>
        # Date 1328238920 21600
        # Branch 2html-dev
        # Node ID 102067eb613dc58082919c594e225411c5c3467d
        # Parent 11151971549e24bab14c759849116f1c60292d90
        fix vimgrep when 'acd' is set

        diff -r 11151971549e -r 102067eb613d src/quickfix.c
        --- a/src/quickfix.c Mon Jan 09 22:19:10 2012 -0600
        +++ b/src/quickfix.c Thu Feb 02 21:15:20 2012 -0600
        @@ -3211,15 +3211,22 @@
        * autocommands applied, etc. */
        buf = load_dummy_buffer(fname);

        - /* When autocommands changed directory: go back. We assume it
        was
        - * ":lcd %:p:h". */
        + /* When autocommands or 'autochdir' option changed directory: go
        + * back. We assume it was ":lcd %:p:h". */
        mch_dirname(dirname_now, MAXPATHL);
        if (STRCMP(dirname_start, dirname_now) != 0)
        {
        exarg_T ea;

        ea.arg = dirname_start;
        - ea.cmdidx = CMD_lcd;
        + if (p_acd)
        + {
        + ea.cmdidx = CMD_cd;
        + }
        + else
        + {
        + ea.cmdidx = CMD_lcd;
        + }
        ex_cd(&ea);
        }

        @@ -3294,6 +3301,25 @@
        * with the same name. */
        wipe_dummy_buffer(buf);
        buf = NULL;
        +
        + /* When autocommands or 'autochdir' option changed
        + * directory: go back. We assume it was ":lcd %:p:h". */
        + mch_dirname(dirname_now, MAXPATHL);
        + if (STRCMP(dirname_start, dirname_now) != 0)
        + {
        + exarg_T ea;
        +
        + ea.arg = dirname_start;
        + if (p_acd)
        + {
        + ea.cmdidx = CMD_cd;
        + }
        + else
        + {
        + ea.cmdidx = CMD_lcd;
        + }
        + ex_cd(&ea);
        + }
        }
        else if (!cmdmod.hide
        || buf->b_p_bh[0] == 'u' /* "unload" */
        @@ -3310,11 +3336,52 @@
        {
        wipe_dummy_buffer(buf);
        buf = NULL;
        +
        + /* When autocommands or 'autochdir' option changed
        + * directory: go back. We assume it was ":lcd %:p:h".
        + */
        + mch_dirname(dirname_now, MAXPATHL);
        + if (STRCMP(dirname_start, dirname_now) != 0)
        + {
        + exarg_T ea;
        +
        + ea.arg = dirname_start;
        + if (p_acd)
        + {
        + ea.cmdidx = CMD_cd;
        + }
        + else
        + {
        + ea.cmdidx = CMD_lcd;
        + }
        + ex_cd(&ea);
        + }
        }
        else if (buf != first_match_buf || (flags & VGR_NOJUMP))
        {
        unload_dummy_buffer(buf);
        buf = NULL;
        +
        + /* When autocommands or 'autochdir' option changed
        + * directory: go back. We assume it was ":lcd %:p:h".
        + */
        + mch_dirname(dirname_now, MAXPATHL);
        + if (STRCMP(dirname_start, dirname_now) != 0)
        + {
        + exarg_T ea;
        +
        + ea.arg = dirname_start;
        + if (p_acd)
        + {
        + ea.cmdidx = CMD_cd;
        + }
        + else
        + {
        + ea.cmdidx = CMD_lcd;
        + }
        + ex_cd(&ea);
        + }
        +
        }
        }

        --
        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
      • Bram Moolenaar
        ... This repeats the same code several times. Please move it to a function. Instead of guessing whether :lcd or :cd was used it would be better to check the
        Message 3 of 9 , Feb 3 8:47 PM
        • 0 Attachment
          Ben Fritz wrote:

          > On Feb 1, 4:29 pm, Ben Fritz <fritzophre...@...> wrote:
          > > Trying to grep an entire project tree fails with autochdir set:
          > >
          > > gvim -N -u NONE -i NONE C:/path/to/project/dir/relative/path/in/
          > > project/somedir/file.c
          > > set autochdir
          > > cd C:/path/to/project/dir
          > > vimgrep /pattern/j relative/path/in/project/**/*.[ch]
          > >
          > > Gives:
          > > C:\path\to\project\dir
          > > "relative\path\in\project\somedir\file.c" [New DIRECTORY]
          > > C:\path\to\project\dir
          > > "relative\path\in\project\somedir\file2.c" [New DIRECTORY]
          > > C:\path\to\project\dir
          > > "relative\path\in\project\somedir\file3.c" [New DIRECTORY]
          > > ...
          > > E480: No match: pattern
          > >
          > > But without the set autochdir, the vimgrep correctly finds all
          > > occurrences of pattern in the project.
          > >
          > > I'm about 95% sure this worked at some point in the past, because this
          > > is how I've done similar searches before.
          > >
          >
          > Actually, it turns out, the issue has been around a long time. I went
          > back to 7.2.160 and the issue exists there as well.
          >
          > This patch fixes it, but probably a better way would be to insert the
          > directory restore code in each "dummy_buffer" function. Is there a way
          > to detect whether 'cd' or 'lcd' was used, as well? It seems that
          > assuming the user used :lcd is an assumption which might bite somebody
          > at some point.
          >
          > Based off v7.3.393.

          This repeats the same code several times. Please move it to a
          function.

          Instead of guessing whether :lcd or :cd was used it would be better to
          check the actual situation. Probably by checking the value of
          curwin->w_localdir.


          --
          How To Keep A Healthy Level Of Insanity:
          9. As often as possible, skip rather than walk.

          /// Bram Moolenaar -- Bram@... -- http://www.Moolenaar.net \\\
          /// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
          \\\ an exciting new programming language -- http://www.Zimbu.org ///
          \\\ help me help AIDS victims -- http://ICCF-Holland.org ///

          --
          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
        • John Little
          Ben Is there a reason you don t use the ?: operator? if (p_acd) { ea.cmdidx = CMD_cd; } else { ea.cmdidx = CMD_lcd; } seems more vim-C-ish written as ea.cmdidx
          Message 4 of 9 , Feb 4 1:21 AM
          • 0 Attachment
            Ben

            Is there a reason you don't use the ?: operator?

            if (p_acd)
            {
            ea.cmdidx = CMD_cd;
            }
            else
            {
            ea.cmdidx = CMD_lcd;
            }

            seems more vim-C-ish written as

            ea.cmdidx = p_acd ? CMD_cd : CMD_lcd;

            to me, anyway.

            Regards, John

            --
            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
          • Benjamin Fritz
            Thanks, everyone. The curwin- w_localdir variable was exactly what I was looking for, and the tertiary operator makes it a little clearer what is going on in
            Message 5 of 9 , Feb 4 8:29 PM
            • 0 Attachment
              Thanks, everyone. The curwin->w_localdir variable was exactly what I
              was looking for, and the tertiary operator makes it a little clearer
              what is going on in the directory restore code.

              I took your suggestions and additionally moved the directory restore
              calls into the ((un)?load|wipe)_dummy_buffer functions so that it is
              easier to maintain in the future, if these functions are used
              elsewhere. The root cause of the problem was that many of the
              functions from buffer.c called by these dummy_buffer functions were
              using the DO_AUTOCHDIR macro, though presumably an appropriately
              defined autocmd would do the same thing, so whenever we touch the
              dummy buffer we need to restore the directory. Presumably creating a
              "dummy buffer" should never have lasting effects on the current
              directory!

              Updated patch attached to fix that 'autochdir' causes :vimgrep to
              fail, based on 7.3.421. I did have one more question: I noticed that
              the pre-existing code dynamically allocates memory of static size
              MAXPATHL for both dirname_start and dirname_now. I have kept this and
              done the same thing for the restore_start_dir function I added, but I
              wonder, is there a reason the path string cannot be stack-allocated
              instead? Is it to limit stack size or something, since MAXPATHL can be
              somewhat large?

              --
              Ben

              --
              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
            • Bram Moolenaar
              ... Thanks, I ll put the patch in the todo list. ... As a rule of thumb: A function should not put more than about 1000 bytes on the stack. Les if it used
              Message 6 of 9 , Feb 4 9:06 PM
              • 0 Attachment
                Ben Fritz wrote:

                > Thanks, everyone. The curwin->w_localdir variable was exactly what I
                > was looking for, and the tertiary operator makes it a little clearer
                > what is going on in the directory restore code.
                >
                > I took your suggestions and additionally moved the directory restore
                > calls into the ((un)?load|wipe)_dummy_buffer functions so that it is
                > easier to maintain in the future, if these functions are used
                > elsewhere. The root cause of the problem was that many of the
                > functions from buffer.c called by these dummy_buffer functions were
                > using the DO_AUTOCHDIR macro, though presumably an appropriately
                > defined autocmd would do the same thing, so whenever we touch the
                > dummy buffer we need to restore the directory. Presumably creating a
                > "dummy buffer" should never have lasting effects on the current
                > directory!

                Thanks, I'll put the patch in the todo list.

                > Updated patch attached to fix that 'autochdir' causes :vimgrep to
                > fail, based on 7.3.421. I did have one more question: I noticed that
                > the pre-existing code dynamically allocates memory of static size
                > MAXPATHL for both dirname_start and dirname_now. I have kept this and
                > done the same thing for the restore_start_dir function I added, but I
                > wonder, is there a reason the path string cannot be stack-allocated
                > instead? Is it to limit stack size or something, since MAXPATHL can be
                > somewhat large?

                As a rule of thumb: A function should not put more than about 1000 bytes
                on the stack. Les if it used recursively. That's because we can
                gracefully handle out-of-memory allocations but not out-of-stack errors,
                these cause a crash.

                --
                Apparently, 1 in 5 people in the world are Chinese. And there are 5
                people in my family, so it must be one of them. It's either my mum
                or my dad. Or my older brother Colin. Or my younger brother
                Ho-Cha-Chu. But I think it's Colin.

                /// Bram Moolenaar -- Bram@... -- http://www.Moolenaar.net \\\
                /// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
                \\\ an exciting new programming language -- http://www.Zimbu.org ///
                \\\ help me help AIDS victims -- http://ICCF-Holland.org ///

                --
                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
              • Bram Moolenaar
                ... Running out of stack space usually causes a nasty crash. Can t even save the swap file then, unless the system supports a signal stack. Therefore Vim
                Message 7 of 9 , Apr 25, 2012
                • 0 Attachment
                  Benjamin Fritz wrote in February:

                  > Updated patch attached to fix that 'autochdir' causes :vimgrep to
                  > fail, based on 7.3.421. I did have one more question: I noticed that
                  > the pre-existing code dynamically allocates memory of static size
                  > MAXPATHL for both dirname_start and dirname_now. I have kept this and
                  > done the same thing for the restore_start_dir function I added, but I
                  > wonder, is there a reason the path string cannot be stack-allocated
                  > instead? Is it to limit stack size or something, since MAXPATHL can be
                  > somewhat large?

                  Running out of stack space usually causes a nasty crash. Can't even
                  save the swap file then, unless the system supports a signal stack.
                  Therefore Vim avoids putting large buffers on the stack. MAXPATHL is
                  often 4000 or 8000 bytes, which is quite a lot. Especially when doing
                  it in many places.

                  --
                  MONK: ... and the Lord spake, saying, "First shalt thou take out the Holy Pin,
                  then shalt thou count to three, no more, no less. Three shalt be the
                  number thou shalt count, and the number of the counting shalt be three.
                  Four shalt thou not count, neither count thou two, excepting that thou
                  then proceed to three. Five is right out. Once the number three, being
                  the third number, be reached, then lobbest thou thy Holy Hand Grenade of
                  Antioch towards thou foe, who being naughty in my sight, shall snuff it.
                  "Monty Python and the Holy Grail" PYTHON (MONTY) PICTURES LTD

                  /// Bram Moolenaar -- Bram@... -- http://www.Moolenaar.net \\\
                  /// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
                  \\\ an exciting new programming language -- http://www.Zimbu.org ///
                  \\\ help me help AIDS victims -- http://ICCF-Holland.org ///

                  --
                  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
                • Ben Fritz
                  ... As I mentioned, the patch uses the existing method of dynamically allocating MAXPATHL bytes instead of putting it on the stack; I decided it was probably
                  Message 8 of 9 , Apr 25, 2012
                  • 0 Attachment
                    On Wednesday, April 25, 2012 11:57:34 AM UTC-5, Bram Moolenaar wrote:
                    > Benjamin Fritz wrote in February:
                    >
                    > > Updated patch attached to fix that 'autochdir' causes :vimgrep to
                    > > fail, based on 7.3.421. I did have one more question: I noticed that
                    > > the pre-existing code dynamically allocates memory of static size
                    > > MAXPATHL for both dirname_start and dirname_now. I have kept this and
                    > > done the same thing for the restore_start_dir function I added, but I
                    > > wonder, is there a reason the path string cannot be stack-allocated
                    > > instead? Is it to limit stack size or something, since MAXPATHL can be
                    > > somewhat large?
                    >
                    > Running out of stack space usually causes a nasty crash. Can't even
                    > save the swap file then, unless the system supports a signal stack.
                    > Therefore Vim avoids putting large buffers on the stack. MAXPATHL is
                    > often 4000 or 8000 bytes, which is quite a lot. Especially when doing
                    > it in many places.
                    >

                    Yes, thanks for the clarification. You said earlier:

                    > As a rule of thumb: A function should not put more than about 1000 bytes
                    > on the stack. Les if it used recursively. That's because we can
                    > gracefully handle out-of-memory allocations but not out-of-stack errors,
                    > these cause a crash.

                    As I mentioned, the patch uses the existing method of dynamically allocating MAXPATHL bytes instead of putting it on the stack; I decided it was probably done for a reason, I just wasn't sure of the reason. I was asking only for the sake of better understanding of the Vim code. This isn't holding up the patch, is it?

                    --
                    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
                  • Ben Fritz
                    ... I just saw patch 509, never mind my premature question :-) -- You received this message from the vim_dev maillist. Do not top-post! Type your reply below
                    Message 9 of 9 , Apr 25, 2012
                    • 0 Attachment
                      On Wednesday, April 25, 2012 12:21:06 PM UTC-5, Ben Fritz wrote:
                      >
                      > As I mentioned, the patch uses the existing method of dynamically allocating
                      > MAXPATHL bytes instead of putting it on the stack; I decided it was probably
                      > done for a reason, I just wasn't sure of the reason. I was asking only for
                      > the sake of better understanding of the Vim code. This isn't holding up the
                      > patch, is it?

                      I just saw patch 509, never mind my premature question :-)

                      --
                      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.