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

branches/tags/commit hashes support #189

Closed
wants to merge 7 commits into from
Closed

branches/tags/commit hashes support #189

wants to merge 7 commits into from

Conversation

Xerkus
Copy link

@Xerkus Xerkus commented Jun 27, 2012

I have added support for git tree-ish. Branch, tag or commit can be specified now.

Bundle 'vendor/bundlename', 'feature/branch-name'
Bundle 'vendor/bundlename', 'a83commit75hash2d5hd1bsdhd'
Bundle 'vendor/bundlename', '0.9.1'

Also i've added alias support in case there are compatible bundles with same name by different vendors

Bundle 'vendor/bundlename'
Bundle 'othervendor/bundlename as altname'

and local flag to prevent vundle from managing bundle installs and updates, but still registering it's resources. Useful for developers.

Bundle 'bundlename', {'local': 1}

new Bundle format is backwards compatible and can be described by

Bundle '(git url)|([vendor/]name)[ as alternate_install_name]'[, 'tree-ish'][, {json encoded options}]

Related issue: #35

@weynhamz
Copy link
Contributor

weynhamz commented Aug 6, 2012

The current implemention requrired the rest args is in a dictionary, you patch broke that, maybe it is convenient to just add a string after the uri, but that would break other possiblilty of extension.

Maybe this is a good way:

Bundle 'vendor/bundlename', {'rev':'feature/branch-name'}
Bundle 'vendor/bundlename', {'rev':'a83commit75hash2d5hd1bsdhd'}
Bundle 'vendor/bundlename', {'rev':'0.9.1'}

And then pass all the rev (does not matter if it is branch name, tag name or just a commit hash) to git rev-parse to get a valid commit hash. This way, other addtional args could be specified, eg, installed dir. like this:

Bundle 'vendor/bundlename', {'rev':'feature/branch-name','dir':'plugin-bundlename'}
Bundle 'vendor/bundlename', {'rev':'a83commit75hash2d5hd1bsdhd','dir':'colors-bundlename'}
Bundle 'vendor/bundlename', {'rev':'0.9.1','dir':'syntax-bundlename'}

The script will be installed at bundle/plugin-bundlename, I have add a pull request #200 about this.

What do you think about?

@Xerkus
Copy link
Author

Xerkus commented Aug 6, 2012

it breaks nothing.

Bundle 'vendor/bundlename', {'tree-ish':'feature/branch-name','dir':'plugin-bundlename'}

exactly the same as

Bundle 'vendor/bundlename', 'feature/branch-name', {'dir':'plugin-bundlename'}

As for your suggestion i don't see any point in that. There is no need to distinguish bundle types. If several different bundles from different repos have one name you can specify alternate install names (in my PR, not in original vundle).

@Xerkus
Copy link
Author

Xerkus commented Aug 6, 2012

This PR will not be accepted though.

@vheon
Copy link
Contributor

vheon commented Aug 7, 2012

@Xerkus why not?

@gmarik
Copy link
Contributor

gmarik commented Aug 13, 2012

@Xerkus thanks for the PR.

My proposal on specifying revision/sha1 looks like this:

Bundle 'vendor/bundlename treeish' or Bundle 'vendor/bundlename a12df23z' or Bundle 'vendor/bundlename@a12df23z'

There's a v branch addressing same problem(but it's incomplete and quite behind master now)

PS: Sorry for late update

@Xerkus
Copy link
Author

Xerkus commented Aug 13, 2012

@gmarik what is wrong with separate parameter?

Your suggestion can be implemented as alternate syntax though

@gmarik
Copy link
Contributor

gmarik commented Aug 13, 2012

@Xerkus for simplicity reasons, i think keeping convention Bundle 'name_spec', {options}
is a good idea for consistency reasons.

Also Bundle 'vendor/bundlename a12df23z' is the same as Bundle 'vendor/bundlename', {'v': 'a12df23z'}
And of course options override everything else Bundle 'vendor/bundlename a12df23z', {'v': 'master'}

Having revision/version as part of name is the simplest way possible to specify revision i think.
And those who don't like the short syntax can always use options.

@weynhamz
Copy link
Contributor

@gmarik I like this one Bundle 'vendor/bundlename@a12df23z', this is very clear and '@' explains everything. To avoiding the name ambiguous, I think Bundle 'vendor/bundlename@b:someting' for branch and Bundle 'vendor/bundlename@t:someting' for tag is a good idea.

And I really don't like Bundle 'vendor/bundlename treeish' or Bundle 'vendor/bundlename a12df23z' these two, it is not simple at all.

@Xerkus
Copy link
Author

Xerkus commented Aug 14, 2012

i can't say i like that much name and tree-ish in single parameter

Bundle 'Xerkus/vundle feature/refactor-git-treeish-support'

Bundle 'git@github.com:Xerkus/vundle.git@feature/refactor-git-treeish-support'

Bundle 'git@github.com:Xerkus/vundle.git feature/refactor-git-treeish-support'

It feels a bit messy.
vs

Bundle 'Xerkus/vundle', {'v': 'feature/refactor-git-treeish-support'}

vs

Bundle 'Xerkus/vundle', 'feature/refactor-git-treeish-support', {'v': 'master'}

And actual usage example:
https://github.com/EvanDotPro/vim-configuration/blob/master/.vimrc#L44

@gmarik public api is not that important in this PR though, can you comment on code changes?

UPD:
Probably ' @' will be better:

Bundle 'Xerkus/vundle @feature/refactor-git-treeish-support'

Bundle 'git@github.com:Xerkus/vundle.git @feature/refactor-git-treeish-support'

I'd prefer to keep 'repo', 'tree-ish' as one of the alternatives though

@greduan
Copy link

greduan commented Dec 2, 2012

I believe the following syntax/format is gonna allow backwards compatibility, and it's clear on what it does:

For a specific branch, with a specific commit:

Bundle 'vendor/reponame/branchname@SHA'

Without branch:

Bundle 'vendor/reponame@SHA'

Normal:

Bundle 'vendor/reponame'

It is uncluttered, clean, and it's backwards compatible.

@Xerkus @techlivezheng @gmarik What do you think?

@gmarik
Copy link
Contributor

gmarik commented Dec 2, 2012

@greduan lots of feedback from you recently )

as for Bundle 'vendor/reponame/branchname@SHA' why include /branchname part? As @sha is just enough.

Also, take a look at #241

@weynhamz
Copy link
Contributor

weynhamz commented Dec 2, 2012

@gmarik I think @greduan means either Bundle 'vendor/reponame/branchname' or Bundle 'vendor/reponame@SHA'.

@jdevera
Copy link
Contributor

jdevera commented Dec 2, 2012

I'm with @Xerkus in that the branch/commit as part of the main spec is not better than having it as an option. The way I see it is: The spec is what we clone. The options change the where, the how, and what we do with it afterwards, including checking something out.

@greduan
Copy link

greduan commented Dec 2, 2012

as for Bundle 'vendor/reponame/branchname@SHA' why include /branchname part? As @sha is just enough.

Ah, I didn't realize the @sha would not be necessary if we used a branch. So yes, @techlivezheng is correct on what I suggested.

I'm with @Xerkus in that the branch/commit as part of the main spec is not better than having it as an option. The way I see it is: The spec is what we clone. The options change the where, the how, and what we do with it afterwards, including checking something out.

@jdevera I do see your point, and it does make sense, I normally see with the same point of view. If we do go with that approach, I would suggest the following format then.

With a specific branch:

Bundle 'vendor/reponame', {'b':'branchname'}

And if we want to specify a SHA:

Bundle 'vendor/reponame', {'s':'sha'}

Of course, this would change to a valid VimL syntax.

Just want to point out, I have no preference, so it's all on you guys. Although I do think the first method is cleaner and more elegant.

@Peeja
Copy link

Peeja commented Dec 2, 2012

I'd prefer if, in either case, we didn't distinguish between tree-ish representations. There's no need for a different syntax for branches and sha IDs. We should be able to accept anything you can git-checkout, including tags and arbitrary refs. Git will resolve them all for us.

@greduan
Copy link

greduan commented Dec 2, 2012

@Peeja Right, but this is more of a power use really, so that we can stick with a certain version of a software for example, or only accept commits from a certain branch, stuff like that.

It's just an extra thing, that's very nice to have. :)

@Peeja
Copy link

Peeja commented Dec 2, 2012

I'm saying that, since we're going to just send that string to git-checkout anyway, there's no reason to use a different syntax for specifying a branch name or a sha (or a tag for that matter). They should all be the same. It's contradictory, for instance, to specify both---unless the branch and the sha are the same commit, in which case it's redundant.

@greduan
Copy link

greduan commented Dec 2, 2012

So, you're saying to not specify a branch and sha at the same time? I'm not clear on what you're saying. ❓

@jdevera
Copy link
Contributor

jdevera commented Dec 2, 2012

@greduan, @Peeja's point is that it's unnecessary complexity to add two parameter names (s and b in your example) when, what we would do with either is just call git checkout with it. The only reason you could really want them as separate parameters is if you wanted to use both with one bundle, but that would not make any sense, since you are, in the end, checking one thing out. For clarity, just think of the parameter name as checkout. Does that help?

@greduan
Copy link

greduan commented Dec 2, 2012

I think I understand now, thanks @jdevera.

Well, what would be a nice method to tell Vundle what branch to get? The extra parameters are really there just for download, after that they're not necessary. So what would be a good method to tell Vundle what SHA or branch to get?

@neersighted
Copy link

👍

@padde
Copy link

padde commented Mar 15, 2013

Why not copy from Bundler and use this syntax (loosely translated):

Bundle 'vendor/reponame', {'ref':'1a2b3c'}
Bundle 'vendor/reponame', {'branch':'experimental'}
Bundle 'vendor/reponame', {'tag':'v2.0'}

This should cover all cases. See how Bundler handles it: http://gembundler.com/v1.3/git.html

@Peeja
Copy link

Peeja commented Mar 16, 2013

I like the idea of being consistent with Bundler's syntax. Astute readers will notice that my support of that syntax contradicts what I said above about only needing one key name for all three.

I was going to say that Bundler treats all three as synonyms, and that it's worth doing the same for the same reason Bundler does it (because it reads sensibly) and to be consistent with Bundler, which most people are familiar with.

It turns out that Bundler treats :branch a little differently, and I'm not sure why. It appears to have to do with #local_override!, but I'm not sure what that does. Whatever it does, it might be useful to us, too. So let's look into that. :)

@vheon
Copy link
Contributor

vheon commented Apr 20, 2013

Is something like that in the roadmap for the main repo?

@jalcine
Copy link

jalcine commented Jul 13, 2013

:bump:

@bitboxer
Copy link

What is the status of this? I really would love to see this in vundle 😭

@MarcWeber
Copy link

#292 #253 #35 #189 (these are related)

@MarcWeber
Copy link

This PR is 2 years old. If you want to help implementing such feature for VAM we'll merge into master in no time.
This commit prepares what you need: allow passing the version as dictionary:
MarcWeber/vim-addon-manager@a59d517

vam#vcs#GitCheckoutFixDepth needs to be changed: If a version / hash /tag is passed as key in repository then no shallow clone should take place (or which depths to use !??).

Then a second git checkout command or such could be followed? Or can you pull targeting tags?
This is the related VAM issue: MarcWeber/vim-addon-manager#152

@jdevera
Copy link
Contributor

jdevera commented Apr 24, 2014

I'm looking at implementing this and it is updates that are the trickiest.

The command used in this PR, for example:

git checkout master && git pull --ff-only --all && git checkout $target

Does not work as intended. The --all flag is passed from pull to fetch, which means getting changes in all remotes. However, since it is master that is checked out, updates will only come into master. If you are locked to a commit or a tag, then it works, they are not mutable, but if you want to follow a branch, then this is not good enough.

We'd have to fetch, then checkout the branch and do a merge --ff-only. But that means we need special treatment for branches.

I think I'll close this PR and start a new one-and-only issue for this. Stay tuned.

@jdevera jdevera closed this Apr 24, 2014
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