Add check for incorrect rebase by checking for duplicate commits and …

[xjusko]

…skip PR rules if any appear.
resolves #207

[mskacelik]

Is it necessary to have the hasDuplicateCommitInBase defined in the GithubProcessor, rather than having a private method in PullRequestRuleProcessor?

Afaik, the method does not use any of the class's states except for LOG.

[xjusko]

tbh, I placed it there, since the PullRequestRuleProcessor already uses method which are specific for this class and are in the GithubProcessor.
However, I can change it if you think it is better. I have no real preference, since I can see that there are some processors that also have helper methods inside its own class, and other which have those helper methods in GithubProcessor

[mskacelik]

I am thinking of three possible solutions:

1) Remove the method and use private methods for each processor.

Cons:

• It would be impossible to test, especially if some private methods perform more complex tasks.
• It would also bloat the individual Processor classes.

2) Make it a static method.

Pros:

• Since it's stateless, it would make more sense to have it as a static method.
• Decomposition.

Cons:

• The logger field needs to be static, which, in my opinion, should be the case anyway. See: https://google.github.io/styleguide/javaguide.html#s5.2.4-constant-names and https://stackoverflow.com/questions/13871337/differences-between-log-and-logger.
• If the GitHubProcessor bean is mocked via InjectMock, the static methods must also be mocked. Their implementation of static methods is unfortunate, even when setting the methods to their default behavior.

3.) Leave it as it is.

• ...

[xjusko]

I would leave it as it is. Or at least change it in another PR if it is needed.

[mskacelik]

Ok

[mskacelik]

Suggested change

	Set<GHCommit> fakeRepoCommitSet = new HashSet<>();
	fakeRepoCommitSet.add(commit1);
	fakeRepoCommitSet.add(commit2);
	Set<GHCommit> fakeRepoCommitSet = Set.of(commit1, commit2);

[mskacelik]

Hm, what about some checks that it did not do any rule hitting (aka the method ended before the rule hitting and other stuff...). Is it possible to check, for example, if there is no "/cc" comment (or reviewers), while the config file (with rules) is present?

Because I don't see a check that checks the "skip" part in this code, if that makes sense.

[mskacelik]

Suggested change

	verify(mocks.pullRequest(pullRequestJson.id()), never()).addLabels(any(String[].class));
	verify(mocks.pullRequest(pullRequestJson.id()), never()).requestReviewers(anyList());
	verify(mocks.pullRequest(pullRequestJson.id()), never()).addLabels(any());
	verify(mocks.pullRequest(pullRequestJson.id()), never()).requestReviewers(any());

any() also includes null, so I think it's better to have it for all cases when using never()* since it widens the set of rejected conditions.

*there are exceptions, for example:

verify(code()).foo("expected-value"); // one expected value
verify(code(),never()).foo(any());  // all the other values are not allowed – this does not work because any() also accepts "expected-value"

[xjusko]

@mskacelik, thanks for catching this
I had to change a bit differently, since addLabels is an overloaded method, I had to use nullable(String[].class}