Treat changed remotes as new plugins

[jdevera]

What this PR is

This my counter proposal to @Uroc327's PR #400
Fixes #367 #48 #183

Based on @aaronjensen's suggestion to use git fetch && git reset --hard origin/HEAD instead of pull, but with an important distinction: Do it only when the URI in the Vundle config is different to that of the origin remote in the existing repository.

This, in effect, wipes out the old repo and checks out the contents of the new.

As a follow up, we should probably warn the user when they have clashing names in their config, as only the latest one will be valid.

From the commit message

If one had this plugin:

Plugin 'foo/bar'

And now switches to another one with the same name:

Plugin 'baz/bar'

Recognise this scenario and replace the old plugin with the new one,
rather than silently assuming they are the same.

How I tested it

I created a dummy repo:

mkdir /tmp/Align
cd /tmp/Align
git init .
echo "foo" > foo
git add foo
git ci -m foo

The I added these two lines to my vimrc:

Plugin 'file:///tmp/Align'
Plugin 'Align'

I restarted Vim and ran :VundleInstall (no need for !). I verified that both plugins got a + on the installer, that the log mentioned the remote change, and that I could use the Align command afterwards.

[jdevera]

@starcraftman could you please give it a spin under Windows when you have some time?

[Uroc327]

@jdevera _Thanks_ for looking into this issue! :)
Hadn't had the time to work out my pr further the last few weeks.

[starcraftman]

@jdevera Very nice proposal. Tested it on Windows, unfortunately it breaks PluginUpdate immediately without testing the collision bit.

As a follow up, we should probably warn the user when they have clashing names in their config, as only the latest one will be valid.

I agree, should be trivial to detect collisions based on name on addition to g:bundles via Bundle/Plugin or interactive.

Edit: Gotta love Windows :)

[jdevera]

@starcraftman Thanks for checking it, I'm going to take a look right away.

[jdevera]

It was an "oops" 😄 . Should be fixed now.

[starcraftman]

@jdevera I did a quick implementation on my rtp branch of the duplicate name detection. Is this what you were envisioning? Since this code includes the lazy_bundle method, I'll probably push it. Likely as a separate PR/branch on top of my rtp changes.

edit: accidently put the post in wrong pr, hehe.
edit2: I wonder if I can make the lazy one a bit shorter, aimed to reduce search time.

diff --git a/autoload/vundle/config.vim b/autoload/vundle/config.vim
index 1f6b0fc..262bc93 100644
--- a/autoload/vundle/config.vim
+++ b/autoload/vundle/config.vim
@@ -1,6 +1,9 @@
 func! vundle#config#bundle(arg, ...)
   let bundle = vundle#config#init_bundle(a:arg, a:000)
   call s:rtp_rm_a()
+  if len(filter(copy(g:bundles), 'v:val.name == bundle.name')) > 0
+    echoerr 'name collision for plugin: ' . bundle.name
+  endif
   call add(g:bundles, bundle)
   call s:rtp_add_a()
   call s:rtp_add_defaults()
@@ -13,6 +16,15 @@ endf

 func! vundle#config#activate_all_plugins()
   call extend(g:bundles, map(s:lazy_bundles, 'vundle#config#init_bundle(v:val[0], v:val[1])'))
+  let l:check = sort(map(copy(g:bundles), 'v:val.name'))
+  let l:last = remove(l:check, 0)
+  while len(l:check) > 1
+    let l:cur = remove(l:check, 0)
+    if l:last ==? l:cur
+      echoerr 'name collision for plugin: ' . l:cur
+    endif
+    let l:last = l:cur
+  endwhile
   call s:rtp_add_a()
   call s:rtp_add_defaults()
 endf

[starcraftman]

@jdevera Retested this on Windows and appears to work as expected. For the test I used our recommended GVim + cmd.exe with git on the path, OS is Win7.

While looking at code, I noticed one thing. See feedback on diff.

[starcraftman]

This line will fail if the repo existing doesn't have an origin (i.e. current_origin_url == ''). In that case it would have to be git remote add.
Not 100% certain this is valid use case.

[jdevera]

It could be a valid use case if someone has a pinned repo that is a clone of nothing and then they publish it and start following the published one. I'll take a stab at it tomorrow.

Is this what you were envisioning?

Yes and no. Yes because that is what we are doing for other stuff, no because I'd prefer to populate a dictionary with the names as keys as we run the Plugin commands and then simply check if the dictionary already has the key.

[starcraftman]

@jdevera

Yes and no. Yes because that is what we are doing for other stuff, no because I'd prefer to populate a dictionary with the names as keys as we run the Plugin commands and then simply check if the dictionary already has the key.

True enough, well your call if you want to put the ugly way temporarily. I agree having the names as keys would be nicer. That's more of its own refactor/PR though.

[jdevera]

@starcraftman I agree, it's a matter of a separate PR to speed up things by using dictionaries rather than iterating.

[jdevera]

I'm thinking: If the repository has no origin remote, then it could be original work and we would be obliterating it by adding a remote and forcing the changes.

I think in these cases (possible due to the pinned feature):

• A directory with the bundle name exists, and it is a git repo with no origin remote
• A directory with the bundle name exists, and it not a git repo

We should probably not overwrite, and instead show error in the installer and log a message suggesting to manually handle that case. Otherwise we risk deleting user content (though they had it coming 😉 )

[starcraftman]

@jdevera If I may make a suggestion. A simple approach may be that taken by PluginClean, on finding a changed URL we simply notify user via message and ask if he wants to overwrite the old one. Otherwise, we instruct him to check his vimrc/bundle_dir. How does that sound? Then it doesn't matter what the bundle folder contains, the user can investigate it or if he knows just respond to question.

[jdevera]

@starcraftman I think I do prefer your option. Asking before doing anything. What is @gmarik's opinion?

[gmarik]

i like it!
But if you make origin/HEAD configurable it becomes version feature.

Have you considered this?

@jdevera if you asking me particular question could you please summarize it?
There's too much and i could misunderstood you…

[jdevera]

@gmarik yes, all the recent additions are like little steps taking us to the point where adding that feature will be really easy, in fact, I already started working on it, but there are a lot of error cases that need to be treated. I'll send a naive PR this week or the next. But for now we need to figure out the thing at hand.

So, to summarise:

A user might have the work of his lifetime in a pinned plugin called, very unluckily, mmm, say, Align:

Plugin 'Align', {'pinned' : 1}

Which might or may not be a git repo within the bundles directory.

Unluckly, I said, because he now heard of this plugin called Align that he's dying to try, so he adds it to his vimrc or maybe even installs it explicitly.

BAM! we killed his puppy. 💣

So what @starcraftman was suggesting is that we ask the user if he wants to obliterate his precious Align work in favour of the new Align plugin he's installing. And the proposed method is just the same way VundleClean does it, asking a yes no question.

The alternative is that we simply do our thing, resetting the HEAD as this PR does. If Align was originally a git repo, perhaps there is a way to recover those branches. If it wasn't, the files would be left there, uncommitted, with no loss.

[jdevera]

While writing the summary above, I realised that for this to be a problem, two bundles would have to be targeted for the same directory at the same time, a problem which @starcraftman and I are already talking about how to prevent.

So the only real scenario where overwriting is a problem is when the user has replaced a bundle with another. I'd say, well, tough luck. And I'd also say, well, dear user, Vundle owns the bundles directory. And as I mentioned, chances are that anything is till recoverable afterwards.

So now I think it's better not to do anything. 😄

[gmarik]

wait, have we rolled out 'pinned' feature ? :)
keep it simple: don't add pinned feature — less headache and edge cases.
IMHO

[jdevera]

It's on branch next, which is next week's master.

A headache free solution is achieved as per my last comment. Which basically said that the problem will not happen.

No need to remove functionality.

[starcraftman]

@jdevera If I understand correctly I'd just like to summarise what is expected Vundle behaviour after this patch:

1. We should detect when multiple Bundle lines by user accidentally overlap in name (i.e. two Plugins that target 'python-vim') and report an error to user.
2. We don't need to do extra work in scenario where user has existing directory and adds a bundle that targets this dir, tough luck. User should be careful about what exists in dir and lines targeting it.
3. No need for confirmation in case 2, just obliterate.

[jdevera]

1. Exactly
2. Yup, just like when you remove a Plugin line from your vimrc and do a VundleClean
3. Yup

If I get some time tomorrow, I'll rebase on latest next and apply the missing changes. Then will push again for further review.

[jdevera]

Rebasing on next screwed up this PR, since it was open against master. I've opened a new one with these changes + what's in the comments: #440