If diff.external is set, diffing via API fails silently as external tool isn't understood #1828

Can you give instructions to get the repository to the state where those statements produce those results? I've tried to reproduce this on Ubuntu and Windows, and so far I've been unable to get an empty list with create_patch=True in a situation where it is nonempty without it.

For example, on Ubuntu 22.04 LTS with git 2.34.1 using Python 3.12.1 with GitPython 3.1.42 installed in a virtual environment, in a repository consisting of a single commit of a one-line file to which a second line is appended and staged and a third line is appended and not staged, all the calls you showed gave one-element results, with no zero-element results:

ek@Glub:~/tmp$ mkdir investigate-1828
ek@Glub:~/tmp$ cd investigate-1828/
ek@Glub:~/tmp/investigate-1828$ git init .
Initialized empty Git repository in /home/ek/tmp/investigate-1828/.git/
ek@Glub:~/tmp/investigate-1828 (main #)$ echo .venv >.gitignore
ek@Glub:~/tmp/investigate-1828 (main #%)$ git add .
ek@Glub:~/tmp/investigate-1828 (main +)$ git commit -m 'Initial commit'
[main (root-commit) 66d6bcc] Initial commit
 1 file changed, 1 insertion(+)
 create mode 100644 .gitignore
ek@Glub:~/tmp/investigate-1828 (main)$ echo __pycache__/ >>.gitignore
ek@Glub:~/tmp/investigate-1828 (main *)$ git add .
ek@Glub:~/tmp/investigate-1828 (main +)$ echo '# third line' >>.gitignore
ek@Glub:~/tmp/investigate-1828 (main *+)$ git show
commit 66d6bcc368351bd23f8cea1bb43113ef79110a99 (HEAD -> main)
Author: Eliah Kagan <degeneracypressure@gmail.com>
Date:   Sun Feb 18 22:04:28 2024 -0500

    Initial commit

diff --git a/.gitignore b/.gitignore
new file mode 100644
index 0000000..1d17dae
--- /dev/null
+++ b/.gitignore
@@ -0,0 +1 @@
+.venv
ek@Glub:~/tmp/investigate-1828 (main *+)$ git diff --staged
diff --git a/.gitignore b/.gitignore
index 1d17dae..3367433 100644
--- a/.gitignore
+++ b/.gitignore
@@ -1 +1,2 @@
 .venv
+__pycache__/
ek@Glub:~/tmp/investigate-1828 (main *+)$ git diff
diff --git a/.gitignore b/.gitignore
index 3367433..a2b3f2c 100644
--- a/.gitignore
+++ b/.gitignore
@@ -1,2 +1,3 @@
 .venv
 __pycache__/
+# third line
ek@Glub:~/tmp/investigate-1828 (main *+)$ python3.12 -m venv .venv
ek@Glub:~/tmp/investigate-1828 (main *+)$ . .venv/bin/activate
(.venv) ek@Glub:~/tmp/investigate-1828 (main *+)$ pip install GitPython
Collecting GitPython
  Obtaining dependency information for GitPython from https://files.pythonhosted.org/packages/67/c7/995360c87dd74e27539ccbfecddfb58e08f140d849fcd7f35d2ed1a5f80f/GitPython-3.1.42-py3-none-any.whl.metadata
  Downloading GitPython-3.1.42-py3-none-any.whl.metadata (12 kB)
Collecting gitdb<5,>=4.0.1 (from GitPython)
  Obtaining dependency information for gitdb<5,>=4.0.1 from https://files.pythonhosted.org/packages/fd/5b/8f0c4a5bb9fd491c277c21eff7ccae71b47d43c4446c9d0c6cff2fe8c2c4/gitdb-4.0.11-py3-none-any.whl.metadata
  Using cached gitdb-4.0.11-py3-none-any.whl.metadata (1.2 kB)
Collecting smmap<6,>=3.0.1 (from gitdb<5,>=4.0.1->GitPython)
  Obtaining dependency information for smmap<6,>=3.0.1 from https://files.pythonhosted.org/packages/a7/a5/10f97f73544edcdef54409f1d839f6049a0d79df68adbc1ceb24d1aaca42/smmap-5.0.1-py3-none-any.whl.metadata
  Using cached smmap-5.0.1-py3-none-any.whl.metadata (4.3 kB)
Downloading GitPython-3.1.42-py3-none-any.whl (195 kB)
   ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 195.4/195.4 kB 1.7 MB/s eta 0:00:00
Using cached gitdb-4.0.11-py3-none-any.whl (62 kB)
Using cached smmap-5.0.1-py3-none-any.whl (24 kB)
Installing collected packages: smmap, gitdb, GitPython
Successfully installed GitPython-3.1.42 gitdb-4.0.11 smmap-5.0.1

[notice] A new release of pip is available: 23.2.1 -> 24.0
[notice] To update, run: pip install --upgrade pip
(.venv) ek@Glub:~/tmp/investigate-1828 (main *+)$ python
Python 3.12.1 (main, Dec 10 2023, 15:07:36) [GCC 11.4.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from git import Repo
>>> repo = Repo(".")
>>> repo.index.diff("HEAD")
[<git.diff.Diff object at 0x7f7ae22cacb0>]
>>> repo.index.diff("HEAD", create_patch=True)
[<git.diff.Diff object at 0x7f7ae22cac20>]
>>> repo.index.diff(None)
[<git.diff.Diff object at 0x7f7ae22cab90>]
>>> repo.index.diff(None, create_patch=True)
[<git.diff.Diff object at 0x7f7ae22cacb0>]
>>> repo.index.diff("HEAD", create_patch=True, R=True)
[<git.diff.Diff object at 0x7f7ae22cab00>]
>>> repo.head.commit.diff(None, create_patch=True)
[<git.diff.Diff object at 0x7f7ae22cac20>]
>>> repo.head.commit.diff(None)
[<git.diff.Diff object at 0x7f7ae22cae60>]
>>> repo.head.commit.diff()
[<git.diff.Diff object at 0x7f7ae22cad40>]
>>> repo.head.commit.diff(create_patch=True)
[<git.diff.Diff object at 0x7f7ae22cab00>]

So my guess is that this may only happen under particular conditions, such as when a repository has a particular combination of committed, staged, and unstaged changes, or maybe only with particular versions of Git, of Python, etc.

Hi, thank you so much for the answer!

I am using Python 3.8 and git version 2.39.3 (Apple Git-145) on Macbook Air M2

Here are steps to reproduce:

mkdir investigate-1828
git --version
# git version 2.39.3 (Apple Git-145)
cd investigate-1828/
git init .
# Initialized empty Git repository in /Users/cantaslicukur/investigate-1828/.git/
echo .venv >.gitignore
git add .
git status
# On branch main
#
# No commits yet
#
# Changes to be committed:
#  (use "git rm --cached <file>..." to unstage)
#	new file:   .gitignore

git commit -m 'Initial commit'
# [main (root-commit) 480924f] Initial commit
# 1 file changed, 1 insertion(+)
# create mode 100644 .gitignore

echo __pycache__/ >>.gitignore
git add .

echo '# third line' >>.gitignore

git show
# commit 480924f1dda82f54472d28809db33451deed18ea (HEAD -> main)
# Author: Can Taşlıçukur <can.taslicukur@ozu.edu.tr>
# Date:   Mon Feb 19 17:47:03 2024 +0300
#
#    Initial commit
#
# diff --git a/.gitignore b/.gitignore
# new file mode 100644
# index 0000000..1d17dae
# --- /dev/null
# +++ b/.gitignore
# @@ -0,0 +1 @@

python --version
# Python 3.8.18
python -m venv .venv
. .venv/bin/activate
pip install GitPython
# Collecting GitPython
#   Downloading GitPython-3.1.42-py3-none-any.whl (195 kB)
#      ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 195.4/195.4 kB 2.4 MB/s eta 0:00:00
# Collecting gitdb<5,>=4.0.1
#   Downloading gitdb-4.0.11-py3-none-any.whl (62 kB)
#      ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 62.7/62.7 kB 3.2 MB/s eta 0:00:00
# Collecting smmap<6,>=3.0.1
#   Downloading smmap-5.0.1-py3-none-any.whl (24 kB)
# Installing collected packages: smmap, gitdb, GitPython
# Successfully installed GitPython-3.1.42 gitdb-4.0.11 smmap-5.0.1

# [notice] A new release of pip is available: 23.0.1 -> 24.0
# [notice] To update, run: pip install --upgrade pip

python
# Python 3.8.18 | packaged by conda-forge | (default, Dec 23 2023, 17:25:47)
# [Clang 16.0.6 ] on darwin
# Type "help", "copyright", "credits" or "license" for more information.
>>> from git import Repo
>>>
>>> repo = Repo(".")
>>> print(repo.index.diff("HEAD"))
[<git.diff.Diff object at 0x1013a0f70>]
>>> print(repo.index.diff("HEAD", create_patch=True))
[]
>>>
>>> print(repo.index.diff(None))
[<git.diff.Diff object at 0x1013a0e50>]
>>> print(repo.index.diff(None, create_patch=True))
[]
>>> print(repo.head.commit.diff(None, create_patch=True))
[]
>>> print(repo.head.commit.diff(None))
[<git.diff.Diff object at 0x1013a0ca0>]
>>>
>>> print(repo.head.commit.diff())
[<git.diff.Diff object at 0x1013a0dc0>]
>>> print(repo.head.commit.diff(create_patch=True))
[]

Update: I have tried running the steps above with Python 3.12.2 and git version 2.43.2 (installed via brew). I get the same results :(

All right! I found the issue! Good news, it is my fault :D

I just remembered that I use difftastic and in my ~/.gitconfig, I have the following:

[diff]
    external = difft

I have deleted this from my ~/.gitconfig and gitPython works fine!

Great to hear it's resolved!

If you are interested, you could submit a PR with a fix, so such workarounds aren't required anymore. It should be quite easy to override this setting using environment variables when launching the git-diff process from within GitPython.

With Python 3.12.3 and GitPython-3.1.44 (git version 2.49.0), running the example above by can-taslicukur, I still get "empty list" instead of a patch.

I verified that my .gitconfig does not set anything under [diff].

With Python 3.12.3 and GitPython-3.1.44 (git version 2.49.0), running the example above by can-taslicukur, I still get "empty list" instead of a patch.

I verified that my .gitconfig does not set anything under [diff].

I’m not entirely sure, but I suspect there might be something in your setup that's generating git diff patches in a format different from the default diff engine. The root issue I encountered was that GitPython couldn’t parse these non-standard patches generated by difftastic, so it ended up returning an empty list. The fix was to add --no-ext-diff argument to the GitPython's git call. Could you check whether the patches produced in your environment through git CLI match the default output of git diff? That might help narrow down the problem.

This reminds me: It should be possible to launch these Git invocations without pulling in global and system configuration. This is how it's done: https://github.com/GitoxideLabs/gitoxide/blob/828e9035a40796f79650cf5e3becb8d8e5e29883/tests/tools/src/lib.rs#L649-L650

Many of the invoked commands would probably be better when only seeing the local repository configuration.

Many of the invoked commands would probably be better when only seeing the local repository configuration.

This could be reasonable in some situations, I think when one or more of the following apply:

1. The local and worktree scopes are also being suppressed.
2. No interaction with a local repository is possible (really, this is a special case of 1).
3. It is being done only in a test suite.
4. It is being done only in code meant only for use in test suites (as in gix-testtools).
5. The user has explicitly configured or otherwise requested this (rare).
6. Specific well-understood scenarios (rare).

Otherwise, I would be reluctant to default to suppressing the system and global scopes when invoking git commands, because doing so can introduce errors or decrease safety.

It can break any git operation that (even if only in principle) reads configuration, because global safe.directory allowlists will not be honored. In some subcommands, the resulting failure is non-obvious, because it does not report anything related safe.directory, nor issue any "dubious ownership" message, instead behaving the same as if there were no repository. diff is such a subcommand:

ek@Kip:~/src$ git version
git version 2.49.0
ek@Kip:~/src$ git init shared-repo
Initialized empty Git repository in /home/ek/src/shared-repo/.git/
ek@Kip:~/src$ cd shared-repo
ek@Kip:~/src/shared-repo (main #)$ echo 'first line' >file
ek@Kip:~/src/shared-repo (main #%)$ git add file
ek@Kip:~/src/shared-repo (main +)$ echo 'second line' >>file
ek@Kip:~/src/shared-repo (main *+)$ sudo chown -R ek2 .
[sudo] password for ek:

ek@Kip:~/src/shared-repo$ git diff
warning: Not a git repository. Use --no-index to compare two paths outside a working tree
usage: git diff --no-index [<options>] <path> <path>

Diff output format options
    -p, --patch           generate patch
    -s, --no-patch        suppress diff output
...

ek@Kip:~/src/shared-repo[129]$ git -c safe.directory=~/src/shared-repo diff
diff --git a/file b/file
index 08fe272..06fcdd7 100644
--- a/file
+++ b/file
@@ -1 +1,2 @@
 first line
+second line

ek@Kip:~/src/shared-repo$ git config --global --add safe.directory ~/src/shared-repo
ek@Kip:~/src/shared-repo (main *+)$ git diff
diff --git a/file b/file
index 08fe272..06fcdd7 100644
--- a/file
+++ b/file
@@ -1 +1,2 @@
 first line
+second line
ek@Kip:~/src/shared-repo (main *+)$ GIT_CONFIG_GLOBAL=/dev/null git diff
warning: Not a git repository. Use --no-index to compare two paths outside a working tree
usage: git diff --no-index [<options>] <path> <path>

Diff output format options
    -p, --patch           generate patch
    -s, --no-patch        suppress diff output
...

The bigger problem is that it can decrease safety because the user may have set a more secure configuration than the default. As one example of this kind of thing, git-config(1) suggests:

If you do not use bare repositories in your workflow, then it may be beneficial to set safe.bareRepository to explicit in your global config. This will protect you from attacks that involve cloning a repository that contains a bare repository and running a Git command within that directory.

A user who sets safe.bareRepository to explicit when using a version of Git that supports safe.bareRepository can, when performing operations that are documented to use the installed Git, reasonably expect that safe.bareRepository will be honored in those operations. But if the user sets it in the global scope as the official documentation suggests doing, and the global scope is suppressed, then safe.bareRepository would not be honored.

(It is usually set in the global scope because, like safe.directory, it has no effect when set in unprotected scopes. These variables are used from the global and system scopes and, on macOS, the outermost "unknown" scope that is higher than the system scope but also suppressed by GIT_CONFIG_NOSYSTEM; they are not used from the local and worktree scopes.)

safe.bareRepository is just one example. There are various other configuration variables that make sense to set in the global scope or higher that a user may rely on to be willing to do something that they would otherwise avoid out of security concerns, such as allowing fewer protocols than are allowed by default.

Thanks for chiming in, and even though initially I was sceptical ("How could anything be a problem for git diff?"), it's clear that not being able to open the repository at all may be a problem :D.

Something more suitable would probably be to override the configuration that a particular function needs to control. git diff could, for instance, assure that external diff programs won't be run, and that the various settings are set to their defaults.

Maybe that would be more feasible?

Overriding specific configuration variables by setting them in the command scope with -c should never cause the kind of problems described in #1828 (comment). Configuration variables that we aren't overriding will still be in place as configured.

This is the case whether the variables are overridden for specific commands or for all commands. The important differences are that the command scope is used, and that the only configuration variables whose preexisting values are suppressed are the ones we are intentionally overriding. This is to say that it's the variables that should be specific; we don't want to remove other variables besides those we are overriding.

It may also be that some of them should be passed only when using particular (sub)commands – and as I argue below, I think it's important only ever to do this when the commands are being run in particular ways – but all that is largely separate from the concerns articulated in #1828 (comment).

I think that whether we should do this depends on how the command is being run (including in the case of git diff). This may already be what you mean, but I am not certain.

When code outside GitPython uses a Git instance directly: not by default

I think we should not, by default, set extra configuration variables when a user runs a subcommand directly on a Git instance – whether through an instance the caller manages, or an instance obtained as the git attribute of a Repo object.

The current effect of invoking a dynamic method of a Git instance, when the user has not done anything special to customize it, is to run a git command whose arguments are formed by applying fairly simple transformation rules to the name, positional arguments, and non-special keyword arguments of the dynamic method, as implemented in _call_process. Relatedly, when using the execute method directly, the specific command run is not augmented with additional configuration options.

I think users expect Git objects to keep working that way. Changing it seems like it would always be a breaking change. I think there is also a good design reason for it to work this way. One of the benefits of GitPython is that it makes it easy to run specific git commands from a Python program.

Of course, this is only about the default behavior. Users can already cause dynamic method calls, invoked under the hood via _call_process, to pass extra -c options. They can do this either persistently by calling set_persistent_git_options with keyword arguments representing the variables to be set, or only for the next call by calling their Git instance itself with such keyword arguments. (#2029 shall, among other changes, add more ways this happens, and even insert extra arguments in direct execute invocations, but still only by opting in.)

When other GitPython facilities perform a specific function, even via Git: very often yes

In addition to being only about preserving default behavior, the above also only applies to the behavior of the Git class. If changing what options are specified from other code – such as code in Repo, IndexFile, or any of the classes representing Git objects or Git references – improves correctness, then I am not arguing against that.

(There may be particular ways that could break compatibility, which we should consider on a case-by-case basis, but it's not the same as changing the behavior of the Git class's dynamic methods or execute method, which are far more general and thus almost surely have uses that would be broken.)

As applied to git diff

I think the specific case of git diff is consistent with this distinction.

• Where g is a Git instance and r is a Repo instance, I would very much expect g.diff(…) or r.git.diff(…) to honor any configured external diff tool (when not overridden by the caller), and I would be surprised if changing this would not break some production code that uses GitPython.
• But I have no such expectation of r.index.diff(…), which produces a DiffIndex listing Diff objects, to use an external diff tool. With most external tools, such an operation is unlikely to succeed, and arguably does not even conceptually make sense. (I expect that r.index.diff(…) does not, by default, override any configured smudge and clean filters, but I wouldn't expect it to use a custom external diff command.)

This is also consistent with the preexisting difference in behavior, both before and after #1832 fixed this issue. Diffable.diff, which IndexFile inherits, carries several other such customizations, which happen only when that diff method is used (or if another caller applies similar customizations itself). For example, --abbrev=40 and --full-index are passed.

I agree, and also only ever thought that such overrides would be passed by the caller, when the caller knows that the invocation is affected by certain configuration that needs to be controlled for that reason.

I had wondered if part of what you were thinking would include having g = Git(…); g.command(…) adjust -c options automatically per-command. Even knowing now that this is not part of what you meant, it's an interesting idea (albeit not something that Git should do by default).

I do admit that after reading your comment above I thought that context managers could probably be used to conveniently enforce settings for commands that follow. That way, existing code wouldn't have to be adjusted (beyond re-indentation) and it's clear what's affected by these settings.

It could be made quite nicely, while adding a useful features to everyone who uses repo.git directly.

that context managers could probably be used to conveniently enforce settings for commands that follow.

Is this something that would apply to uses of a particular Git instance, or to all Git instances?

That is, would the context manager be:

1. Conceptually on the instance (even if created in some other way besides a new instance method, since new public instance methods could clash with dynamic methods in use for people's custom git commands)? This could have the effect of modifying its _persistent_git_options. Or maybe it would modify a newly introduced instance attribute that would hold a stack of items, the topmost of which would be used in lieu of _persistent_git_options. Either way, the modification would be on __enter__ and would be undone on __exit__. Or…

2. Independent of any particular instance, so that any use of any number of Git instances between the context manager's __enter__ and __exit__ would be affected? (Even though more global, this could probably also be done without introducing any new sources of thread unsafety, and in a way that probably works as people would expect with asynchronous code, by using contextvars.)

In either case, decisions would have to be made about how it should interact with preexisting mechanisms of specifying configuration. For example, suppose a custom_git_options method were introduced, somewhat analogously to custom_environment (though, echoing a concern above, this would break any existing usage of a custom git custom-git-options command, which seems like a plausible thing):

import git

g = git.Git(".")
g.set_persistent_git_options(c="core.abbrev=20")
g(c="core.abbrev=40")  # Usually comes just before a dynamic method call, but not always.
with g.custom_git_options(c="core.abbrev=30"):
    print(g.log(oneline=True, n=1))  # Does this print a 30 or 40 character hash?
print(g.log(oneline=True, n=1))  # Does this print a 20 or a 40 character hash?

These are valid concerns and I didn't think that far. But independently of difficulties of adding a new method, I'd think that the context manager will affect only a specific instance.
If one really wanted to, one could do something like with repo.git_options("core.foo" = "bar") as git: git.foo(), which doesn't seem to unergonomic. All names mentioned here are examples.

If one really wanted to, one could do something like with repo.git_options("core.foo" = "bar") as git: git.foo()

If, in the code of intended use cases, it's okay to replace uses of the original Git instance with uses of a newly introduced variable referring to some other object, then we may actually not need a context manager at all. An alternative would be to introduce a view of the Git instance that adds options.

Currently, calling the Git instance directly with keyword arguments sets options to be passed before the subcommand, which are used and cleared by _call_process next time a dynamic method is called:

But it does not currently accept positional arguments, so maybe something like this can be done while maintaining full compatibility (where the implementation of the important GitView class is omitted because it would require design decisions; docstrings, and the In class inheriting enum.Enum, are omitted for brevity; but ... is meant literally as @overload is only for static type checkers):

@overload
def __call__(self, scope: Literal[In.NEXT_CALL], **kwargs: Any) -> Self: ...

@overload
def __call__(self, scope: Literal[In.VIEW], **kwargs: Any) -> GitView[Self]: ...

# Other "overloads", if any. (In.CONTEXT?)

def __call__(self, scope: In = In.NEXT_CALL, **kwargs: Any) -> Union[Self, GitView[Self]]:
    if scope is In.NEXT_CALL:
        self._git_options = self.transform_kwargs(split_single_char_options=True, **kwargs)
        return self
    if scope is In.VIEW:
        return GitView(self, kwargs)
    # Code for any others.

To clarify, this is mostly just an example of the kind of thing I mean, but also to show one idea for avoiding introducing new public methods of Git. I do not, at least at this point, mean to advocate for this or any other particular design. (I think there are various other design considerations not touched on here.)

That's amazing, and looks much better than what I had in mind, mainly out of ignorance :)!
Maybe that could indeed be a way forward.