Repository navigation
Work out which commits are new by ancestry, so merge-commit revisions work. Fixes #438 - #439
Merged
Merged
Conversation
… work. Fixes #438 To mark the commits that are new since the previous job, check_for_update looked for the previous job's revision in the list of upstream commits, which is built with `git log --no-merges`. For libraries whose revisions are merge commits (every PDF.js revision is a pull-request merge) it was never found, so every commit was treated as new and the bug never said which ones were. Decide instead whether the previous job is between the in-tree revision and the new one with `git merge-base --is-ancestor`. For the PDF.js bugs in 2083225..2083542 this gives 5, 3, 13, 1, 7, 12 and 2 new commits instead of all of them; for linear histories the result is unchanged. The description says the new commits are "the top N", but git lists commits by date, and with merges a new commit can be older than ones from the previous update (it was in 2 of those 7 bugs), so list the new commits first. The "All N commits" compare link in the verbose-diff layout was also built from the list (parent of the oldest commit ... newest commit), which misses commits when the new version is a merge: for 2083499 it would have covered 32 of 33. Vendoring now passes the in-tree revision and the new version instead. Also repair tests/scm.py, which wasn't being run and had rotted, add merge history tests to it, run it from test.py, and teach the functionality tests' fake git about ancestry.
There was a problem hiding this comment.
🟡 Changes recommended
The ancestry check misclassifies previously reported side-branch commits as unseen, potentially repeating reports and inflating commit-frequency counts.
1 open issue
What changed in this PR
This PR makes upstream commit tracking ancestry-aware so merge-commit revisions are handled correctly in update notifications.
Changes:
- Use ancestry checks to identify commits added since the previous job.
- List unseen commits first and use explicit revision endpoints for verbose compare links.
- Restore SCM tests, add merge-history coverage, and extend the fake Git commands.
| File | Description |
|---|---|
| tests/scm.py | Repairs existing tests and adds merge-history regression coverage. |
| tests/functionality_utilities.py | Adds fake Git ancestry and revision-resolution responses. |
| tests/build_bug_description.py | Tests explicit compare-link endpoints. |
| test.py | Includes SCM tests in the test runner. |
| tasktypes/vendoring.py | Passes unseen commits and revision endpoints to description generation. |
| components/scmprovider.py | Adds ancestry checks, unseen-first ordering, and explicit compare ranges. |
Review effort: Balanced
…ion as seen The previous commit decided whether the most recent job was newer than the in-tree revision by checking that the in-tree revision was its ancestor. A commit-alert job can be a commit on a side branch that forked before the in-tree revision and was merged after it; such a commit isn't a descendant of the in-tree revision, so it (and everything else) was reported as new again. (Before the previous commit this case raised instead, because the commits after the job included ones already in-tree.) Check instead that the job is reachable from the new version but not from the common ancestor, i.e. it is one of the update's commits, and restrict the unseen commits to the update's commits. This also makes the separate check that the job isn't the in-tree revision unnecessary.
If the new version only adds merge commits since the previous job, there are no unseen commits, and check_for_update returned early without populating the details of the update's commits. Vendoring then failed to build the bug description and filed the bug saying it couldn't get the upstream commits. This was possible before, but merge-commit jobs (e.g. every PDF.js revision) now take this path. Carry on instead, so the details are populated. The description would then have said "including 0 new upstream commits ... (They're the top 0.)", so say that they've all been filed before instead.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
To mark the commits that are new since the previous job, check_for_update looked for the previous job's revision in the list of upstream commits, which is built with
git log --no-merges. For libraries whose revisions are merge commits (every PDF.js revision is a pull-request merge) it was never found, so every commit was treated as new and the bug never said which ones were. Decide instead whether the previous job is between the in-tree revision and the new one withgit merge-base --is-ancestor. For the PDF.js bugs in 2083225..2083542 this gives 5, 3, 13, 1, 7, 12 and 2 new commits instead of all of them; for linear histories the result is unchanged.The description says the new commits are "the top N", but git lists commits by date, and with merges a new commit can be older than ones from the previous update (it was in 2 of those 7 bugs), so list the new commits first.
The "All N commits" compare link in the verbose-diff layout was also built from the list (parent of the oldest commit ... newest commit), which misses commits when the new version is a merge: for 2083499 it would have covered 32 of 33. Vendoring now passes the in-tree revision and the new version instead.
Also repair tests/scm.py, which wasn't being run and had rotted, add merge history tests to it, run it from test.py, and teach the functionality tests' fake git about ancestry.