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

Always sync submodules before updating them #912

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

chiphogg
Copy link
Contributor

This ensures the origin in .git/config matches the one in
.gitmodules. Git will quite appropriately refrain from doing this
automatically, because it never allows remote repositories to update
local config. You have to ask.

(See: https://stackoverflow.com/a/45679261)

In Vundle's case, it is always correct to sync. These aren't repos that
a developer maintains; they are effectively read-only copies of remote
state. Since syncing is always correct, and git won't sync unless we
ask, then we should always sync.

Fixes #911.

Test plan

This is an obscure but important issue. It's only a matter of time before a
plugin which actually has significant numbers of users feels the need to fix
a submodule origin URL.

I realize Vundle is staffed by volunteers, and I realize very few people are up
to date on this issue. So, to help out the reviewer, I'm providing fully
detailed reproduction instructions. It should take less than 5 minutes to
confirm the bug and the fix, even if you have no idea what it's doing. I hope
that this detailed reproduction plan, combined with:

will be enough to make this a pretty painless merge. And thanks again to those
same volunteers for their hard work on vundle!

Step 1: setup state

The goal here will be to get the vim-vtd folder in a similar state to how it
would be for pre-existing users (who are the only people impacted by this
change). Specifically, its submodule remotes need to be set to the old
upstream.

We can't do this within vundle, because the first thing vundle does is to pull
the latest version! So we'll need to use bare git. We also need to begin by
blowing away anything that might be there, because the git config settings can
get buried kinda deep.

The breaking change was chiphogg/vim-vtd@55771bc, which is why we'll be checking
out the parent.

#
# ENSURE YOU ARE IN YOUR VUNDLE BUNDLE DIRECTORY!!
#

rm -Rf vim-vtd/
git clone https://github.com/chiphogg/vim-vtd.git
cd vim-vtd
git reset --hard 55771bc~
git submodule update --init --recursive 

Step 2: confirm error

This should fail. (That's the bug.)

Open up vim, and run:

:PluginUpdate vim-vtd

Press l to view the log, and observe that we get the error mentioned in #911.

Step 3: patch

Patch in this new version of Vundle.vim in your bundle directory in whatever
manner you choose.

Step 4: confirm fix

Repeat Steps 1 and 2 above. (Actually, you can omit the "clean state", Step 1,
if you want to. The only truly necessary step is Step 2, reproduced here:)

:PluginUpdate vim-vtd

This is the exact same command as before, except now, it works!

This ensures the origin in `.git/config` matches the one in
`.gitmodules`.  Git will quite appropriately refrain from doing this
automatically, because it never allows remote repositories to update
local config.  You have to ask.

(See: https://stackoverflow.com/a/45679261)

In Vundle's case, it is always correct to sync.  These aren't repos that
a developer maintains; they are effectively read-only copies of remote
state.  Since syncing is always correct, and git won't sync unless we
ask, then we should always sync.

Fixes VundleVim#911.
@davidlowryduda
Copy link

This looks good to me. Thank you for identifying a problem and then fixing it (especially since the fix is not so hard).

@chiphogg
Copy link
Contributor Author

Thanks for the kind words! I don't suppose you have write access to this repo? 😉

@ryanoasis @gmarik ---anybody have time to check this over? (Could also tag with 0.10.3 if you want to pad its statistics... 🙂)

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.

Updating plugins should first call git submodule sync
2 participants