MarcWeber / vim-addon-manager

manage and install vim plugins (including their dependencies) in a sane way. If you have any trouble contact me. Usually I reply within 24 hours
Other
660 stars 59 forks source link

svn updating broken #162

Closed cheater closed 10 years ago

cheater commented 10 years ago

When I did :UpdateActivatedAddons today, I got to see this:

Press ENTER or type command to continue svn: E155036: Please see the 'svn upgrade' command svn: E155036: Working copy '/home/cheater/.vim/vim-addons/Conque_Shell' is too old (format 10, created by Subversion 1.6)

shell returned 1

may make sense to just run 'svn upgrade' all the time, regardless of whether it's needed or not?

ZyX-I commented 10 years ago

I do not think this is good idea: one directory may be shared among different computers with different SVN versions. The error message that is given here is clear and is not caused by VAM. If we do upgrade and then user had to downgrade subversion or use older version then regular and implicit upgrade will make finding the problem much harder.

MarcWeber commented 10 years ago

1) you could also ask this question to SVN devs 2) if you upgrade always and then use an older system, it could fail there, so upgrading always could lead to trouble, too. So which case is more likely? Maybe yours. 3) There is a second solution "rm -fr" the plugin and checkout from scratch (which always works in all cases)

VAM allows to override the update/checkout implementations easily. See MyGitUpdate example in getting-started.txt Thus I'd like to ask users to solve this on their own.

:-) @ZyX-I thinks the same. So I'm closing this till there is a second request asking for this.

Maybe we could even be as agressive as asking the user "updating failed, do you want me to rm -fr .. and checkout again?" We could at least print the rm -fr command so that its faster for the user to solve it (and take responsibility on his/her own)

cheater commented 10 years ago

Sharing an SVN checkout is doing it terribly, terribly wrong. SVN was never meant for this. ZyX-I's scenario is in effect a non-issue, unless the person running into the issue is using SVN in a way it wasn't meant to be used. Therefore I ask you to reopen this ticket.

ZyX-I commented 10 years ago

@cheater You are understanding it wrong. A person will likely not to share SVN checkout. He will most likely share .vim directory as a whole.

cheater commented 10 years ago

@ZyX-I, I fully understood that. Since your .vim directory may contain SVN checkouts, it should not be shared across machines. Either ask VAM to store checkouts elsewhere, or just share the vimrc, which has everything you need to recreate all the checkouts. That's what I do and it works well. Would you like to share /dev across machines too? There are files which shouldn't be shared between computers, and SVN checkouts are one example.

cheater commented 10 years ago

I just think it sucks we end up with things that break in 1 of 100 cases because rather than come up with the solution that'll work for most people (i.e. make the update work well automatically) we end up bickering over details ("i cannot do this crazy thing with an SVN dir that SVN was never meant for, so nobody touch this code")

Besides, here's a fix for you: just create the .vim dir on the machine with the oldest SVN, and propagate that to other machines, they'll happily upgrade, and your old machine will still work, because it doesn't need to upgrade. That solves even your, very unnatural, situation.

ZyX-I commented 10 years ago

Sharing .vim is not unnatural. I constantly see people who use dropbox to share their configuration files among systems in place of some version control systems. And you did not provide any details on what and why will break. The fact that some thing was never designed for some job does not mean it cannot do this job.

And one thing why svn upgrade must not run always: if there is nothing to upgrade it prints messages to stderr and exits with non-zero exit code.

ZyX-I commented 10 years ago

Some additions:

MarcWeber commented 10 years ago

My argument was: This should be fixed in SVN if at all. Thus talk to SVN folks and make them add a ~/.svnrc (or such) option which automatically upgrades in such cases. I'm not against fixing 1% of failing updates. echo 'autoupgrade = true' >> ~/.svnrc or similar (sample)

cheater commented 10 years ago

@MarcWeber expecting svn to change in any way at all is completely unrealistic, I don't know why you bring up this idea? I'm sorry but it feels like an attempt to shift this idea into a black hole.

@ZyX-I I don't understand the "controversy" (see comments below). That said, it's as simple as:

error=$(svn upgrade 2>&1) status="$?" if [ "$status" -gt 0 ] && ! (echo "$error" | grep E155019 > /dev/null ); then echo "$error" 1>&2; exit "$status"; fi

Those three lines can't possibly be overbearing. And they're in the same style that all possible svn guides recommend, so we're not doing anything weird or unusual with svn.

Guys, I seriously didn't expect The French Revolution on a change request that is literally one if branch, 3 simple lines of code, and which can only go wrong extremely unrealistic edge cases. And being defended is a system which goes wrong in more realistic, more common cases, for everyone, every time an svn bug fix changes the format version number. More than "controversy" this feels like Fox News sensationalism.

I see a situation here where we're taking literally garbage (automatically generated files meant only for use of VAM, not meant to be moved to other machines, not meant to be modified by hand), and give it special meaning and special treatment, "just because someone might want to". Why even enter this dark, deep passage leading to insanity? For example, an argument is that the automatic upgrade might go wrong. So what? The directory was created by VAM automatically, it's just as permanent as a temp file. It's very unlikely that the automated upgrade will kick in, and out of all those cases it's even less likely that it'll fail, because the checkout would have to be very, very broken for that. Right now we're talking 1 in 100 000. And even in this unlikely case the solution is very simple - delete and get it again. If automated upgrading keeps on failing, then the repository is broken, and you should report to the maintainer, because they'll actually want to know. Worst case scenario, if you're trying to resolve an issue and automatic upgrades are somehow interfering, grep the VAM source code for "svn upgrade" and disable it there temporarily. Right now we're talking one in a million probability.

Another unrealistic argument - people share .vim over dropbox AND might have an old version of svn on one computer being shared to. It's unlikely someone has dropbox (fairly recent software) but doesn't have fairly recent svn. Again, we're coming up with unrealistic edge cases.

ZyX-I commented 10 years ago

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA512

On June 28, 2014 1:52:23 PM GMT+03:00, cheater notifications@github.com wrote:

@MarcWeber expecting svn to change in any way at all is completely unrealistic, I don't know why you bring up this idea? I'm sorry but it feels like an attempt to shift this idea into a black hole.

@ZyX-I I don't understand the "controversy" (see comments below). That said, it's as simple as:

error=$(svn upgrade 2>&1) status="$?" if [ "$status" -gt 0 ] && ! (echo "$error" | grep E155019 > /dev/null ); then echo "$error" 1>&2; exit "$status"; fi

These lines are written for POSIX-compatible shell and as such cannot be accepted: shell may be tcsh, fish, cmd.exe and powershell where they will not work. grep may be not present on windows as well.

You can write this in VimL, but you will not cope with five lines (no these are not three lines: I can squash the whole autoload into one line, but this will not make such code acceptable).

And, by the way, E155019 is not the only message: I also see "E150000: Missing default entry". I am not sure what it means and should it be silenced by this code as well.

Those three lines can't possibly be overbearing. And they're in the same style that all possible svn guides recommend, so we're not doing anything weird or unusual with svn.

Guys, I seriously didn't expect The French Revolution on a change request that is literally one if branch, 3 simple lines of code, and which can only go wrong extremely unrealistic edge cases. And being defended is a system which goes wrong in more realistic, more common cases, for everyone, every time an svn bug fix changes the format version number. More than "controversy" this feels like Fox News sensationalism.

I see a situation here where we're taking literally garbage (automatically generated files meant only for use of VAM, not meant to be moved to other machines, not meant to be modified by hand), and give it special meaning and special treatment, "just because someone might want to". Why even enter this dark, deep passage leading to insanity? For example, an argument is that the automatic upgrade might go wrong. So what? The directory was created by VAM automatically, it's just as permanent as a temp file. It's very unlikely that the automated upgrade will kick in, and out of all those cases it's even less likely that it'll fail, because the checkout would have to be very, very broken for that. Right now we're talking 1 in 100 000. And even in this unlikely case the solution is very simple - delete and get it again. If automated upgrading keeps on failing, then the repository is broken, and you should report to the maintainer, because they'll actually want to know. Worst case scenario, if you're trying to resolve an issue and automatic upgrades are somehow interfering, grep the VAM source code for "svn upgrade" and disable it there temporarily. Right now we're talking one in a million probability.

You see resolution as simple because you know where to look. One will not grep VAM source because he has no idea that it is VAM source that should be searched.

This may be solved by confirming "svn upgrade". But in this case upgrade must be run only when it is needed to upgrade to not bother user always.

Another unrealistic argument - people share .vim over dropbox AND might have an old version of svn on one computer being shared to. It's unlikely someone has dropbox (fairly recent software) but doesn't have fairly recent svn. Again, we're coming up with unrealistic edge cases.

It is not unlikely: Gentoo amd64 (stable) subversion (1.7.) is not compatible with Gentoo ~amd64 subversion (1.8). Knowing about some other distributions that tend to upgrade to bleeding edge software it is not impossible to have different distributions or different sets of keywords on different machines.

Dropbox is not the only variant of sharing filesystems as well.


Reply to this email directly or view it on GitHub: https://github.com/MarcWeber/vim-addon-manager/issues/162#issuecomment-47423308

-----BEGIN PGP SIGNATURE----- Version: APG v1.1.1

iQJNBAEBCgA3BQJTrrHCMBwfMDI7PjIgHTg6PjswOSAQOzU6QTA9NEA+MjhHIDxr cC1wYXZAeWFuZGV4LnJ1PgAKCRBu+P2/AXZZIhe9D/9hfKn9oSofdbenaVfafaTA DxqnidJ+eq4/lO/AgaZo1rQPP30oFtNHmaU74sJPKbShplwhFL43BODE76lmFpj2 X1+7RHHQp+yTzWG2Yw+lhSOp1qVPpkkgJB3tJEx+AY0EYgn4ATIycQoYKvsY24fS WkssNtJ0FikCN5uYhQYmLnOI+YmeVV6guKQpozUdribLReZVEtRzXAMXkFuIDZc2 yQCjHa8Np/PGQfmXSCjR45EziXT8DdY5JtJwzuklbwQ2iXufdt3YcKqO02AKph3+ ezgvESAYw9jxOHkyaV54cUQIepACQ32BKTHcqAZKCpQ2M93s2GQm3bGIY9pZsBKU JEeJr/jH7Y81U/DkRnRU1svEvMiVkSoacqUoNDf2tbQtqCBuC5pOgpFdIfDiQVmH LEkFFVWmePl9Lqkn+1A545zqreSvZXWx5Xo5TVLToJ/pl425CEgdypvPqLm5eXjr 31zv0nd89/gHsuqMhck/0hHoWRBsC9E49+/4yo/XhIDVyaThUHA0H1FGilBxtoFV zHJhKmCdgNtbEK0b0Nv3rw+qHkkveJnHamRIqDC+qrrdktT1wJab2zZOzts4h7Dk x+OZE6xONeK/eATfhCGM9vJ6NXlT4XbWA3yPuz04FoIm82luFugOF664ciJx9O2y o/xmmIGA51pBDpgiTurukg== =yJG3 -----END PGP SIGNATURE-----

MarcWeber commented 10 years ago

@MarcWeber expecting svn to change in any way at all is completely unrealistic,

Did you try it? I seriously believe this is the way to go. Because your case does not only affect VAM users, it affects all SVN users. For that reason SVN dev team should be asked before VAM.

And I totally agree that VAM's take (using addon-info.json files) is limited. However you're wrong: You may edit them by hand. And I encourage others to adopt it. It just didn't happen. Vundle people still discuss how to fix the parser issues (VAM does not suffer from those).

I want to say: that nobody adopted a JSON style config file for vim plugins is not only my fault - but also shows lack of collaboration.

The long time fix would be create a package manager which knows about Vim (and all if_* supports), and plugins which need that and so on. I have an idea how it should look like and described some properties here: https://github.com/code-once/ypm However I don't have time to work on it at the moment.

For this reason I only maintain VAM without spending much more effort on it.

If you want to get your change in consider providing a patch and at least get feedback from SVN devs about what I said. Thinking about it again I'm 50 for it and 50 against it. So probably I'd accept a patch because I don't care enough. Both choices are equally nice in most cases.

ZyX-I commented 10 years ago

About patch for subversion: there may actually a reason why they do not upgrade automatically. If there is it is likely that it also applies to VAM should it add automatic upgrading.

MarcWeber commented 10 years ago

My proposal was to make the subversion patch "opt-in" by .svnrc or similar. If there is a patch for VAM we can make it opt-in/out. And we should add the arguments of this discussions by adding comments. There are not that many SVN repositories anymore. Having a happy user might be more important than trying to figure out which case is worse/better. I personally would slightly prefer adding a sample to the documentation - but I also want users to be happy. A use case where this auto upgrading behavior does hurt has yet to be found, and if it exists, it can be solved by "rm -fr" and checking out again. After all the user is asking VAM to make a change (do an update). So there are many more risks which he runs (such as breaking code). If can't decide we should at least think about allowing an opt-in solution for VAM. The noise we're generating here is more than the patch is about eventually. @cheater: Would you be fine with "opt-in" option? such as let g:vim_addon_manager_svn_try_upgrade_always = 1 ? It looks everybody would be happy then. Do you feel strong enough to provide a patch or do you need help? I really encourage you to get feedback from SVN devs. My choice here also takes into account that most Vim plugins are small in size and that checking out from scratch is likely to hurt a little - and that that fix is likely to solve most problems. We could also link to this thread to gather more feedback.

ZyX-I commented 10 years ago

Would you be fine with "opt-in" option? such as let g:vim_addon_manager_svn_try_upgrade_always = 1 ? It looks everybody would be happy then.

This variant is inconsistent with existing options which all live in one dictionary. And such option solves nothing except for ability to ship implementation that upgrades by default with VAM. Best variant would be confirmation of svn upgrade only asked when upgrade is needed and an option to disable confirmation not set by default.

cheater commented 10 years ago

@MarcWeber having a global option whether to auto-upgrade every svn repository on your system is a whole different question. svn in general can host very complicated projects, there's a lot that can go wrong in an svn upgrade if you are dealing with this kind of thing. modules, weird hooks, whatever, all of that can go wrong. None of this applies to us, for the reason that historically there are only two reasons to have your code in svn:

  1. Your project uses a super-complicated svn repository, and probably has a build system based off of it, and it was set up by some svn god whose help you cannot ask any more, and you can't migrate to anything else. Those are usually very complicated and shouldn't be upgraded.
  2. Your manager is clueless and doesn't let you upgrade from svn (this case doesn't matter to us, no Vim plugins are developed by companies) or: you're clueless and you host your code on Google Code. This last case is the only one that matters for us. Those repositories are going to be dead simple. I don't think you can even have a complicated repository with Google.

@ZyX-I this is why you're overthinking this: we're only dealing with super-simple repositories, there's no complicated stuff at play here. There are 28 plugins registered with VAM that use svn; out of those, only two are not on google code, and with one of those the repository doesn't even exist any more. The other one is on sourceforge and is very uncomplicated.

I would be happy with a flag, with the reservation that it should probably default to on - because it's not discoverable if it defaults to off. Whereas if it were default on, then the person who wants to disable svn upgrade will go into VAM's source code, look at the line that has "svn upgrade", look above it and notice an if branch based off that variable. Or can look in the docs, and grep them for "svn upgrade".

@MarcWeber I will try coming up with a patch, but VimL comes with difficulty for me. I'll see if I can get it cobbled together.

MarcWeber commented 10 years ago

@cheater: test this patch and fix remaining issues, then send a pull request http://mawercer.de/tmp/vam-svn-update.patch

This should get you started fast.

@ZyX-I I hope this implementation is fine because you can opt-out and it only upgrades if svn message suggests it.

Is this fine for everybody?

ZyX-I commented 10 years ago

@MarcWeber You must not use &>. It is not portable. There is in fact no one portable way to redirect stderr, but AFAIK &shell* options are already set to redirect stderr for known shells when using system(). So you do not have to redirect anything, you just need to be sure you use system().

ZyX-I commented 10 years ago

Ah, and s:c.svn. There is no such a thing.

ZyX-I commented 10 years ago

And also == and =~ for string comparison. First should be replaced with is#. Second with at least =~#, better with stridx(haystack, needle) != -1 because regexes are not needed here.