[bug] error E201 when opening 3 large files with -o option when using LargeFile plugin

[Dominique Pellé]

Hi

I'm using Charle's plugin for large files, version 5 (2013-11-25)
available at:

http://www.vim.org/scripts/script.php?script_id=1506

The plugin is causing the following error when trying to
open 3 large files with the -o or -O options:

E201: *ReadPre autocommands must not change current buffer"

Steps to reproduce:

1) create 3 large files:

$ yes 111 | head -10000000 > foo1
$ yes 222 | head -10000000 > foo2
$ yes 333 | head -10000000 > foo3

2) open the 3 files with Vim, with LargeFile plugin installed, as follows:

$ vim -o foo1 foo2 foo3

3) notice the error message:

E201: *ReadPre autocommands must not change current buffer
Press ENTER or type command to continue

After pressing ENTER, notice that file foo1 is loaded in top
and bottom window, and middle window is empty. I would
expect instead to see foo1 in top window, foo2 in middle
window and foo3 in bottom window.

I noticed the bug using vim-7.4.979 (huge) on Linux x86_64.
Bug does not happen with Vim that comes with xubuntu-14.04 (7.4.52).

Doing a bisection in git:

7.4.072 --> OK (no error)
7.4.073 --> Error E201

So the regression is introduced by this patch:

commit f5a2fd880ae8f6225814209ab73783f65078a4d5
Author: Bram Moolenaar <Br...@vim.org>
Date: Wed Nov 6 05:26:15 2013 +0100

updated for version 7.4.073
Problem: Setting undolevels for one buffer changes undo in another.
Solution: Make 'undolevels' a global-local option. (Christian Brabandt)

Regards
Dominique

[Christian Brabandt]

Hm, this seems to be caused by the NoMatchParen command, that iterates
over all windows (but does not reset the cursor to the correct window
position) and disable parenthesis highlighting.

I think, the matchparen plugin needs to be fixed:

--- matchparen.vim.orig 2015-12-22 13:54:06.165428118 +0100
+++ matchparen.vim.new 2015-12-22 13:56:30.760946989 +0100
@@ -181,9 +181,9 @@
endfunction

" Define commands that will disable and enable the plugin.
-command! NoMatchParen windo silent! call matchdelete(3) | unlet! g:loaded_matchparen |
- \ au! matchparen
-command! DoMatchParen runtime plugin/matchparen.vim | windo doau CursorMoved
+command! NoMatchParen let a=winnr() | windo silent! call matchdelete(3) | unlet! g:loaded_matchparen |
+ \ exe a. "wincmd w" | au! matchparen
+command! DoMatchParen runtime plugin/matchparen.vim | let a=winnr() | windo doau CursorMoved | exe a. "wincmd w"

let &cpo = s:cpo_save
unlet s:cpo_save


Perhaps we should add a windo! command, that does restore cursor positioning (also for the other ..do commands)?

Best,
Christian
--
Man hat den Epikur, der ein armer Hund war wie ich, sehr
missverstanden, wenn er das Höchste in die Schmerzlosigkeit legte.
-- Goethe, Maximen und Reflektionen, Nr. 1290

[Ben Fritz]

Already discussed here: https://groups.google.com/d/topic/vim_dev/P8EyOpmNj5A/discussion

Supposedly the latest unreleased version of LargeFile fixes it for LargeFile. I don't remember if I ever tried it. Regardless I'd support NoMatchParen restoring current window. But what about the alternate window as somebody pointed out in that other thread?

[Christian Brabandt]

On Di, 22 Dez 2015, Ben Fritz wrote:

> Already discussed here:
> https://groups.google.com/d/topic/vim_dev/P8EyOpmNj5A/discussion

Thanks for pointing that out.

> Supposedly the latest unreleased version of LargeFile fixes it for
> LargeFile. I don't remember if I ever tried it. Regardless I'd support
> NoMatchParen restoring current window. But what about the alternate
> window as somebody pointed out in that other thread?

Here is a test patch, that works for :tabdo! and :windo!
I did not invent a solution for :bufdo/cfdo/cdo, since those commands
already support the '!' and personally, it does not seem to be a problem
for those commands.

Interestingly, :windo! is already supported. I don't know why or what it
is supposed to do differently from :windo

diff --git a/src/ex_cmds.h b/src/ex_cmds.h
--- a/src/ex_cmds.h
+++ b/src/ex_cmds.h
@@ -1397,7 +1397,7 @@ EX(CMD_tabclose, "tabclose", ex_tabclose
RANGE|NOTADR|COUNT|BANG|TRLBAR|CMDWIN,
ADDR_TABS),
EX(CMD_tabdo, "tabdo", ex_listdo,
- NEEDARG|EXTRA|NOTRLCOM|RANGE|NOTADR|DFLALL,
+ BANG|NEEDARG|EXTRA|NOTRLCOM|RANGE|NOTADR|DFLALL,
ADDR_TABS),
EX(CMD_tabedit, "tabedit", ex_splitview,
BANG|FILE1|RANGE|NOTADR|ZEROR|EDITCMD|ARGOPT|TRLBAR,
diff --git a/src/ex_cmds2.c b/src/ex_cmds2.c
--- a/src/ex_cmds2.c
+++ b/src/ex_cmds2.c
@@ -2438,7 +2438,11 @@ ex_listdo(eap)
int i;
#ifdef FEAT_WINDOWS
win_T *wp;
+ win_T *oldwp = curwin;
+ win_T *oldprev = prevwin;
tabpage_T *tp;
+ tabpage_T *oldtp = curtab;
+ win_T *oldtpprev = curtab->tp_prevwin;
#endif
buf_T *buf = curbuf;
int next_fnum = 0;
@@ -2660,6 +2664,15 @@ ex_listdo(eap)
}
listcmd_busy = FALSE;
}
+#ifdef FEAT_WINDOWS
+ if ((eap->cmdidx == CMD_windo || eap->cmdidx == CMD_tabdo) && eap->forceit)
+ {
+ curwin = oldwp;
+ prevwin = oldprev;
+ curtab = oldtp;
+ curtab->tp_prevwin = oldtpprev;
+ }
+#endif


Best,
Christian
--
Ein geistreicher Humorist als quasi Poet, der, der Fülle seines
Wissens und Empfindens gedenkend, sich in Tropen auszusprechen
genötigt fühlt.
-- Goethe, Maximen und Reflektionen, Nr. 1314

[Bram Moolenaar]

Christian Brabandt wrote:

> On Di, 22 Dez 2015, Ben Fritz wrote:
>
> > Already discussed here:
> > https://groups.google.com/d/topic/vim_dev/P8EyOpmNj5A/discussion
>
> Thanks for pointing that out.
>
> > Supposedly the latest unreleased version of LargeFile fixes it for
> > LargeFile. I don't remember if I ever tried it. Regardless I'd support
> > NoMatchParen restoring current window. But what about the alternate
> > window as somebody pointed out in that other thread?
>
> Here is a test patch, that works for :tabdo! and :windo!
> I did not invent a solution for :bufdo/cfdo/cdo, since those commands
> already support the '!' and personally, it does not seem to be a problem
> for those commands.
>
> Interestingly, :windo! is already supported. I don't know why or what it
> is supposed to do differently from :windo

Hmm, I think it's confusing that adding ! means something different for
:windo and :bufdo. A separate argument would be better.
That's more work, but I'm sure you can handle it :-).


--
Not too long ago, a program was something you watched on TV...

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

[Christian Brabandt]

Hi Bram!

On Di, 22 Dez 2015, Bram Moolenaar wrote:

> Hmm, I think it's confusing that adding ! means something different for
> :windo and :bufdo. A separate argument would be better.
> That's more work, but I'm sure you can handle it :-).

You mean like windo restoreview command?

Best,
Christian
--
Kleine Mädchen scheinen am leichtesten gut erzogen, weil ihre Natur
nicht heftig, sondern immer furchtsam ist und also jeden Schein der
Erziehung leichter nachspiegelt.
-- Jean Paul

[Bram Moolenaar]

Christian Brabandt wrote:

> On Di, 22 Dez 2015, Bram Moolenaar wrote:
>
> > Hmm, I think it's confusing that adding ! means something different for
> > :windo and :bufdo. A separate argument would be better.
> > That's more work, but I'm sure you can handle it :-).
>
> You mean like windo restoreview command?

Well, how do we add an argument to a command, in a way that it's simple
and consistent? Perhaps <restore> ? We don't have dashed arguments,
we do have <buffer> for :map. It's less likely to clash with a valid
argument at least. And we have arguments like <sfile>, <abuf>, etc.

--
PRINCE: He's come to rescue me, father.
LAUNCELOT: (embarrassed) Well, let's not jump to conclusions ...
"Monty Python and the Holy Grail" PYTHON (MONTY) PICTURES LTD

[Christian Brabandt]

On Sa, 26 Dez 2015, Bram Moolenaar wrote:

> Christian Brabandt wrote:
>
> > On Di, 22 Dez 2015, Bram Moolenaar wrote:
> >
> > > Hmm, I think it's confusing that adding ! means something different for
> > > :windo and :bufdo. A separate argument would be better.
> > > That's more work, but I'm sure you can handle it :-).
> >
> > You mean like windo restoreview command?
>
> Well, how do we add an argument to a command, in a way that it's simple
> and consistent? Perhaps <restore> ? We don't have dashed arguments,
> we do have <buffer> for :map. It's less likely to clash with a valid
> argument at least. And we have arguments like <sfile>, <abuf>, etc.

Okay, I'll go with <restore> and check it out within the next 2 weeks. I
am on vacation currently, so don't expect anything very soon.

Best,
Christian
--
Wahre, in alle Zeiten und Nationen eingreifende Urteile sind
sehr selten.
-- Goethe, Maximen und Reflektionen, Nr. 777

[Christian Brabandt]

On Sa, 26 Dez 2015, Christian Brabandt wrote:

> Okay, I'll go with <restore> and check it out within the next 2 weeks. I
> am on vacation currently, so don't expect anything very soon.

Here is a patch series, that adds the <restore> argument to the
:windo/bufdo/argdo/cdo commands that should correctly restore the cursor
position after those commands. The patch includes updates to the
documentation, a newstyle test and an update to the matchparen plugin,
that initially caused the trouble.

Instead of sending one single patch, I decided to practice my git skills
and created a patch series. That should be easier to review. Let's see
how well this works.

Best,
Christian
--
Es ist leichter zu schmeicheln, als zu loben.
-- Jean Paul

[Christian Brabandt]

On Mi, 06 Jan 2016, Christian Brabandt wrote:

> Here is a patch series, that adds the <restore> argument to the
> :windo/bufdo/argdo/cdo commands that should correctly restore the cursor
> position after those commands. The patch includes updates to the
> documentation, a newstyle test and an update to the matchparen plugin,
> that initially caused the trouble.
>
> Instead of sending one single patch, I decided to practice my git skills
> and created a patch series. That should be easier to review. Let's see
> how well this works.

And this time, also include the newstyle test.


Best,
Christian
--
"Ich bin über die Wurzeln des Baumes gestolpert, den ich
gepflanzt hatte." Das muss ein alter Forstmann gewesen sein, der dies
gesagt hat.
-- Goethe, Maximen und Reflektionen, Nr. 547

[Bram Moolenaar]

Christian Brabandt wrote:

> On Sa, 26 Dez 2015, Christian Brabandt wrote:
>
> > Okay, I'll go with <restore> and check it out within the next 2 weeks. I
> > am on vacation currently, so don't expect anything very soon.
>
> Here is a patch series, that adds the <restore> argument to the
> :windo/bufdo/argdo/cdo commands that should correctly restore the cursor
> position after those commands. The patch includes updates to the
> documentation, a newstyle test and an update to the matchparen plugin,
> that initially caused the trouble.

Thanks, I'll put it on the todo list.

> Instead of sending one single patch, I decided to practice my git skills
> and created a patch series. That should be easier to review. Let's see
> how well this works.

Hmm, it will probably end up as one patch.


--
It is illegal for anyone to give lighted cigars to dogs, cats, and other
domesticated animal kept as pets.
[real standing law in Illinois, United States of America]

[Justin M. Keyes]

Instead of requiring <restore>, the default behavior could be changed.
Can anyone give a single reason why the old behavior is desirable or
useful, or necessary for backwards compatibility?

Ben mentioned that the existing behavior gives the user an indication
that "something was done". But this could be addressed by *actually
providing user feedback* (e.g. a brief message like "processed N
buffers" in the case of :bufdo).


Justin M. Keyes

> --
> --
> 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
>
> ---
> You received this message because you are subscribed to the Google Groups "vim_dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to vim_dev+u...@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

[Christian Brabandt]

Hi Justin!

On Fr, 08 Jan 2016, Justin M. Keyes wrote:

> Instead of requiring <restore>, the default behavior could be changed.
> Can anyone give a single reason why the old behavior is desirable or
> useful, or necessary for backwards compatibility?
>
> Ben mentioned that the existing behavior gives the user an indication
> that "something was done". But this could be addressed by *actually
> providing user feedback* (e.g. a brief message like "processed N
> buffers" in the case of :bufdo).

If we change this behaviour now, we'll probably break many plugins, that
expect the cursor to be in a different place after that command.

Having said that, I personally don't like the <restore> argument as
well. Perhaps we could use a new command modifier like
:keeppos windo ...

That could be useful for other commands as well.

Best,
Christian
--
Meister der Beredsamkeit ist der, der alles Nötige sagt und nur dies.
-- François de La Rochefoucault

[Ben Fritz]

On Friday, January 8, 2016 at 2:33:04 AM UTC-6, Christian Brabandt wrote:
>
> Having said that, I personally don't like the <restore> argument as
> well. Perhaps we could use a new command modifier like
> :keeppos windo ...
>
> That could be useful for other commands as well.
>

I like that idea better as well.

[Charles E Campbell]

I, too, like the "keeppos" (short for keepposn?) command modifier.

I agree that one shouldn't change the default behavior due to backwards
compatability considerations. My own plugins typically do a
save&restore position and so wouldn't be affected by whether or not that
default behavior changed.

One thing about keepposn, though: a lot of the save&restore position
commands currently available keep the cursor in the same place in the
text, but don't keep the text in the same position on the screen.
Consequently using these causes the text to bounce around, which IMHO is
unacceptable. For example,

:set ve=all
:help getcurpos()
:let scp= getcurpos()
:norm! 4k4zl
:call setpos('.',scp)

This set of operations restores the cursor position in the text, but
does not restore the text position relative to the window.

Regards,
Chip Campbell

[Christian Brabandt]

Hi Charles!

Yes, that is the idea and that is what the <restore> argument already
does. I believe however, that the :keeppos modifier is a lot more
effort. I'll check this within the next week.

Best,
Christian
--
Viele verlieren den Verstand deshalb nicht, weil sie keinen haben.
-- Arthur Schopenhauer

[Justin M. Keyes]

On Fri, Jan 8, 2016 at 12:15 PM, Charles E Campbell
<drc...@campbellfamily.biz> wrote:
> Ben Fritz wrote:
>> On Friday, January 8, 2016 at 2:33:04 AM UTC-6, Christian Brabandt wrote:
>>> Having said that, I personally don't like the <restore> argument as
>>> well. Perhaps we could use a new command modifier like
>>> :keeppos windo ...
>>>
>>> That could be useful for other commands as well.
>>>
>> I like that idea better as well.
>>
> I, too, like the "keeppos" (short for keepposn?) command modifier.
>
> I agree that one shouldn't change the default behavior due to backwards
> compatability considerations. My own plugins typically do a
> save&restore position and so wouldn't be affected by whether or not that
> default behavior changed.

It would not make sense for a plugin to depend on the current behavior
because the current behavior is unpredictable: if an error occurs the
cursor could end up anywhere; and the contents of each buffer are
unpredictable.

So again I ask, can anyone name one reasonable, realistic scenario
where a plugin would break by fixing this long-standing pain-point? I
think that "backwards compatibility" has become the easy way out of
giving extra thought to making the occasional bold decision in favor
of usability.

---
Justin M. Keyes

[Bram Moolenaar]

We could use either the <restore> argument or a :keeppos modifier.
Both have advantages and disadvantages. I think the problem with
:keeppos is that this gives the idea that it works for all commands,
while only a few commands will actually support it.
On the other hand, for a command like :s, a <restore> argument isn't
really possible, would need to use a flag, which means different
commands have different ways to keep the position.

--
<Beeth> Girls are like internet domain names,
the ones I like are already taken.
<honx> Well, you can stil get one from a strange country :-P

[Bram Moolenaar]

Then current behavior ends up on where the last change was done. That
can be useful. Especially for :argdo, where there very well would not
be a change at the cursor, or even in the current buffer. E.g.:

:argdo %s/\<that_var\>/\<thatVar\>/g

--
Lawmakers made it obligatory for everybody to take at least one bath
each week -- on Saturday night.
[real standing law in Vermont, United States of America]

[Olaf Dabrunz]

Then why not make the :keeppos modifier work for everything called with
it?

It could just be a shorthand for the equivalent of a winsaveview() /
winrestview() bracket around the command and everything it calls. This
needs to be done for each window that is modified while :keeppos is in
effect. The view information is saved internally in the window
structure. And when a buffer is switched, the saved view is kept
associated with it (buf->b_wininfo) and the view of the switched-to
buffer is saved as well, and restored before switching away.

And the active window in each tab is marked and restored later.

Actually, the "last active window" could be determined with timestamps,
one for each window, updated when entering or leaving the window, and
locked against changes when :keeppos is in effect.

So when the command closes the active window, the previously active
window becomes the active window.

This could also be useful in scripts: get_mru_time(tabnr, winnr).

--
Olaf Dabrunz (oda <at> fctrace.org)

[Justin M. Keyes]

I don't think any plugin depends on that. And even if there exists
such a plugin, the usability benefit greatly outweighs the cost of a
broken plugin which made this fragile assumption instead of using the
'[ and '] registers. Good engineering weighs cost vs. benefit: the
cost here is hypothetical and small, and the benefit is that a
long-standing usability problem in Vim will be fixed.

Justin M. Keyes

[Bram Moolenaar]

You misunderstand. I want that command to end at the last change, not
at whereever I happened to start it from.

--
To keep milk from turning sour: Keep it in the cow.

[Justin M. Keyes]

Fair point. I retract my objection then.

Justin M. Keyes