Master

[rogovskiy]

A few stability fixes found in my environment - 1) Unsubmitted files used to cause NPE in sonarRunner 2) Unsubmitted files cause projects to not import because the change set date was undetermined

[henryju]

@rogovskiy For other SCMs we usually tend to try detecting unsubmitted files and not even try to report blame on them. Adding if (!null) in several places in the code is a bit hard to maintain in the long run. Maybe we could check the result from executor.getServer().getFileAnnotations() => I guess that if it returns an empty list we can simply stop further processing. Can you test this option?

[rogovskiy]

I don't think that method returned null nor empty list. executor.getServer().getRevisionHistory() method returned <null, null> map entry for one of the files I had. I am not sure why, guessing it was a new file.

Same for the date null check , I don't know what causes it, but something does. May be unsubmitted change sets don't have a date.

I am guessing the issues I fixed a not important for when you have a fresh workspace, otherwise perforce interaction may seem broken.

[henryju]

By the way it relates to https://jira.sonarsource.com/browse/SONARSCPER-4

[mjdetullio]

Pretty sure this is related to https://groups.google.com/d/topic/sonarqube/Z4D-n9eQ0Ts/discussion as well

[henryju]

Hi guys,

To be perfectly clear: SonarQube expect as many blame info as lines in the file. And for each blame info we need a date. There is no point trying to return partial information since it will fail either during analysis, or later it will produce unexpected behavior (coverage on new code not properly computed for example). I still don't understand how it is possible to have a changeset without a date, but if this is something that can't be fixed on perforce side I think the solution is to return nothing for such files and simply skip them with a clear warning in logs. I'll try to create a patch for that.

[mjdetullio]

Perhaps if anything unexpected happens in the PerforceBlameResult methods (processBlameLines, processRevisionHistory, createBlameLines) , those methods should throw an exception (add a new type, PerforceBlameException?) so it can be handled in PerforceBlameCommand. The error handling would then invoke continue inside the loop to skip the file.

Although using exceptions to control flow may not be ideal, there's also the option of changing the return types to boolean or null return (for createBlameLines) at the cost of some complexity.

Thoughts?

[henryju]

The point is to avoid a "catch all and ignore" approach to avoid publishing incomplete SCM data when possible. The usual example is if you provide a wrong credential we don't want to have a successful analysis without SCM data, because in this situation all issues will no be properly assigned. Here we prefer to fail the analysis and let the user fix the configuration issue. Give me some time to work on a proposal so that you can test and give me feedback.

[henryju]

Hi guys. I made an attempt to improve the code and make it more testable. Could you please test and report any issue (especially @rogovskiy with your null date). I tried to catch unsubmitted files as soon as possible. Thanks

[henryju]

Outdated by #7