Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork938
FIx issue #464 corrupt log file#469
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
It must only have the first line of thecommit messages, not the while multiple line log.
Do you think it's possible for you to write a test which goes red if the change is not there ? Thecurrent 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:
|
This fixes a UI problem with using GitPython from a GUI python probgram.Each repo that is opened creates a git cat-file processs and that provess will createa console window with out this change.
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) |
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 am afraid that usingcreationflags as positional argument doesn't actually end up in the correct spot. Also I thought that positional arguments after keyword args are not allowed.
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.
This concern is reflected in actual breakage as well:https://travis-ci.org/gitpython-developers/GitPython/jobs/148304657#L318
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. |
- add a second line to commit messages with the "BAD MESSAGE" text- read in the log and confirm that the seond line is not in the log file
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. |
@barry-scott Thank you, looks very good now ! I am particularly excited about the test, and hope in future you can givetest-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.