Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upFIx issue #464 corrupt log file #469
Conversation
|
Do you think it's possible for you to write a test which goes red if the change is not there ? The current test-suite should be a good starting-point for a fixture-based test. |
|
Please don't wait for me to write the test. I have a lot of work on my plate at moment. How I detected the bug was:
|
|
I'm not sure how to write the tests you want. Can you point to docs on what is required? |
| else: | ||
| creationflags = None | ||
|
|
||
| p = Popen(['ps', '--ppid', str(pid)], stdout=PIPE, creationflags) |
Byron
Jul 30, 2016
Member
I am afraid that using creationflags as positional argument doesn't actually end up in the correct spot. Also I thought that positional arguments after keyword args are not allowed.
I am afraid that using creationflags as positional argument doesn't actually end up in the correct spot. Also I thought that positional arguments after keyword args are not allowed.
Byron
Jul 30, 2016
Member
This concern is reflected in actual breakage as well: https://travis-ci.org/gitpython-developers/GitPython/jobs/148304657#L318
This concern is reflected in actual breakage as well: https://travis-ci.org/gitpython-developers/GitPython/jobs/148304657#L318
| @@ -609,6 +609,12 @@ def execute(self, command, | |||
| # end handle | |||
|
|
|||
| try: | |||
| if sys.platform == 'win32': | |||
| CREATE_NO_WINDOW = 0x08000000 | |||
Byron
Jul 30, 2016
Member
CREATE_NO_WINDOW is used twice, is there a reason it is not a constant on module or class level ?
CREATE_NO_WINDOW is used twice, is there a reason it is not a constant on module or class level ?
barry-scott
Aug 1, 2016
Author
Contributor
I intended to provide a obvious patch. You usually change my process control patches so I left that for you to decide on where you wished to define CREATE_NO_WINDOW.
I intended to provide a obvious patch. You usually change my process control patches so I left that for you to decide on where you wished to define CREATE_NO_WINDOW.
| @@ -609,6 +609,12 @@ def execute(self, command, | |||
| # end handle | |||
|
|
|||
| try: | |||
| if sys.platform == 'win32': | |||
Byron
Jul 30, 2016
Member
Generally for these kinds of assignment, I believe it's easier to read by starting out like this:
creationflags = None
if sys.platform == 'win32':
creationflags = CREATE_NO_WINDOW
Generally for these kinds of assignment, I believe it's easier to read by starting out like this:
creationflags = None
if sys.platform == 'win32':
creationflags = CREATE_NO_WINDOW
|
Now this PR contains two distinct and independent changes. The most recent one is hard to test, and I'd be fine without one. |
|
I fixed the keyword parameter passing. |
|
I have added a basic test by changing one of the existing test_repo.py tests. You can see the test fail by undoing the first line fix and running the test. |
Byron
merged commit d8ef023
into
gitpython-developers:master
|
@barry-scott Thank you, looks very good now ! I am particularly excited about the test, and hope in future you can give test-first development a shot as well. |
|
To get the test as one patch and the the test + fix as a second you will have to fix the dependency of the tests on master. Use of master means that you do not have a reproducible test. Its inputs depend on what the user did to create there master. I suspect that if you had to create the test data that you use master for you might have found this bugs ages ago. |
Must only append the first line of the commit to the log
Otherwise the log file is not parsable by GitPython.