Use of GIT_REPO env-var for setting Repo path in 2.0.9

[ethereon]

This change in 2.0.9 is a bit problematic:

• Even if a path is explicitly provided to the constructor, the GIT_DIR environment variable is used instead
• For things like git hooks, GIT_DIR is set to .git by default. This will result in self.git_dir being set to .git (os.path.abspath instead of os.path.normpath would fix this). Consequently, subsequent commands fails (eg: repo.git.status())

[ankostis]

Even if a path is explicitly provided to the constructor, the GIT_DIR environment variable is used instead

That should be the expected behavior, env-variables taking precedence over code, correct?

For things like git hooks, GIT_DIR is set to '.git' by default.

Can you clarify a bit on that?
Who sets that by default?
By looking at the code I cannot find gitpython doing such a thing before invoking a hook - actually nowhere else the GIT_DIR var is modified, it is used as a read-only variable.

[ethereon]

That should be the expected behavior, env-variables taking precedence over code, correct?

I'm not so sure. Code is more explicit than an implicit override by an environment variable. Also, with the env var taking precedence over code, you can't do things like have multiple Repo instances with different paths.

Who sets that by default?

git sets this. For instance, if you have a pre-commit hook at [your-repo]/.git/hooks/pre-commit.

The following pre-commit script would have previously worked on 2.0.8 but not 2.0.9:

#!/usr/bin/env python
import git
repo = git.Repo()
print('Status is {}'.format(repo.git.status()))

[cheshirekow]

Here's a demonstration of GIT_DIR being set by git during a pre-commit hook. git version 2.10.1.

~$ cd /tmp/
/tmp$ mkdir test
/tmp$ cd test/
/tmp/test$ git init
Initialized empty Git repository in /tmp/test/.git/
/tmp/test$ nano .git/hooks/pre-commit
/tmp/test$ chmod +x .git/hooks/pre-commit
/tmp/test$ cat .git/hooks/pre-commit
env | grep GIT > /tmp/hook_env.txt
/tmp/test$ echo "hello" >> a.txt
/tmp/test$ git add a.txt 
/tmp/test$ env | grep GIT
/tmp/test$ git commit -m "add a file"
[master 18e3929] add a file
 1 file changed, 1 insertion(+)
/tmp/test$ cat /tmp/hook_env.txt 
GIT_DIR=.git
GIT_AUTHOR_DATE=@1476735917 -0700
GIT_EDITOR=:
GIT_INDEX_FILE=.git/index
GIT_AUTHOR_NAME=cheshirekow
GIT_PREFIX=
[email protected]

[ankostis]

Indeed 26bb778 introduced a bug in the Repo.__init__(), because the expansion of the path happens earlier, before the use of the GIT_DIR variable.

[ankostis]

Concerning @cheshirekow useful example, it demonstrates that the git command sets GIT_DIR, so any hook-scripts relying on the gitpython library, indeed, would no work (until #537 is merged).

But there is another issue here:

• git 2.10.1 cmd set 7 env-vars for executing hook-commits:

  GIT_DIR
  GIT_AUTHOR_DATE
  GIT_EDITOR
  GIT_INDEX_FILE
  GIT_AUTHOR_NAME
  GIT_PREFIX
  GIT_AUTHOR_EMAIL
  

• the gitpython library sets [edit]probably only 2 env-vars:

  GIT_INDEX_FILE
  GIT_EDITOR`
  

• The git-docs are not explicit on that, as confirmed by many sources[1][2][3].

• Is that a bug, and should gitpython replicate git's behavior for hook env-vars?
  Or we will be hunting a moving target?
  (@Byron any input on that?)

[1] https://longair.net/blog/2011/04/09/missing-git-hooks-documentation/
[2] https://www.digitalocean.com/community/tutorials/how-to-use-git-hooks-to-automate-development-and-deployment-tasks
[3] http://stackoverflow.com/questions/5531309/reset-hard-on-git-push

[ankostis]

@ethereon on #537 wrote:

The environment variable taking precedence over the specified path seems error prone and undesirable. For instance, with GIT_DIR set, you can no longer do something like this:

repo_1 = git.Repo('/path/to/repo1')
repo_2 = git.Repo('/path/to/repo2')

I think GIT_DIR as a fallback makes sense (if path is None for instance). However, is there any reason to prefer it if an explicit path is provided?

[ankostis]

I have a conflicting gut-feeling there...

The general idea for the priority of the configuration params is borrowed from the way command-line tools configure themselves; usually there is a list of configuration sources (rough description follows):

1. source-code "default" kwds
2. source-code kwds
3. system conf-file
4. user conf-file
5. env-vars
6. cmd-line args

So based on the list above, 5. env-vars override 2. source-code kwds which is at the bottom.

BUT gitpython is a not a Cmd-line tool but a Library, and function arguments (1. & 2.) can be though of as the equivalent of the 6. cmd-line args, so they must have the top-most priority, and what @ethereon asks makes sense. [edit:] That becomes even more obvious with interactive python-REPL, as it is hinted by ethereon's latest example.

Taking a strict stance on that would mean that env-vars (such as GIT_DIR) are not expanded by the the library-code, and they should be left for the end-user to decide. Unfortunately there are places where this decision (for the the var-expansion and override) happens internally, in code-paths that are not under the direct control of the end-user, so this "strict stance" is too idealistic to be functioning.

Any ideas on this?

[Byron]

@ethereon Thanks for all the effort you put into this issue ! Personally I believe that the current behaviour is unintuitive, as the path specifically provided of course has the final say. Ideally, GitPython would not have started adding so much magic as to expand GIT_DIR itself internally, maybe it has gotten off-track. Also I am unsure which intend the offending commit might have had, and I would be fine using GIT_DIR only if path is None, if that is needed to keep things in working order.

@ankostis regarding the env-vars that are set when GitPython executes hooks: Maybe this is another instance of GitPython getting into its own way as it now has to chase a flying, and in this case, undocumented bullet. However, to me it looks like another issue that should probably be addressed independently of this one if there is any need.

[ankostis]

...and I would be fine using GIT_DIR only if path is None,

Done, with a slight twist: env-var used if path is None or empty.

... [Hook env-vars] should probably be addressed independently of this one if there is any need.

Of course; and is not urgent at all.

... the current behaviour is unintuitive, as the path specifically provided of course has the final say.

I would say that this depends on situation:

• when you have an old gitpython-script doing the same stuff and you want to quik'n dirty modify it's repo without editing its code, it would make sense to set just the env-var.
• For interactive usage in a REPL is totaly surprising for the var to take over what you type.

The solution you both described is respecting both cases, and have implemented in 5fac8d4.