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

Re: vimgrep fails when 'autochdir' is set

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