Harden system calls to git by layus · Pull Request #699 · VundleVim/Vundle.vim

layus · 2016-02-26T21:54:12Z

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~~.

layus · 2016-02-26T22:21:15Z

autoload/vundle/installer.vim

+
+  let git = ['git', '--git-dir='.gitdir, '--work-tree='.workdir]
+
+  return join(map(copy(git + args), 'vundle#installer#shellesc'))


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 · 2016-02-29T15:08:30Z

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

ryanoasis · 2016-02-29T15:44:03Z

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 · 2016-02-29T16:49:10Z

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 · 2016-03-01T04:35:07Z

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

lucc · 2016-03-01T06:18:19Z

autoload/vundle/installer.vim

 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.

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

Le 1 mars 2016 07:18:40 UTC+01:00, Lucas Hoffmann notifications@github.com 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 · 2016-03-01T08:52:53Z

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 · 2016-03-02T15:06:59Z

@klaernie, Did it work for you ?

klaernie · 2016-03-03T00:13:31Z

@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 · 2016-03-03T08:14:12Z

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 · 2016-03-04T02:42:28Z

@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 · 2016-03-07T15:20:12Z

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

ryanoasis · 2016-03-07T17:53:01Z

@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 · 2016-03-07T18:16:53Z

@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 · 2016-03-07T18:47:24Z

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 · 2016-03-28T18:22:30Z

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 · 2016-03-28T22:15:43Z

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

ryanoasis · 2016-04-14T14:11:25Z

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 😄

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 · 2016-04-22T22:22:28Z

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?

Harden system calls to git

87eb7c2

layus reviewed Feb 26, 2016
View reviewed changes

lucc reviewed Mar 1, 2016
View reviewed changes

Fix syntax (and other) issues

4629700

layus force-pushed the master branch from b38cb68 to 4629700 Compare March 1, 2016 08:48

ryanoasis modified the milestone: 0.10.3 Mar 16, 2016

ryanoasis mentioned this pull request Apr 14, 2016

Added variables for git, curl & wget executables. #684

Open

ryanoasis mentioned this pull request Apr 23, 2016

Some git commands were not properly updated. #719

Merged


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

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

Conversation

layus commented Feb 26, 2016

Uh oh!

layus Feb 26, 2016

Choose a reason for hiding this comment

Uh oh!

layus commented Feb 29, 2016

Uh oh!

ryanoasis commented Feb 29, 2016

Uh oh!

layus commented Feb 29, 2016

Uh oh!

klaernie commented Mar 1, 2016

Uh oh!

lucc Mar 1, 2016

Choose a reason for hiding this comment

Uh oh!

layus Mar 1, 2016

Choose a reason for hiding this comment

"

Uh oh!

layus commented Mar 1, 2016

Uh oh!

layus commented Mar 2, 2016

Uh oh!

klaernie commented Mar 3, 2016

Uh oh!

layus commented Mar 3, 2016

Uh oh!

ryanoasis commented Mar 4, 2016

Uh oh!

layus commented Mar 7, 2016

Uh oh!

ryanoasis commented Mar 7, 2016

Uh oh!

layus commented Mar 7, 2016

Uh oh!

ryanoasis commented Mar 7, 2016

Uh oh!

ryanoasis commented Mar 28, 2016

Uh oh!

layus commented Mar 28, 2016

Uh oh!

ryanoasis commented Apr 14, 2016

Uh oh!

ryanoasis commented Apr 22, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants