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

Re: vimgrep fails when 'autochdir' is set

Expand Messages
  • 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 1 of 9 , Feb 2, 2012
      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 2 of 9 , Feb 3, 2012
        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 3 of 9 , Feb 4, 2012
          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 4 of 9 , Feb 4, 2012
            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 5 of 9 , Feb 4, 2012
              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 6 of 9 , Apr 25, 2012
                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 7 of 9 , Apr 25, 2012
                  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 8 of 9 , Apr 25, 2012
                    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.