-
-
Notifications
You must be signed in to change notification settings - Fork 933
Do not call get_user_id if it is not needed #1314
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
Merged
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
39f12bd
Do not call get_user_id if it is not needed
eric-wieser ff0ecf7
Fix mypy
eric-wieser 335e59d
Update config.py
eric-wieser 05c77cf
Merge branch 'main' into patch-1
eric-wieser 994f387
Use get instead of get_value
eric-wieser a71b617
Merge branch 'gitpython-developers:main' into patch-1
eric-wieser 70b50e0
Fix test
eric-wieser d490d66
Try a better test
eric-wieser ec04ea0
Update test_util.py
eric-wieser File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,7 +22,10 @@ | |
parse_date, | ||
tzoffset, | ||
from_timestamp) | ||
from test.lib import TestBase | ||
from test.lib import ( | ||
TestBase, | ||
with_rw_repo, | ||
) | ||
from git.util import ( | ||
LockFile, | ||
BlockingLockFile, | ||
|
@@ -217,16 +220,31 @@ def test_actor(self): | |
self.assertIsInstance(Actor.author(cr), Actor) | ||
# END assure config reader is handled | ||
|
||
@with_rw_repo('HEAD') | ||
@mock.patch("getpass.getuser") | ||
def test_actor_get_uid_laziness_not_called(self, mock_get_uid): | ||
def test_actor_get_uid_laziness_not_called(self, rwrepo, mock_get_uid): | ||
with rwrepo.config_writer() as cw: | ||
cw.set_value("user", "name", "John Config Doe") | ||
cw.set_value("user", "email", "[email protected]") | ||
|
||
cr = rwrepo.config_reader() | ||
committer = Actor.committer(cr) | ||
author = Actor.author(cr) | ||
|
||
self.assertEqual(committer.name, 'John Config Doe') | ||
self.assertEqual(committer.email, '[email protected]') | ||
self.assertEqual(author.name, 'John Config Doe') | ||
self.assertEqual(author.email, '[email protected]') | ||
self.assertFalse(mock_get_uid.called) | ||
|
||
env = { | ||
"GIT_AUTHOR_NAME": "John Doe", | ||
"GIT_AUTHOR_EMAIL": "[email protected]", | ||
"GIT_COMMITTER_NAME": "Jane Doe", | ||
"GIT_COMMITTER_EMAIL": "[email protected]", | ||
} | ||
os.environ.update(env) | ||
for cr in (None, self.rorepo.config_reader()): | ||
for cr in (None, rwrepo.config_reader()): | ||
committer = Actor.committer(cr) | ||
author = Actor.author(cr) | ||
self.assertEqual(committer.name, 'Jane Doe') | ||
|
@@ -238,16 +256,16 @@ def test_actor_get_uid_laziness_not_called(self, mock_get_uid): | |
@mock.patch("getpass.getuser") | ||
def test_actor_get_uid_laziness_called(self, mock_get_uid): | ||
mock_get_uid.return_value = "user" | ||
for cr in (None, self.rorepo.config_reader()): | ||
committer = Actor.committer(cr) | ||
author = Actor.author(cr) | ||
if cr is None: # otherwise, use value from config_reader | ||
self.assertEqual(committer.name, 'user') | ||
self.assertTrue(committer.email.startswith('user@')) | ||
self.assertEqual(author.name, 'user') | ||
self.assertTrue(committer.email.startswith('user@')) | ||
committer = Actor.committer(None) | ||
author = Actor.author(None) | ||
# We can't test with `self.rorepo.config_reader()` here, as the uuid laziness | ||
# depends on whether the user running the test has their global user.name config set. | ||
self.assertEqual(committer.name, 'user') | ||
self.assertTrue(committer.email.startswith('user@')) | ||
self.assertEqual(author.name, 'user') | ||
self.assertTrue(committer.email.startswith('user@')) | ||
self.assertTrue(mock_get_uid.called) | ||
self.assertEqual(mock_get_uid.call_count, 4) | ||
self.assertEqual(mock_get_uid.call_count, 2) | ||
|
||
def test_actor_from_string(self): | ||
self.assertEqual(Actor._from_string("name"), Actor("name", None)) | ||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 assume there's some way to get a writeable repo that we can override the config in, but I'm not familiar enough with your fixtures to want to go to the effort of working that out.
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 a lot for helping with the test. Getting a repository with write permissions can be done like this.
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, I added another test. It turns out that this comment still applies, as the only reason now that the patched get_user would be called is if the global git config is missing.
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.
Just wondering, does it matter if Dev's have to have git.name --global set?
I just did a fresh Ubuntu install and one test failed until I set git.email anyway.
If that wasn't a fluke, can put in test instructions to set both.
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.
Which test fails?
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.
Test_remote.py, assertion at line 333 failed.
Also came with warnings about not being able to do something (create temporary file/folder?) at git://localhost:19418/daemon_repo-test_base-5tjbtmwj.
But fixed by setting git.email --global
Could have been an anomaly, too much work to reinstall and build python from source again to check!
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.
Oh, I thought you meant my new test failed!