Skip to content
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

Feature/gitrepository #52

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

marquiz
Copy link
Contributor

@marquiz marquiz commented Jan 23, 2018

No description provided.

@agx
Copy link
Owner

agx commented Jan 23, 2018

Hi,
thanks for splitting this out.
commit_all has an author_info so it should get a committer_info as well.

@marquiz
Copy link
Contributor Author

marquiz commented Jan 23, 2018

commit_all has an author_info so it should get a committer_info as well.

Done

extra_env = author_info.get_author_env() if author_info else None
def _commit(self, msg, args=[], author_info=None, committer_info=None):
extra_env = author_info.get_author_env() if author_info else {}
if committer_info:
Copy link
Owner

Choose a reason for hiding this comment

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

If we change from None to {} here we need to fixup _build_env to return None for extra_env being {} or None

@agx
Copy link
Owner

agx commented Jan 23, 2018

I spotted one other change that might introduce problems since __build_env would change return type and that gets passed to Popen and Popen's default type for "do nothing" is None not {} (see _execute_child in subprocess.py). I don't think that matter but it might be worth checking.

@marquiz
Copy link
Contributor Author

marquiz commented Jan 23, 2018

I spotted one other change that might introduce problems since __build_env would change return type and that gets passed to Popen and Popen's default type for "do nothing" is None not {} (see _execute_child in subprocess.py). I don't think that matter but it might be worth checking.

Well spotted :) It won't make any difference as the current env is copied in this case. An empty dict is already used in some places, e.g. add_files(). However, I now added one more commit so that extra_env={} will be handled as expected.

@marquiz marquiz force-pushed the feature/gitrepository branch 3 times, most recently from 437b9e7 to ea3952f Compare September 17, 2018 11:38
For setting the committer name/email/date - similarly to author_info.

Signed-off-by: Markus Lehtonen <[email protected]>
For setting the committer name/email/date - similarly to author_info.

Signed-off-by: Markus Lehtonen <[email protected]>
No need to build 'env' arg for popen when extra_env is empty. In some
methods an empty dict is used to indicate "no extra env". This commit
does not affect the output/outcome of GitRepository operations, but,
just makes the code a tiny bit sleeker.

Signed-off-by: Markus Lehtonen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants