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

Multiple improvements #12

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

mjdetullio
Copy link
Contributor

I found that "follow all integrations" gives far more accurate results than "follow branches".

There were cases I've run into with CRLF and mixed line endings that do not report blame correctly. I've added an attempt to account for CRLFs in cases where the P4Java API returns double the annotations than the file has lines.

Added support for multi-threaded blame, which makes things blazingly fast compared to blaming one file at a time. Code highly resembles the Git blame connector.

I also ran into some cases where connection would fail randomly during blame. This made it impossible to blame very large projects when running a first-time analysis. I added a basic retry.

I've been using these changes in production for a month now. Take it or leave it.

According to the documentation, P4Java is "basically thread-safe" and
that "you can have multiple threads working against a single IServer
object simultaneously".
@@ -101,7 +101,8 @@ void blame(InputFile inputFile, IOptionsServer server, BlameOutput output) throw
}
}

List<BlameLine> lines = computeBlame(inputFile, server, fileAnnotations);
boolean handleCrlf = fileAnnotations.size() >= (inputFile.lines() - 1) * 2;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this use case could have a dedicated unit test

@henryju
Copy link
Contributor

henryju commented Apr 15, 2016

Hi @mjdetullio and thanks again for your contribution on this plugin. All changes look good but I was just looking for a few more tests if possible.

@henryju henryju self-assigned this Apr 15, 2016
@henryju henryju removed their assignment Jun 14, 2016
@ganncamp
Copy link
Contributor

@mjdetullio we've moved this plugin to the community and no longer maintain it directly. Let us know if you'd like to take over maintenance of this. Then you could merge your own PR. :-)

@mjdetullio
Copy link
Contributor Author

I've since switched employers and no longer have access to a production Perforce system. It would be better for someone else to maintain. I may still get around to writing a couple tests for this though.

@ganncamp
Copy link
Contributor

Okay, thanks.

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.

3 participants