Repository navigation
If diff.external is set, diffing via API fails silently as external tool isn't understood #1828
Description
Activity
- changed the title
[-]Using `create_patch=True` when comparison includes index or working tree always returns empty list[/-][+]Using `create_patch=True` when diff includes index or working tree always returns empty list[/+]on Feb 18, 2024 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=Truein 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 = difftI have deleted this from my
~/.gitconfigand 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-diffprocess from within GitPython.Reacted by Ege Can Taşlıçukur- changed the title
[-]Using `create_patch=True` when diff includes index or working tree always returns empty list[/-][+]If `diff.external` is set, diffing via API fails silently as external tool isn't understood[/+]on Feb 19, 2024 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:
- The local and worktree scopes are also being suppressed.
- No interaction with a local repository is possible (really, this is a special case of 1).
- It is being done only in a test suite.
- It is being done only in code meant only for use in test suites (as in
gix-testtools). - The user has explicitly configured or otherwise requested this (rare).
- Specific well-understood scenarios (rare).
Otherwise, I would be reluctant to default to suppressing the system and global scopes when invoking
gitcommands, because doing so can introduce errors or decrease safety.It can break any
gitoperation that (even if only in principle) reads configuration, because globalsafe.directoryallowlists will not be honored. In some subcommands, the resulting failure is non-obvious, because it does not report anything relatedsafe.directory, nor issue any "dubious ownership" message, instead behaving the same as if there were no repository.diffis 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 lineek@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.bareRepositorytoexplicitin 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.bareRepositorytoexplicitwhen using a version of Git that supportssafe.bareRepositorycan, when performing operations that are documented to use the installed Git, reasonably expect thatsafe.bareRepositorywill 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, thensafe.bareRepositorywould 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 byGIT_CONFIG_NOSYSTEM; they are not used from the local and worktree scopes.)safe.bareRepositoryis 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 diffcould, 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
-cshould 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).
Why do I advocate
-cover other techniques of setting command-scope variables?
(click to expand if interested)Portability
There are two other approaches, besides passing
-c <name>=<value>togitbefore any subcommands, that can be used to set configuration variables in the command scope. But they are not portable enough to use as the primary strategy, other than in the test suite:-
GIT_CONFIG_{COUNT,KEY,VALUE}is recommended for the purpose of setting command-scope Git configuration variables when it is inconvenient or infeasible to use-c. It would arguably be ideal.But it is not supported by all versions of
gitin practical use, because downstream distributions often package old versions ofgit, to which they backport security patches but not most new features.(Interaction with inherited
GIT_CONFIG_{COUNT,KEY,VALUE}configuration is not a problem, though. One incrementsGIT_CONFIG_COUNT, treating it as0if unset, and "pushes" new key-value pairs by placing them starting inGIT_CONFIG_KEY_kGIT_CONFIG_VALUE_kwherekis the old value ofGIT_CONFIG_COUNT. The algorithm can be seen in this test helper, though if used in production then it should be set in the subprocess environment only, to avoid race conditions.) -
GIT_CONFIG_PARAMETERSis the waygitpasses command-scope configuration variables originally set with-cto its subprocesses, as well as through non-Git subprocesses, such as if one runsgit -c name=value customandgitfinds agit-customcommand in aPATHsearch.But while this environment variable has been recognized by
gitfor a much longer time, it is considered to be an implementation detail. Its specific syntax is not documented. Its syntax has changed once so far, so we would probably have to support both, to accommodate downstream distributions as in (1). At least in principle, it could change again, in which case GitPython would automatically break.The syntax of
GIT_CONFIG_PARAMETERSis also more complicated and less intuitive than that ofGIT_CONFIG_{COUNT,KEY,VALUE}. If I recall correctly, this is one of the reasons theGIT_CONFIG_{COUNT,KEY,VALUE}syntax was added rather than documenting and promoting the use ofGIT_CONFIG_PARAMETERS.
Limitations
There is admittedly a disadvantage of
-c: most platforms impose a maximum length of an entire command line, i.e., of a command and all its arguments. Commands that are already long and complex might, in rare cases, be pushed over the limit by the inclusion of enough-cname=valueargument pairs.(There are other disadvantages in some situations. For example, on some operating systems, it may be easier for other processes, including those run as other users, to exfiltrate sensitive data passed in command-line arguments than passed in other ways, even environment variables. But for hard-coded site-nonspecific configuration options, they are not sensitive, so this does not apply.)
I do not think this justifies the direct use – that is, any use other than as an implicit effect of
-c– ofGIT_CONFIG_PARAMETERS, since it is considered an implementation detail. It might justify some interfaces offering an option to useGIT_CONFIG_{COUNT,KEY,VALUE}instead of using-c.It is tempting to say that this could automatically be done as a fallback, or even automatically preferred when the output of
git versionreveals thatgitfully supportsGIT_CONFIG_{COUNT,KEY,VALUE}. But I suspect we should avoid that, and either refrain from usingGIT_CONFIG_{COUNT,KEY,VALUE}in lieu of-c, or do in an opt-in manner, because…The command scope behaves like two scopes
gittreats the command scope as though it were two scopes: one for-c/GIT_CONFIG_PARAMETERS, and the other forGIT_CONFIG_{COUNT,KEY,VALUE}. When a variable of the same name is set in both ways:-c/GIT_CONFIG_PARAMETERSalways overridesGIT_CONFIG_{COUNT,KEY,VALUE}…- …even if non-
gitprocesses set-c/GIT_CONFIG_PARAMETERSsolely via-c(i.e., this is not specifically an effect of "unauthorized" or incompatible use ofGIT_CONFIG_PARAMETERS) - …even if the
-cvariable is inherited from a process higher in the tree.
For example:
ek@Kip:~$ git config get foo.bar ek@Kip:~[1]$ GIT_CONFIG_COUNT=1 GIT_CONFIG_KEY_0=foo.bar GIT_CONFIG_VALUE_0=inner git config get foo.bar inner ek@Kip:~$ cat ~/bin/git-run #!/bin/sh "$@" ek@Kip:~$ git -c foo.bar=outer run sh -c 'git -c foo.bar=inner config get foo.bar' inner ek@Kip:~$ git -c foo.bar=outer run sh -c 'GIT_CONFIG_COUNT=1 GIT_CONFIG_KEY_0=foo.bar GIT_CONFIG_VALUE_0=inner git config get foo.bar' outerThat effect – including, as shown above, where only
-cand notGIT_CONFIG_PARAMETERSis used explicitly – is a straighforward consequence of thegitbehavior of setting variables fromGIT_CONFIG_{COUNT,KEY,VALUE}before setting them fromGIT_CONFIG_PARAMETERS(the latter thereby taking precedence):ek@Kip:~$ git -c foo.bar=outer run sh -c 'GIT_CONFIG_COUNT=1 GIT_CONFIG_KEY_0=foo.bar GIT_CONFIG_VALUE_0=inner git config list --show-scope' | grep ^command command foo.bar=inner command foo.bar=outer ek@Kip:~$ git -c foo.bar=outer run sh -c 'GIT_CONFIG_COUNT=1 GIT_CONFIG_KEY_0=foo.bar GIT_CONFIG_VALUE_0=inner git run printenv' | grep ^GIT_ GIT_EXEC_PATH=/usr/lib/git-core GIT_CONFIG_COUNT=1 GIT_CONFIG_PARAMETERS='foo.bar'='outer' GIT_CONFIG_VALUE_0=inner GIT_CONFIG_KEY_0=foo.barAlthough the mechanism is straightforward, the effect is unintuitive because, in other cases, when
gitconfiguration variables of the same name are considered to be in the same scope, descendant processes override values set in their ancestors.More important than it being unintuitive is that it makes
-candGIT_CONFIG_{COUNT,KEY,VALUE}non-equivalent in general, even when using versions ofgitthat fully support the latter. Therefore, I think we should avoid substituting one approach for the other automatically.(Note that this issue of the command scope being treated as though it were two separate scopes is not itself a point in favor of any particular choice, only that we should not intermix the choices in ways that users and developers would not expect.)
So…
If we cannot use
-c, then of the two environment-variable-based alternatives,GIT_CONFIG_{COUNT,KEY,VALUE}is preferable because it is documented. But I think we can just use-c.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
Gitinstance directly: not by defaultI think we should not, by default, set extra configuration variables when a user runs a subcommand directly on a
Gitinstance – whether through an instance the caller manages, or an instance obtained as thegitattribute of aRepoobject.The current effect of invoking a dynamic method of a
Gitinstance, when the user has not done anything special to customize it, is to run agitcommand 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 theexecutemethod directly, the specific command run is not augmented with additional configuration options.I think users expect
Gitobjects 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 specificgitcommands 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-coptions. They can do this either persistently by callingset_persistent_git_optionswith keyword arguments representing the variables to be set, or only for the next call by calling theirGitinstance itself with such keyword arguments. (#2029 shall, among other changes, add more ways this happens, and even insert extra arguments in directexecuteinvocations, but still only by opting in.)When other GitPython facilities perform a specific function, even via
Git: very often yesIn addition to being only about preserving default behavior, the above also only applies to the behavior of the
Gitclass. If changing what options are specified from other code – such as code inRepo,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
Gitclass's dynamic methods orexecutemethod, which are far more general and thus almost surely have uses that would be broken.)As applied to
git diffI think the specific case of
git diffis consistent with this distinction.- Where
gis aGitinstance andris aRepoinstance, I would very much expectg.diff(…)orr.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 aDiffIndexlistingDiffobjects, 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 thatr.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, whichIndexFileinherits, carries several other such customizations, which happen only when thatdiffmethod is used (or if another caller applies similar customizations itself). For example,--abbrev=40and--full-indexare 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-coptions automatically per-command. Even knowing now that this is not part of what you meant, it's an interesting idea (albeit not something thatGitshould 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.gitdirectly.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
Gitinstance, or to allGitinstances?That is, would the context manager be:
-
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
gitcommands)? 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… -
Independent of any particular instance, so that any use of any number of
Gitinstances 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 usingcontextvars.)
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_optionsmethod were introduced, somewhat analogously tocustom_environment(though, echoing a concern above, this would break any existing usage of a customgit custom-git-optionscommand, 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 likewith 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
Gitinstance 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 theGitinstance that adds options.Currently, calling the
Gitinstance directly with keyword arguments sets options to be passed before the subcommand, which are used and cleared by_call_processnext time a dynamic method is called:Lines 1505 to 1518 in 2e10199
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
GitViewclass is omitted because it would require design decisions; docstrings, and theInclass inheritingenum.Enum, are omitted for brevity; but...is meant literally as@overloadis 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.
GitPython version: 3.1.42.
prints
R=Trueworkaround mentioned in #852 does not help either:This also happens when I try to diff tree against index or working tree
returns
It looks like using
create_patch=Truewhen comparison includes index or working tree always returns empty list. So right now only way to reliably usecreate_patch=Trueis to diff tree against tree.Originally posted by @can-taslicukur in #852 (comment)