Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork939
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
On systems without any environment variables and no pwd module, gitpython crashes as it tries to read the environment variable before looking at its config.
It looks like a maintainer might have to approve the workflow runs each time I try to fix the mypy troubles... |
Yobmod commentedAug 3, 2021 • 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 added an overload to config.py get_value() in main that fixes the/a mypy issue, if you want to copy that accross edit: |
This won't try and do something silly like convert `username=1` to a number.
No you didn't, you closed it! |
| 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 |
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 permissionscan 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!
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.
Thank a lot, this looks good to me.
It seems the difficulty stems more from dealing with tests and CI than the actual code change, and I don't fully follow. Hence I leave it to@Yobmod to merge in case there is nothing I missed.
Uh oh!
There was an error while loading.Please reload this page.
On systems without any environment variables (such as sandboxed CI runs) and no
pwdmodule (such as windows), GitPython crashes.This is just a logic error - there is no reason to fallback to
pwduntil the config lookup has actually failed. The implementation ofConfigWriter.get_valueis to catchException, so this patch doesn't attempt to work out a more specific exception than what was used before.Thisfixes#356