RFR: 2043: Skara mistakenly tagged the wrong commit when processing a /tag command

[Erik Joelsson]

On Thu, 28 Sep 2023 21:19:50 GMT, Zhao Song <zsong at openjdk.org> wrote:

>> The method `GitLabRepository::recentCommitComments` may sometimes return a comment associated with the wrong commit. We have seen it happen, which caused a tag to be set on the wrong commit. My theory is that this was caused by a race, which was probably made more potent due to GitLab sometimes being slow in returning up to date data. See bug for more details.
>> 
>> The main fix is to always verify that an event note about a recent commit comment actually matches a comment found on the commit in question. The current implementation has a quick return when only one potential commit is found.
>> 
>> In addition to that, I'm also reworking the implementation to be a bit more robust. @zhaosongzs found that there is an alternate API for retrieving commit comments called "discussions" and that gives us more data, notably the same ID field as an event has. This makes it possible to match an event note with a commit comment note by ID instead of by the combination of author and created_at. From this further simplifications naturally fell out.
>> 
>> A CommitComment returned from `GitLabRepository::addCommitComment` no longer contains a value for ID. I think this is more correct than making up a value. I verified that no code seems to be using the returned object currently, so this shouldn't impact anything.
>> 
>> The two new tests were run and inspected manually.
>
> forge/src/main/java/org/openjdk/skara/forge/gitlab/GitLabRepository.java line 557:
> 
>> 555:             log.warning("Did not find commit with title " + commitTitle + " for repository " + projectName);
>> 556:         }
>> 557:         return found;
> 
> Just want to confirm that if we stop throwing exceptions here, the commitComment will be handled again in the next CommitCommentsWorkItem.

The way it's currently implemented, yes it will. The drawback of throwing an exception is that it's treated as an error and we get notified about it. But since this is something that can happen, and if it happens it's likely benign, I would like to avoid that.

The `CommitCommentsWorkItem` always queries for recent comments from the last 4 days. All processed comments are stored in a map to avoid processing again. If we never return a comment, it will be retried for 4 days, regardless of if we throw the exception or not.

-------------

PR Review Comment: https://git.openjdk.org/skara/pull/1564#discussion_r1341326483