Skip to content

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 9 commits into from
Aug 6, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion git/util.py
Original file line number Diff line number Diff line change
Expand Up @@ -705,7 +705,11 @@ def default_name() -> str:
setattr(actor, attr, val)
except KeyError:
if config_reader is not None:
setattr(actor, attr, config_reader.get_value('user', cvar, default()))
try:
val = config_reader.get('user', cvar)
except Exception:
val = default()
setattr(actor, attr, val)
# END config-reader handling
if not getattr(actor, attr):
setattr(actor, attr, default())
Expand Down
42 changes: 30 additions & 12 deletions test/test_util.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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')
Expand All @@ -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
Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which test fails?

Copy link
Contributor

@Yobmod Yobmod Aug 4, 2021

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!

Copy link
Contributor Author

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!

# 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))
Expand Down