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

Harden system calls to git #699

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Harden system calls to git #699

wants to merge 2 commits into from

Conversation

layus
Copy link

@layus layus commented Feb 26, 2016

This pull-request ensures that git uses the right directory.
In particular, it fixes #693 by overriding annoying environment variables.

⚠️ I am no vim guru, and I have not tested it, so this PR needs a careful review and probably many edits.


let git = ['git', '--git-dir='.gitdir, '--work-tree='.workdir]

return join(map(copy(git + args), 'vundle#installer#shellesc'))
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm... no idea really :-D

I copie this from elsewhere, just to be sure. I do not know how vim handles this, but it was used elsewhere, so better safe than sorry.

@layus
Copy link
Author

layus commented Feb 29, 2016

@klaernie, would you mind to test this in a vcsh environment ? Your feedback would help a lot.

@ryanoasis
Copy link
Member

I am no vim guru, and I have not tested it, so this PR needs a careful review and probably many edits.

This PR seems like a great idea IMHO 👍 . Yeah lots of testing and some of it is probably over my head but i will try to also help out testing and reviewing.

@layus
Copy link
Author

layus commented Feb 29, 2016

This looks like a duplicate of #523, I do not know how I missed this.
@12qu, @jdevera, @lucc, @gmarik: do you see any difference between the two implementations ?

I see that there could be an issue with submodules and the worktree option...

@klaernie
Copy link

klaernie commented Mar 1, 2016

@layus: I'll try it out after work.

@@ -357,12 +356,37 @@ endf
" return -- A 15 character log sha for the current HEAD
" ---------------------------------------------------------------------------
func! s:get_current_sha(bundle)
let cmd = 'cd '.vundle#installer#shellesc(a:bundle.path()).' && git rev-parse HEAD'
let cmd = vundle#installer#shellesc_cd(cmd)
let cmd = s:make_sync_command(a:bundle, ['rev-parse', 'HEAD'])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this work? It looks like the function expects totaly different args.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... No, of course no. This must be an autocomplete typo.

Le 1 mars 2016 07:18:40 UTC+01:00, Lucas Hoffmann [email protected] a écrit :

@@ -357,12 +356,37 @@ endf
" return -- A 15 character log sha for the current HEAD

"

func! s:get_current_sha(bundle)

  • let cmd = 'cd '.vundle#installer#shellesc(a:bundle.path()).' &&
    git rev-parse HEAD'
  • let cmd = vundle#installer#shellesc_cd(cmd)
  • let cmd = s:make_sync_command(a:bundle, ['rev-parse', 'HEAD'])

Does this work? It looks like the function expects totaly different
args.


Reply to this email directly or view it on GitHub:
https://github.com/VundleVim/Vundle.vim/pull/699/files#r54526969

@layus
Copy link
Author

layus commented Mar 1, 2016

I have now tested my code and fixed many syntax issues. See 4629700 for details.
I also tested within vcsh, which works now. All my tests were on (Arch)Linux.
PS: I have no windows at hand.

@layus
Copy link
Author

layus commented Mar 2, 2016

@klaernie, Did it work for you ?

@klaernie
Copy link

klaernie commented Mar 3, 2016

@layus worked perfectly. Although I had to try three times until I noticed that my temporary change to my vimrc needed to uncomment the original bootstrap git clone line..

@layus
Copy link
Author

layus commented Mar 3, 2016

I also tested deletion and update. even the call to submodules seems to work fine.
@ryanoasis, what can I further do to get this merged ?

@ryanoasis
Copy link
Member

@layus I am not really sure. I would like to test this out myself as well. If you have any sort of test plan else I'll just go through ad hoc as best as I can 😊

I have mostly just been merging the low hanging fruit but I do plan on looking at these more impactful/non-trivial PRs as well

I don't know when exactly I can get to more PRs right know but this project is back on my radar so I am trying to help out more... cheers.

@layus
Copy link
Author

layus commented Mar 7, 2016

Yep, but at the same time this is quite a trivial change. Use git arguments instead of cd'ing into a repo.

@ryanoasis
Copy link
Member

@layus Yeah sorry, actually for a code change you are correct this could definitely be described as trivial.

I am just trying to be extra careful (maybe overly so) about merging things in because I am not extremely familiar with the project code and/or impact. Also I would not want to break Vundle since it is still used by a lot of vim users.

@layus
Copy link
Author

layus commented Mar 7, 2016

@ryanoasis, what do you mean by 'still used' ? Is there some acknowledged successor ? I have been an happy user ever since I discovered vim plugins :-). Should I move to something else ?

@ryanoasis
Copy link
Member

Oh I think I was refering to NeoBundle (started as a fork of Vundle) but seems like active development has stopped on it and it will only get bug fixes in favor of dein.

There are also some other ones with some cool ideas but I haven't checked them out yet: vim-plug

I still use Vundle btw.

@ryanoasis ryanoasis modified the milestone: 0.10.3 Mar 16, 2016
@ryanoasis
Copy link
Member

Hey I promise I haven't forgotten about these PRs and I think so far I will target these in a milestone I created.

I have tried to reach out to @gmarik to see about workflow, etc. I plan on continuing on even without hearing back and just hoping I do things correctly 😅

@layus
Copy link
Author

layus commented Mar 28, 2016

Nice ! Thanks ! I promise I will test the new release before it is published :-)

@ryanoasis
Copy link
Member

Just an update, I finally got around to merging these into a release branch but I need to resolve some conflicts regard #699 and #684 😄

ryanoasis added a commit that referenced this pull request Apr 22, 2016
Harden system calls to git

Fixes from PR #684 (cameris/master) re-applied to new function 's:make_git_command'

Conflicts:
	autoload/vundle/installer.vim
	autoload/vundle/scripts.vim
@ryanoasis
Copy link
Member

Finally got around to getting these into branch/release 0.10.3

Have only cursory tested :VundleInstall, :VundleUpdate, :VundleSearch, and viewed the vundle log.

Side note, not sure when it happened (seems to even happen on master) but during my testing :VundleUpdate is VERY SLOW... maybe it is just on my end?

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.

vcsh support
4 participants