Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork937
Ask git where its daemon is and use that#1697
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This changes the test helpers on Windows to use "git --exec-path"(with whatever "git" GitPython is using) to find the directory thatcontains "git-daemon.exe", instead of finding it in a PATH search.
@@ -182,7 +182,7 @@ def git_daemon_launched(base_path, ip, port): | |||
# and then CANNOT DIE! | |||
# So, invoke it as a single command. | |||
daemon_cmd = [ | |||
"git-daemon", | |||
osp.join(Git()._call_process("--exec-path"), "git-daemon"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm uneasy about having the test suite call the nonpublic_call_process
method to do this. I want it to use the samegit
as GitPython uses, including the effect of theGIT_PYTHON_GIT_EXECUTABLE
(as well as any future nuances that might ever arise) automatically, whichGit().execute(["git", "--exec-path"])
would not do. If the command weregit exec-path
orgit something --exec-path
, then I thinkGit().exec_path()
orGit.something(exec_path=True)
, respectively, could be used. But for agit
command that has no subcommand and just passes an option, I don't know of a way to use GitPython's public interface to run it. It may be that I'm just missing something obvious here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
That's definitely a shortcoming in theGit
class' API, it does always assume a sub-command. This also makes it impossible to set configuration overrides, for instance, so finding a solution for this will have immediate benefits, and it would definitely be welcome.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think the main hard part of adding such functionality is figuring out a way to do it that wouldn't be a breaking change. Most ordinary public-style method names could clash with someone's customgit
commands (such as scripts named likegit-*
), which GitPython is generally able to run and thus would begin to break after such a change. The most intuitive names for this, likeinvoke
, would be especially likely to clash (I'm sure some people have agit-invoke
script).
This also makes it impossible to set configuration overrides
Is this because only one-c
can be passed by calling theGit
instance withc=...
, or for some other reason (or am I misunderstanding what you mean)? To use an example inspired bycheck-version.sh
, and withg
as agit.cmd.Git
instance, I can cause-c versionsort.suffix=-pre
to be passed, and in the correct position, with:
g(c="versionsort.suffix=-pre").tag(sort="-v:refname")
That runsgit -c versionsort.suffix=-pre tag --sort=-v:refname
as desired, with-c versionsort.suffix=-pre
before the subcommand name and--sort=-v:refname
following it.
However, I can't pass more than one-c
that way, because a single call can't pass the same keyword argument multiple times, and the preceding arguments are discarded with multiple calls, i.e., these do the same thing:
g(c="versionsort.suffix=-pre")(c="versionsort.suffix=-RC").tag(sort="-v:refname")
g(c="versionsort.suffix=-RC").tag(sort="-v:refname")
But I'm not sure this is the problem you're thinking of, because a solution for passing-c
arguments and their operands, or for passing arbitrary argumentsbefore a subcommand, would not necessarily facilitate runninggit
without a subcommand. Nor would a solution for runninggit
without a subcommand necessarily allow a subcommand to be added in a user-friendly way supporting the keyword argument syntax for specifying the subcommand's own flags.
finding a solution for this will have immediate benefits
Can people just use_call_process
?
For having GitPython rungit
with arbitrarily specified arguments, the nonpublic_call_process
method does that. Does its behavior differ from the desired behavior for doing so?
If not, then that method could be made public simply by documenting it as public, which would avoid breaking any customgit
commands, because(a) it wouldn't change the actual behavior of GitPython at all, and(b) GitPython already doesn't support customgit
commands that start with_
, andgit
itself doesn't support custom commands that start with-
(since an attempt to invoke such a command would pass one or more options instead).
An example of where an attribute with a leading_
that is made public by documenting it as public, for the same reasons as we might want to do so here--that any other name might clash--is how types constructed with thecollections.namedtuple
factory have public_make
,_asdict
,_replace
,_fields
, and_field_defaults
attributes. (In contrast, although the_thread
module is public, this is not really an example of this, because it is not named that way for a similar reason.)
On the other hand, there may be some reasons not to make_call_process
public by declaring it so. The interface forcollections.namedtuple
is simpler than forgit.cmd.Git
, and also more widely known about because it is part of the standard library, so deviations from common naming conventions may be more discoverable. Also, intuitively, even if_call_process
were public, its name suggests that its use from outside GitPython's own code would be rarer thanexecute
. But using aGit
object to run a non-git
command should be rare, so if_call_process
is public then it should be usedmore often thanexecute
.
Making a "submethod" to rungit
with literal arguments
One possibility, again whereg
is aGit
instance, could be to allowg.execute.git(*args)
, accepting zero or more separate positional arguments in place of*args
that GitPython would immediately rungit
with. I find this intuitive, and it could be achieved by making theexecute
method a custom descriptor that works like a bound method, except that it also causesg.execute.git
to resolve tog._call_process
, andGit.execute.git
to resolve toGit._call_process
(so it also works explicitly passg
to the unbound form, as methods are expected to support).
But the problem with this is that it is not obvious whether the "submethod" ought to continue being usable when a class that derives fromGit
overridesexecute
. Secondarily, I think having overrides turn into descriptors that also support.git
would be complicated, and might go against assumptions people make about he effect of writing a subclass.
To be clear, the problem is not that overridingexecute
affects thebehavior. That is already the case with_call_process
and everything that uses it, and is probably the main reason for a subclass ofGit
to overrideexecute
. Rather, the question is whetherMyGit().execute.git(*args)
andMyGit().execute.git(my_g, *args)
should work and, if so, whether the complexity to make it work is justified.
Other ways, which also don't seem ideal
Other possibilities include:
- Naming the method a single underscore:
g._(*args)
. This seems unintuitive. - Versioning the interface, so something has to be passed when a
Git
object is constructed to enable new methods. - Keeping the
Git
class the same but providing a derived class ofGit
that includes new methods. - Using a top-level function that receives the
Git
object as its first argument. - Using a top-level function that does not use the
Git
object. - Picking some name peopleprobably are not using as a custom
git
command (but the more reliably they are not, the less intuitive the command is, probably). - Not adding a feature for this, but adding a convenient way to get the
git
command (relative or absolute path) that_call_process
passes toexecute
, and noting how to useexecute
with it inexecute
's docstring, elsewhere in the documentation, or both.
A hack that shouldn't be used
By the way, it turns out there actuallyis a way I could have used the "public" interface to achieve the effect ofg._call_process("--exec-path")
. Becausegit
accepts a--
after this option with no change in behavior, we can fool GitPython into thinking--
is the subcommand. Where againg
is aGit
instance:
>>>getattr(g(exec_path=True),"--")()'C:/Users/ek/scoop/apps/git/2.42.0.2/mingw64/libexec/git-core'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
However, I can't pass more than one
-c
that way, because a single call can't pass the same keyword argument multiple times, and the preceding arguments are discarded with multiple calls
That's interesting! I wasn't even aware this works. Also I don't know how this interacts with the maybe more typical usage ofrepo.git.subcommand()
, or is something likerepo.git(c="foo=bar").subcommand()
possible?
Regarding the multi-issue, maybe it already works like this?
g(c=["versionsort.suffix=-pre","other=baz"]).tag(sort="-v:refname")
To be clear, the problem is not that overriding
execute
affects thebehavior. That is already the case with_call_process
and everything that uses it, and is probably the main reason for a subclass ofGit
to overrideexecute
. Rather, the question is whetherMyGit().execute.git(*args)
andMyGit().execute.git(my_g, *args)
should work and, if so, whether the complexity to make it work is justified.
Do you think it's common to subclassGit
? I'd argue that this was never intended and I'd rather forbid it than think about it. And if it can't be prohibited officially, maybe it's possible to document it as "unsupported" which allows subclasses to break if they happen. Of course, that itself would be a breaking change, but I wonder anyone would notice.
Also, apologies in advance if what I say here doesn't make much sense or seems to ignore something you already mentioned - I am quite ignorant as to howGit
(the class) is truly working and I really don't know what's best.
But simply making_call_process
public officially seemed like the easiest while safe-enough way to go to me.
PS:>>> getattr(g(exec_path=True), "--")()
is wonderfully creative :D.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
That's interesting! I wasn't even aware this works. Also I don't know how this interacts with the maybe more typical usage of
repo.git.subcommand()
, or is something likerepo.git(c="foo=bar").subcommand()
possible?g(c=["versionsort.suffix=-pre","other=baz"]).tag(sort="-v:refname")
That does work! This treated bothpre
andRC1
as lower versions than their corresponding stable versions:
g(c=["versionsort.suffix=-pre","versionsort.suffix=-RC"]).tag(sort="-v:refname")
It does also work withrepo.git(...).subcommand(...)
:
>>>importlogging>>>logging.basicConfig(level=logging.INFO)>>>importgit>>>git.Git.GIT_PYTHON_TRACE=True>>>r=git.Repo("../Flood")>>>r.git(c=["versionsort.suffix=-pre","versionsort.suffix=-RC"]).tag(sort="-v:refname")INFO:git.cmd:git-cversionsort.suffix=-pre-cversionsort.suffix=-RCtag--sort=-v:refname'unletterspaced\nline-height\nletterspaced\nalpha-6\nalpha-5\nalpha-4\nalpha-3\nalpha-2\nalpha-1\nalpha-0'
(The repo I tested on doesn't have tags whose order is affected by versionsort, but the debugging output shows that both-c ...
are passed and in the correct positions.)
Do you think it's common to subclass
Git
?
I'm not sure, but it may be possible to effectively search at least code on GitHub and elsewhere where rich code searching is implemented to figure it out. For a lot of stuff using popular projects like GitPython, I find such searching hard, because one gets lots of code in forks and vendored copies of the project. But if GitPython is never itself inheriting fromGit
or testing for that, it may be reasonably easy to find that sort of thing.
If I figure anything out about that, I'll let you know. I would intuitively expect to be able to inherit from it.
But simply making
_call_process
public officially seemed like the easiest while safe-enough way to go to me.
Yes. If that's acceptable, then I think it should be strongly considered before doing anything more complicated that also expands the GitPython public interface. A further argument for preferring this to other approaches is that it is already referenced in some public methods' docstrings.
PS:
>>> getattr(g(exec_path=True), "--")()
is wonderfully creative :D.
:) I guess there's an interesting question about whether the--
"attribute" ofGit
instances should be considered public on the grounds that its name does not start with an underscore. 😺
Actually, I had meant to be joking, just then, but it checks out:
ek@Kip:~$catx.pyglobals()["--"]="Hello, world!"ek@Kip:~$python3.11-c'from x import *; print(globals()["--"])'Hello,world!
ek@Kip:~$caty.pyglobals()["_--"]="Hello, world!"ek@Kip:~$python3.11-c'from y import *; print(globals()["_--"])'Traceback (mostrecentcalllast):File"<string>",line1,in<module>KeyError:'_--'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If I figure anything out about that, I'll let you know. I would intuitively expect to be able to inherit from it.
I am definitely happy to make this a non-feature, or at least document that subclass behaviour might change unannounced.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks so much for your help! Despite a small improvement, it's much appreciated and I see how it will help to get the tests on windows to work.
Sincegitoxide
also has tests that rely ongit daemon
to run which work on Windows, I'd believe that getting these to work should generally possible.
Regarding the implementation of Windows- and test-specific environment variables, I hope their implementation can be adjusted to be usable on Windows as well.
Thanks for everything.
@@ -182,7 +182,7 @@ def git_daemon_launched(base_path, ip, port): | |||
# and then CANNOT DIE! | |||
# So, invoke it as a single command. | |||
daemon_cmd = [ | |||
"git-daemon", | |||
osp.join(Git()._call_process("--exec-path"), "git-daemon"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
That's definitely a shortcoming in theGit
class' API, it does always assume a sub-command. This also makes it impossible to set configuration overrides, for instance, so finding a solution for this will have immediate benefits, and it would definitely be welcome.
EliahKagan commentedOct 9, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I've described this in#1698 and proposed a fix as part of#1700. (In the longer term, I hope these can be eliminated or at least moved,along with all associated test-specific logic, out of the |
[](https://renovatebot.com)This PR contains the following updates:| Package | Change | Age | Adoption | Passing | Confidence ||---|---|---|---|---|---|| [GitPython](https://togithub.com/gitpython-developers/GitPython) |`==3.1.37` -> `==3.1.40` |[](https://docs.renovatebot.com/merge-confidence/)|[](https://docs.renovatebot.com/merge-confidence/)|[](https://docs.renovatebot.com/merge-confidence/)|[](https://docs.renovatebot.com/merge-confidence/)|---### Release Notes<details><summary>gitpython-developers/GitPython (GitPython)</summary>###[`v3.1.40`](https://togithub.com/gitpython-developers/GitPython/compare/3.1.38...3.1.40)[CompareSource](https://togithub.com/gitpython-developers/GitPython/compare/3.1.38...3.1.40)###[`v3.1.38`](https://togithub.com/gitpython-developers/GitPython/releases/tag/3.1.38)[CompareSource](https://togithub.com/gitpython-developers/GitPython/compare/3.1.37...3.1.38)#### What's Changed- Add missing assert keywords by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1678](https://togithub.com/gitpython-developers/GitPython/pull/1678)- Make clear every test's status in every CI run by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1679](https://togithub.com/gitpython-developers/GitPython/pull/1679)- Fix new link to license in readme by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1680](https://togithub.com/gitpython-developers/GitPython/pull/1680)- Drop unneeded flake8 suppressions by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1681](https://togithub.com/gitpython-developers/GitPython/pull/1681)- Update instructions and test helpers for git-daemon by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1684](https://togithub.com/gitpython-developers/GitPython/pull/1684)- Fix Git.execute shell use and reporting bugs by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1687](https://togithub.com/gitpython-developers/GitPython/pull/1687)- No longer allow CI to select a prerelease for 3.12 by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1689](https://togithub.com/gitpython-developers/GitPython/pull/1689)- Clarify Git.execute and Popen arguments by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1688](https://togithub.com/gitpython-developers/GitPython/pull/1688)- Ask git where its daemon is and use that by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1697](https://togithub.com/gitpython-developers/GitPython/pull/1697)- Fix bugs affecting exception wrapping in rmtree callback by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1700](https://togithub.com/gitpython-developers/GitPython/pull/1700)- Fix dynamically-set **all** variable by[@​DeflateAwning](https://togithub.com/DeflateAwning) in[https://github.com/gitpython-developers/GitPython/pull/1659](https://togithub.com/gitpython-developers/GitPython/pull/1659)- Fix small[#​1662](https://togithub.com/gitpython-developers/GitPython/issues/1662)regression due to[#​1659](https://togithub.com/gitpython-developers/GitPython/issues/1659)by [@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1701](https://togithub.com/gitpython-developers/GitPython/pull/1701)- Drop obsolete info on yanking from security policy by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1703](https://togithub.com/gitpython-developers/GitPython/pull/1703)- Have Dependabot offer submodule updates by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1702](https://togithub.com/gitpython-developers/GitPython/pull/1702)- Bump git/ext/gitdb from `49c3178` to `8ec2390` by[@​dependabot](https://togithub.com/dependabot) in[https://github.com/gitpython-developers/GitPython/pull/1704](https://togithub.com/gitpython-developers/GitPython/pull/1704)- Bump git/ext/gitdb from `8ec2390` to `6a22706` by[@​dependabot](https://togithub.com/dependabot) in[https://github.com/gitpython-developers/GitPython/pull/1705](https://togithub.com/gitpython-developers/GitPython/pull/1705)- Update readme for milestone-less releasing by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1707](https://togithub.com/gitpython-developers/GitPython/pull/1707)- Run Cygwin CI workflow commands in login shells by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1709](https://togithub.com/gitpython-developers/GitPython/pull/1709)#### New Contributors- [@​DeflateAwning](https://togithub.com/DeflateAwning) made theirfirst contribution in[https://github.com/gitpython-developers/GitPython/pull/1659](https://togithub.com/gitpython-developers/GitPython/pull/1659)**Full Changelog**:gitpython-developers/GitPython@3.1.37...3.1.38</details>---### Configuration📅 **Schedule**: Branch creation - At any time (no schedule defined),Automerge - At any time (no schedule defined).🚦 **Automerge**: Enabled.♻ **Rebasing**: Whenever PR becomes conflicted, or you tick therebase/retry checkbox.🔕 **Ignore**: Close this PR and you won't be reminded about this updateagain.---- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, checkthis box---This PR has been generated by [MendRenovate](https://www.mend.io/free-developer-tools/renovate/). Viewrepository job log[here](https://developer.mend.io/github/allenporter/flux-local).<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4xOS4yIiwidXBkYXRlZEluVmVyIjoiMzcuMTkuMiIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==-->Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
This removes the Windows-specific information in the warningmessage in git_daemon_launched.After the associated functionality was updated ingitpython-developers#1684 and thewarning message was abridged accordingly, the functionality wasupdated again ingitpython-developers#1697, causing the message to be outdated and nolonger helpeful (since having git-daemon.exe in a PATH directory isno longer necessary or useful), without any corresponding change tothe message.
This removes the Windows-specific information in the warningmessage in git_daemon_launched.After the associated functionality was updated ingitpython-developers#1684 and thewarning message was abridged accordingly, the functionality wasupdated again ingitpython-developers#1697, causing the message to be outdated and nolonger helpeful (since having git-daemon.exe in a PATH directory isno longer necessary or useful), without any corresponding change tothe message.
This removes the Windows-specific information in the warningmessage in git_daemon_launched.After the associated functionality was updated ingitpython-developers#1684 and thewarning message was abridged accordingly, the functionality wasupdated again ingitpython-developers#1697, causing the message to be outdated and nolonger helpeful (since having git-daemon.exe in a PATH directory isno longer necessary or useful), without any corresponding change tothe message.
Uh oh!
There was an error while loading.Please reload this page.
This changes the test helpers on Windows to use
git --exec-path
(with whatevergit
GitPython is using) to find the directory that containsgit-daemon.exe
, instead of findinggit-daemon.exe
in a PATH search.Because it is only on Windows that the tests run
git-daemon
directly rather than usinggit daemon
, this only affects Windows. Note that this doesnot affect Cygwin, which like other Unix-like systems usesgit daemon
for the tests.This change has three benefits:
git-daemon
working on Windows is removed.libexec\git-core
directory in theirPATH
. This directory contains a number of.dll
files, which are there for the executables in the directory that use them. But Windows includes allPATH
directories when searching forlibraries as well as programs, which can create very strange and confusing problems where completely unrelated programs end up using them instead of the versions of the libraries they should use.Note, however, what it doesnot cover:
The tests that use
git-daemon
are still skipped by default on Windows. These are exactly the tests that are skipped whenHIDE_WINDOWS_FREEZE_ERRORS
is set toTrue
, which is the case by default on Windows. I temporarily changed its default value toFalse
to test this PR. That change is deliberately not included in this PR and probably should not be made without further accompanying changes. (Seethis comment, but also the information below about how the tests usually fail, which is not by freezing.)The mechanism to skip them remains to be fixed. The
HIDE_WINDOWS_KNOWN_ERRORS
andHIDE_WINDOWS_FREEZE_ERRORS
variables are intended to be affected by the same-named environment variables, but this does not work properly. They areTrue
by default, but the only way to use environment variables to set them to a false value isto define the environment variable with an empty value (since everything else, as a string, is truthy). In addition to being very unlikely to have been the intended behavior, this is also hard to do on Windows when using typical shells, because in bothcmd.exe
and PowerShell setting an environment variable as empty actually removes the variable altogether. Rather than attempting to fix that here, I tested by temporarily changing the default value instead of using an environment variable.The tests that use
git-daemon
still usually fail on Windows. The failure mode is the same as when they are run withgit-daemon.exe
in a directory inPATH
, however. That is,whengit-daemon
is not found it looks like this, whilewhen it is found by aPATH
search it looks like this, andthe new way, finding it viagit --exec-path
, looks similar. Note how all errors of this form occur in the logwith-no-git-daemon.txt
:The files can be compared with
diff
or withthis convenient diff tool. I would also be happy to produce diffs using a particular technique, on request. Those diffs were also produced before rebasing this to include the changes from#1693, since I had neglected by merge that on my Windows system before working on this, but I believe none of the changes there are able to affect this.That is to say that this PR is very narrow in its scope of effects--it automates, and at least as reliably achieves, the desired effect of a setup step that had to be done manually before, but it doesn't address any of the other problems related to it.
One aspect of the way I did this change deserves special scrutiny; I've madea review comment about it.