Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Asynchronous tag updates (attempt two) #84

Merged
merged 13 commits into from Jul 8, 2014
Merged

Asynchronous tag updates (attempt two) #84

merged 13 commits into from Jul 8, 2014

Conversation

xolox
Copy link
Owner

@xolox xolox commented Jun 22, 2014

What is this all about?

I've reimplemented support for asynchronous tags file updates (see pull request #49 for my previous and failed attempt). Most of the complex code has moved to vim-misc so to try out this branch you need to pull the latest changes to vim-misc as well. Apart from that all you (should) need to do is add :let g:easytags_async = 1 to your vimrc script. An easy way to demonstrate that it works is to run :UpdateTags -R and continue to work in Vim...

So what's the plan?

Once a couple of people confirm that this feature branch works for them, both in in synchronous and asynchronous mode, I will merge this into master. Then we can see when we switch the default mode to asynchronous updates :-).

About code complexity

Even though I pushed most of the complexity of handling asynchronous processing into vim-misc and I ripped out some useful features that were blocking the implementation of asynchronous updates, I'm still kind of pleased with the diff --stat:

 README.md                             |   22 +-
 autoload/xolox/easytags.vim           |  582 +++++----------------------------
 autoload/xolox/easytags/filetypes.vim |  122 +++++++
 autoload/xolox/easytags/update.vim    |  268 +++++++++++++++
 autoload/xolox/easytags/utils.vim     |   20 ++
 doc/easytags.txt                      |   66 ++--
 plugin/easytags.vim                   |   19 +-
 7 files changed, 537 insertions(+), 562 deletions(-)

Disclaimer: A couple of refactorings sneaked into this feature branch because I tried to simplify and generalize code as I went on with my quest towards asynchronous tags file updates in master :-)

@xolox xolox mentioned this pull request Jun 22, 2014
@xolox
Copy link
Owner Author

xolox commented Jun 22, 2014

@tarmack: You offered to test drive the new asynchronous mode of vim-easytags... If you're still up for it you can find it in the feature branch referenced in this pull request. Thanks!

@xolox
Copy link
Owner Author

xolox commented Jun 22, 2014

Required platform support before I merge this into master:

  • Graphical Vim on Linux (tested on my Ubuntu 12.04 system)
  • Console Vim on Linux (tested on my Ubuntu 12.04 system)
  • Graphical Vim on Windows
  • Console Vim on Windows?
  • Graphical Vim on Mac OS X
  • Console Vim on Mac OS X

inkarkat and others added 9 commits June 23, 2014 17:54
On Windows, tagfiles() can return a filespec that is absolute to the current drive (i.e. \foo\bar\tags). In async mode, the forked Vim process may have another current drive, so it should be ensured that the filespec is a full one, including the drive letter: D:\foo\bar\tags.
This change is related to pull request #82 however that pull request
wasn't merged here (and won't be merged at all) because it was based on
the old/dead `async-cleanup' feature branch (see pull request #49 on
GitHub) instead of the new `async-take-two' feature branch (see pull
request #84 on GitHub). This change set implements the equivalent on the
new feature branch.

In addition to Ingo's comments in pull request #82, the asynchronous
message frequently disturbs me while typing a Vim command, which is kind
of annoying. If everything goes well and we can get the async mode to be
stable enough to become the default mode then the status messages will
only be interesting when debugging a problem anyway.
This change is related to pull request #83 however that pull request
wasn't merged here (and won't be merged at all) because it was based on
the old/dead `async-cleanup' feature branch (see pull request #49 on
GitHub) instead of the new `async-take-two' feature branch (see pull
request #84 on GitHub). This change set implements the equivalent on the
new feature branch (without introducing another option).
xolox added a commit to xolox/vim-misc that referenced this pull request Jun 29, 2014
To be used in the feature branch xolox/vim-easytags#async-take-two of
the vim-easytags plug-in (see also xolox/vim-easytags#84) in order to
preserve the permissions of tags files (as explained in the docs; it's a
good example of why this functionality is very useful to have on UNIX).
xolox added a commit to xolox/vim-misc that referenced this pull request Jun 29, 2014
To be used in the feature branch xolox/vim-easytags#async-take-two of
the vim-easytags plug-in (see also xolox/vim-easytags#84) in order to
preserve the permissions of tags files (as explained in the docs).
@xolox
Copy link
Owner Author

xolox commented Jul 1, 2014

@inkarkat: With the additional changes in this feature branch and vim-misc I was able to get asynchronous tags file updates working on my Windows XP VM running Vim 7.4 and Exuberant Ctags 5.8. Unfortunately I don't have access to any more recent versions of Windows so I can't test there. Is it still broken for you?

One nasty bug I encountered this weekend (which may very well be related) is that things work fine when vim-shell is installed but not at all without vim-shell installed. While debugging I went so far as to run procmon and I can clearly see that without vim-shell installed the command line being executed is completely broken. The thing is, I've tried a dozen variants of quoting, not quoting, broken quoting, horrible quoting, using ^, etc. and none of these variants have worked so far.

I can't believe I'm debugging Windows issues a couple of years after I let the Microsoft platform behind me ;-) (ducks for cover)

@inkarkat
Copy link
Contributor

inkarkat commented Jul 7, 2014

Thanks! I can confirm that it's working, both with 7.4.316 and 7.3.823 on Windows 7/x64. I'm using the latest commits from async-take-two, vim-misc, and vim-shell.

There was just one minor issue: For #86, you didn't directly use my patch, but instead xolox#misc#path#absolute(), which has a bug with drive-local paths. I've just sent you a patch for that.

I've re-enabled async updates in my regular config; I will test this in the following days and report back any problems (in the hope that there will be none, and this finally gets into the mainline.)

@xolox
Copy link
Owner Author

xolox commented Jul 7, 2014

Thanks for the feedback, I just merged xolox/vim-misc#12.

I'm looking forward to merging this feature branch into main line :-). Even if I don't switch the default until some time later, at least we'll be sure the code doesn't bit rot and is instead kept up to date.

About switching the default: Before I do that I would like to get things working even without vim-shell installed, because the whole point of the complicated vim-easytags → vim-misc → vim-shell construct was that vim-shell should be optional at all times. Right now in this branch on Windows it's not, at least in my tests based on Windows XP. Fixing this issue is probably going to get nasty and will likely require me to dive (way too) deep into Windows programming (so it might take a while :-), but in the end vim-misc and all Vim plug-ins built on top of vim-misc will benefit from it, so despite my aversion to debugging Windows command escaping issues, I still want to fix this.

@inkarkat
Copy link
Contributor

inkarkat commented Jul 8, 2014

For me personally, it would be sufficient to make async updates dependent on vim-shell; but I commend your perseverance in face of the daunting challenges that the Windows shell definitely poses.

Is there an easy way to disable vim-shell without uninstalling it? (So that I could easily test that configuration.)

Apart from that, I can only offer these two comments:

  • The problems seem to be caused by sending quoted command arguments within the command line (and the resulting double-escaping). If you instead send individual arguments, and only assemble and shellescape them in stage 2, the issue might be avoided (but you lose the generic functionality of remote commands, or at least the convenient API). I had remarked something similar earlier already.
  • Note that the Windows quoting was changed (hopefully improved) in the middle of version 7.3; be sure to use a late build of that version (not 7.3.000), or 7.4.

@xolox
Copy link
Owner Author

xolox commented Jul 8, 2014

Is there an easy way to disable vim-shell without uninstalling it? (So that I could easily test that configuration.)

I actually added an option for that to make it easier to test this feature branch... See xolox/vim-shell@c697297 :-)

The problems seem to be caused by sending quoted command arguments within the command line (and the resulting double-escaping). If you instead send individual arguments, and only assemble and shellescape them in stage 2, the issue might be avoided (but you lose the generic functionality of remote commands, or at least the convenient API). I had remarked something similar earlier already.

Yes that's what I thought as well, however I created a modification of this feature branch that would generate a Vim script and load it into the async background process using vim -S (so that the Vim command line was fairly simple and certainly didn't involve any complex quoting) and still the external command failed according to procmon. At that point I gave up and decided to give things a few days to sink in and then try again. I'm convinced it has something to do with the funky semantics of the start /b command, but that doesn't give me a lot to go on :-)

Anyway, at the end of the day the asynchronous mode is an optional feature, and having to use vim-shell on Windows wouldn't be that bad, it just bothers me that I can't force Windows into submission to make it do what I want it to do. In the end it's just software so it shouldn't be this hard :-P

@xolox
Copy link
Owner Author

xolox commented Jul 8, 2014

I'm considering merging this into master tonight. I may be going a bit fast, but there are a dozen other issues I want to fix in vim-easytags and I'm currently blocked by this huge feature branch that is waiting to be merged (refactorings on both sides of the merge = merge hell).

I've been running this feature branch for two weeks now and have fixed all major issues I found. It even works on Windows given the installation of vim-shell. For now I can keep the feature opt-in but add some advertisement in the readme :-)

@xolox xolox merged commit 4aafe1c into master Jul 8, 2014
xolox added a commit that referenced this pull request Jul 8, 2014
This is part 1/2 of speeding up the vim-easytags plug-in. Refer to the
pull request for details: #84
In part 2/2 I want to speed up the dynamic syntax highlighting.
Potentially related open issues on GitHub (probably missed a few):

 - #32
 - #41
 - #68
@xolox xolox deleted the async-take-two branch July 8, 2014 21:08
@xolox
Copy link
Owner Author

xolox commented Jul 8, 2014

And there we go. I pushed the results to the master branch on GitHub but haven't published a ZIP archive to www.vim.org yet, might wait a couple of days with that. Here's hoping I won't receive a flurry of issues on GitHub before then :-).

@inkarkat: Thanks a lot for all of the feedback and contributions in getting this feature implemented, it's very much appreciated! I've credited you in the readme in 5f17a01. Really should have done that earlier given that you implemented the support for custom language commands. Anyway, better late than never :-).

xolox added a commit that referenced this pull request Aug 8, 2014
See issue 90 on GitHub:
  #90

This was caused by the async refactor:
  #84
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants