vimgrep fails when 'autochdir' is set

[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 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 digit...@SPAMdancingpaper.com
Huge version with GUI. Features included (+) or not (-):

[Ben Fritz]

On Feb 1, 4:29 pm, Ben Fritz <fritzophre...@gmail.com> 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 <fritzo...@gmail.com>
# 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);
+ }
+
}
}

[Bram Moolenaar]

Ben Fritz wrote:

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 -- Br...@Moolenaar.net -- 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 ///

[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 = p_acd ? CMD_cd : CMD_lcd;

to me, anyway.

Regards, John

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

[Bram Moolenaar]

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]

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

[Ben Fritz]

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?

[Ben Fritz]

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 :-)