git support

[amorphous1]

It would be nice if the plugin supported git besides svn.

(We are in the process of migrating from svn to git - the developer's working copies are still svn, but the Jenkins build already uses a read-only git mirror.)

[Grundlefleck]

This would be a fantastic addition. My only requirement is that the git library used be a pure Java version. That doesn't seem like too big a hurdle:

http://www.eclipse.org/jgit/

I understand you want to try implementing yourself. Please discuss here if you have any questions.

Many thanks!

[Grundlefleck]

Hey @amorphous1

I took a look, and found a couple of small issues. You can let me know if you are keen to work on fixing them, if you are too busy or not interested, no worries, I can do it.

First problem is a null pointer exception is thrown from GitCommittedCodeDetailsFetcher. I haven't tracked this down to the exact JGit call, but I'm pretty sure it comes when trying to get the commit details for an uncommitted line of code. We can assume that if a line of code is not committed yet, then it's not too old, and just carry on.

Second issue is down to my pickiness. I don't like the way the properties for choosing between svn and git is leaving the versionControlProjectRoot empty. How about the following:

We prefix the property names related to version control with the vcs being used. For SVN, change versionControlProjectRoot to svn.versionControlProjectRoot, which needs to be set as the same value as before. The former should be deprecated, with warnings printed to the console to redirect users to the new property. I don't know of anyone else who uses findbugs4junit besides us two, but in case we have other users, it would be nice not to break without warning.

The projectBaseDirName property should be split into svn.projectBaseDirName and git.projectBaseDirName.

It is a runtime to have a mix of vcs-prefixed properties being populated. So if you have svn.projectBaseDirName=... and git.projectBaseDirName, then properties are considered invalid.

Alternatively we could just have a new property, like versionControlSystemUsed={git|svn}. There would still be runtime errors for ambiguous/contradictory settings.

What do you think?

---

As I said, if you're happy with the contribution so far (and I am definitely happy with it) then feel free to let me know, and I'll fix these two issues.

Thanks!

[amorphous1]

I agree that the properties should be improved (just wanted to have a rather minimal changeset that works for my usecase and that you can look at). On first glance, I like the alternative approach with "versionControlSystemUsed" a bit more, because I think that it would be easier when you wanted to support even more VCS (in theory, mercurial should work very similar to git) - might be wrong though. Feel free to implement any way you seem fit, I do not intend to tackle that issue.

I could reproduce the NPE and pushed a quick fix to my fork (I had not tested this case because in CI you always have a clean working copy). There are probably more NPEs lurking...

(Sorry for answering late, I blame the weather in London...)

[Grundlefleck]

Cool, will pick up both of those points.

Many thanks!