FIx issue #464 corrupt log file by barry-scott · Pull Request #469 · gitpython-developers/GitPython (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Must only append the first line of the commit to the log
Otherwise the log file is not parsable by GitPython.
It must only have the first line of the commit 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 ? 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.
Maybe merge this and leave the bug open awiting a test case?
How I detected the bug was:
- Use GitPython to commit to a repo - causing the fault
- Use GitPython to scan the log ref file - detecting the fault
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 create a 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) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now this PR contains two distinct and independent changes. The most recent one is hard to test, and I'd be fine without one.
As for the first set of changes, I believe I already referred to the respective test-suite. Generally, you want to write a test that fails until your changes are applied. This is the whole trick. The suite in question should already contain everything you need to setup your own.
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 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.