RFR: 1452: Can never notify on first commit [v3]

[Erik Joelsson]

On Wed, 5 Oct 2022 18:02:05 GMT, Zhao Song <zsong at openjdk.org> wrote:

>> I changed the logic when there is no notify history.
>> 
>> Originally, on the first commit of a new repo, the bot would find no notify history, so it will make the first commit as an initial state and it will not make any notification.
>> 
>> Now, on the first commit of a new repo, the bot would find no notify history too, but it will make the initial git hash as the initial state of the repo, so it will treat the first commit as an 'update' operation and will make notifications.
>
> Zhao Song has updated the pull request incrementally with one additional commit since the last revision:
> 
>   set a threshold to determine whether should notify on first commit

Looks pretty good, comments mostly about cosmetics and details.

bots/notify/src/main/java/org/openjdk/skara/bots/notify/RepositoryWorkItem.java line 66:

> 64:     private final List<RepositoryListener> listeners;
> 65: 
> 66:     private static final String INITIAL_GIT_HASH = "4b825dc642cb6eb9a060e54bf8d69288fbee4904";

This hash is already defined as a constant in `GitRepository`. I think it's better to make that public and use it from there.

bots/notify/src/main/java/org/openjdk/skara/bots/notify/RepositoryWorkItem.java line 68:

> 66:     private static final String INITIAL_GIT_HASH = "4b825dc642cb6eb9a060e54bf8d69288fbee4904";
> 67: 
> 68:     private static final int THRESHOLD = 5;

This constant needs a better name. Something like `NEW_REPOSITORY_COMMIT_COUNT_THRESHOLD`

bots/notify/src/main/java/org/openjdk/skara/bots/notify/RepositoryWorkItem.java line 368:

> 366: 
> 367:             boolean hasBranchHistory = !history.isEmpty();
> 368:             int existingCommits = localRepo.getExistingCommits();

I think this should probably be inlined on line 372. Yes, that will make us call the method for each branch when there are branches without commit history, but that only happens the very first time this WorkItem runs on a new branch.

bots/notify/src/main/java/org/openjdk/skara/bots/notify/RepositoryWorkItem.java line 374:

> 372:                     if (existingCommits <= THRESHOLD) {
> 373:                         // In this case, the repo will be considered as a new repo, notify bot will notify start from the first commit
> 374:                         log.info("This is a new repo");

Suggestion:

                        log.info("This is a new repo, starting notifications from the very first commit");

This that log message, I think the comment above becomes redundant.

bots/notify/src/main/java/org/openjdk/skara/bots/notify/RepositoryWorkItem.java line 378:

> 376:                             log.info("Resetting mark for branch '" + ref.name() + "' for listener '" + listener.name() + "'");
> 377:                             // Initial the hash for the branches in the first commit, so that the branches will not be treated as 'new'
> 378:                             // and the first commit will be treated as 'update', so we will get notifications

Suggestion:

                            // Initialize the mark for the branch with the special Git empty tree
                            // hash to trigger notifications on all existing commits.

bots/notify/src/main/java/org/openjdk/skara/bots/notify/RepositoryWorkItem.java line 383:

> 381:                     } else {
> 382:                         // In this case, the repo will be considered as an existing repo with history, notify bot will only notify on new commits
> 383:                         log.info("This is an existing repo with history");

Suggestion:

                        log.info("This is an existing repo with history, starting notifications from commits after " + ref.hash());

This that log message, I think the comment above becomes redundant.

bots/notify/src/main/java/org/openjdk/skara/bots/notify/RepositoryWorkItem.java line 387:

> 385:                             log.info("Resetting mark for branch '" + ref.name() + "' for listener '" + listener.name() + "'");
> 386:                             // Initial the hash for the branches in the first commit, so that the branches will not be treated as 'new'
> 387:                             // and the first commit will be treated as 'update', so we will get notifications

Suggestion:

                            // Initialize the mark for the branch with the current HEAD hash. Notifications 
                            // will start on future commits.

bots/notify/src/test/java/org/openjdk/skara/bots/notify/mailinglist/MailingListNotifierTests.java line 84:

> 82:             updater.attachTo(notifyBot);
> 83: 
> 84:             // One mail should be sent on first commit

So all these tests needed to be modified because the test repos are so small the fall below the threshold?

bots/notify/src/test/java/org/openjdk/skara/bots/notify/mailinglist/MailingListNotifierTests.java line 1229:

> 1227:             localRepo.push(updateHash,repo.url(),"master");
> 1228: 
> 1229:             // No mail should be sent on first commit because it has a long history(commit times > 5)

Suggestion:

            // No mail should be sent on first commit because it has a long history(commit count > 5)

vcs/src/main/java/org/openjdk/skara/vcs/Repository.java line 301:

> 299:     }
> 300: 
> 301:     int getExistingCommits() throws IOException;

Suggestion:

    int commitCount() throws IOException;

Also, this should be moved to `ReadOnlyRepository` as it's a pure read operation.

vcs/src/main/java/org/openjdk/skara/vcs/hg/HgRepository.java line 1500:

> 1498:     @Override
> 1499:     public int getExistingCommits() throws IOException {
> 1500:         //TODO:: implement it later, return 10 here so that it will maintain the previous behavior

Should probably throw `UnsupportedOperationException` if not implementing this. I know we aren't using this for any HG repos in production. Is there any test that would trip this?

You could also implement it. A reasonable command line would be

hg id --num --rev tip

and then add 1 to that number, because the first commit in HG is numbered 0.

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

PR: https://git.openjdk.org/skara/pull/1385