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

FIx issue #464 corrupt log file #469

Merged
merged 9 commits into from
Aug 2, 2016
Merged

FIx issue #464 corrupt log file #469

merged 9 commits into from
Aug 2, 2016

Conversation

barry-scott
Copy link
Contributor

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.
@Byron Byron added this to the v2.0.6 - Bugfixes milestone Jun 14, 2016
@Byron
Copy link
Member

Byron commented Jun 14, 2016

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.

@barry-scott
Copy link
Contributor Author

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:

  1. Use GitPython to commit to a repo - causing the fault
  2. 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.
@barry-scott
Copy link
Contributor Author

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)
Copy link
Member

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.

Copy link
Member

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

@Byron
Copy link
Member

Byron commented Jul 30, 2016

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.

@barry-scott
Copy link
Contributor Author

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
@barry-scott
Copy link
Contributor Author

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 Byron merged commit d8ef023 into gitpython-developers:master Aug 2, 2016
@Byron
Copy link
Member

Byron commented Aug 2, 2016

@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.

@barry-scott
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants