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

Made git commands work despite values of GIT_DIR and GIT_WORK_TREE #523

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

Conversation

12qu
Copy link

@12qu 12qu commented Sep 28, 2014

The previous method for interacting with a bundle via git involved
cd'ing into the bundle directory then running the relevant git commands.
The problem with this approach is that if the environmental variables
GIT_DIR and GIT_WORK_TREE are set at this point then git will
preference their values over those implied by the current directory.
(This can cause trouble for vcsh users, for example.)

This patch uses the git arguments -C, --git-dir, and --work-tree
to specify the location of the bundle explicitly. This overcomes the
original problem since git gives these values precedence over GIT_DIR
and GIT_WORK_TREE.

The previous method for interacting with a bundle via git involved
cd'ing into the bundle directory then running the relevant git commands.
The problem with this approach is that if the environmental variables
`GIT_DIR` and `GIT_WORK_TREE` are set at this point then git will
preference their values over those implied by the current directory.
(This can cause trouble for vcsh users, for example.)

This patch uses the git arguments `-C`, `--git-dir`, and `--work-tree`
to specify the location of the bundle explicitly. This overcomes the
original problem since git gives these values precedence over `GIT_DIR`
and `GIT_WORK_TREE`.
@lucc
Copy link
Contributor

lucc commented Sep 28, 2014

The code looks good to me. I just ran a :PluginUpdate using this patch and it works well (does not seem to introduce errors). (I also set GIT_DIR and GIT_WORK_TREE to some nonexistent dirs for a test)

@jdevera
Copy link
Contributor

jdevera commented Mar 2, 2015

This is exciting. By removing the && we gain some compatibility with the Fish shell, and we could at some point stop telling people to set their shells to bash in vim.

I am concerned about the -C option, I seem to recall reading about it in a git's changelog, which would mean it's relatively new. Must check this.

@jdevera jdevera self-assigned this Mar 2, 2015
@lucc
Copy link
Contributor

lucc commented Mar 3, 2015

If you decide that we really can not use the -C option it can also be done with the --git-dir and --work-tree options alone.

@jdevera
Copy link
Contributor

jdevera commented Mar 3, 2015

The -C option was introduced in git 1.8.5 in November 2013. I'd say it's long enough, and especially worth it considering it would rid us of the &&. @gmarik do you object?

@gmarik
Copy link
Contributor

gmarik commented Mar 3, 2015

The -C option was introduced in git 1.8.5 in November 2013.

It's not that long ago.
This actually may cause a lot of problems for people.

@12qu
Copy link
Author

12qu commented Mar 4, 2015

@lucc Unfortunately I don't think this can be done with --git-dir and --work-tree alone, at least, not in a way that removes the &&. I tried the following code:

func! vundle#installer#expand_git_cmd(bundle, cmd) abort
  let git_dir = vundle#installer#shellesc(a:bundle.path().'/.git')
  let work_tree = vundle#installer#shellesc(a:bundle.path())
  let out = 'git --git-dir='.git_dir.' --work-tree='.work_tree.' '.a:cmd
  return out
endf

but receive an error fatal: /usr/lib/git-core/git-submodule cannot be used without a working tree. when the submodule update --init --recursive command is used. Apparently git submodule does not like --work-tree.

However, my original motivation for this patch was not to remove the && but rather to enable Vundle to work even when GIT_DIR and GIT_WORK_TREE are set. I still think there is reason to fix this, which may be done as follows:

func! vundle#installer#expand_git_cmd(bundle, cmd) abort
  let out = 'cd '.vundle#installer#shellesc(a:bundle.path()).' && git --git-dir=.git/ --work-tree=. '.a:cmd
  return out
endf

@jdevera
Copy link
Contributor

jdevera commented Mar 4, 2015

Right on. The ability to remove '&&' was an observed nice side effect, doesn't matter if we don't get it.

@jdevera
Copy link
Contributor

jdevera commented Mar 21, 2015

What if we, at the beginning of the installation, checked whether these vars are set, if so cache their values and unset them, and then at the end do the reverse? Seems to me like a simpler change.

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.

5 participants