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

Redo fossil segment to use RepoStats #286

Merged
merged 6 commits into from
Sep 29, 2017
Merged

Conversation

emansije
Copy link
Contributor

Includes a hack to make it work in older versions of fossil that throw an error with fossil changes --differ, like version 1.37 does.

This implementation does not work well in older versions of fossil, like 1.37,
apparently due to a bug in that version of fossil that throws an error with
`fossil changes --differ`.
For older versions of fossil that throw an error on `fossil changes --differ`,
like version 1.37.
@b-ryan
Copy link
Owner

b-ryan commented Sep 15, 2017

This looks great. I'm going to try to look in more detail & do some of my own testing today. I am a bit busy though so if I don't get back to you by Monday please feel free to remind me.

If the fossil repository had untracked files and no modifications in the
tracked files, the parsing would fail because the status list would begin with
an unexpected blank element.
@emansije
Copy link
Contributor Author

In the meanwhile I found a bug that manifested when the fossil repository had no changes in the tracked files but had untracked files. That situation would brake the parsing because it would introduce an unexpected blank element in the beginning of the status list.

@b-ryan
Copy link
Owner

b-ryan commented Sep 19, 2017

@emansije how would you feel about introducing tests for this? I think the most helpful thing you could do would be to test the parse_fossil_stats test - you could use the uptime_test file as an example of how this might look.

@emansije
Copy link
Contributor Author

Sure! I really should have implemented the tests from the start, it'd probably have saved me some time. I'll use your suggestions.

It implies going from a threaded segment to a basic one, otherwise it throws a
RuntimeError: threads can only be started once
@emansije
Copy link
Contributor Author

I can't get the test to work when the fossil segment is a threaded one, it keeps throwing this error:

RuntimeError: threads can only be started once

@b-ryan
Copy link
Owner

b-ryan commented Sep 26, 2017

I just played around a bit to figure out what was going on. I made some changes here: ebb7187

The issue was the use of self.segment.start() within a for loop in your test_all function. The segment is created in the setup function but then started multiple times within the loop. The easiest way I could figure to get around this was to just test the parsing function rather than testing the segment itself.

Let me know what you think / if you have other suggestions.

@emansije
Copy link
Contributor Author

Looks good to me! The tests and the segment work just fine and it's a cleaner solution, too.

@b-ryan b-ryan merged commit b6c7142 into b-ryan:master Sep 29, 2017
b-ryan pushed a commit that referenced this pull request Sep 29, 2017
@emansije emansije deleted the fossil-repostats branch October 1, 2017 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants