Skip to content

fix(gitter): Update affected commit logic when introduced=0#5147

Open
Ly-Joey wants to merge 6 commits intogoogle:masterfrom
Ly-Joey:fix-gitter-intro-0
Open

fix(gitter): Update affected commit logic when introduced=0#5147
Ly-Joey wants to merge 6 commits intogoogle:masterfrom
Ly-Joey:fix-gitter-intro-0

Conversation

@Ly-Joey
Copy link
Copy Markdown
Contributor

@Ly-Joey Ly-Joey commented Mar 29, 2026

When introduced=0, we now only evaluate root commits that are ancestors of a fixed or last_affected commit. This prevents disjoint trees in multi-root repos from being incorrectly marked as fully affected when a vuln is only applicable to the tree with the fix.

If no fix / last affected commit, introduced=0 still defaults to evaluating all roots.

Also extracted the event parsing and cherry-pick expansion into a resolveEvents helper function for clarity.

Ly-Joey added 3 commits March 29, 2026 22:18
Now only roots that are ancestors of provided fix commits will be added to affected commit traversal. If no fix exists, all roots will be added (original behaviour).
michaelkedar
michaelkedar previously approved these changes Mar 29, 2026
Copy link
Copy Markdown
Member

@michaelkedar michaelkedar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

another-rex
another-rex previously approved these changes Mar 29, 2026
Copy link
Copy Markdown
Contributor

@another-rex another-rex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Looks great

introduced = append(introduced, newIntro...)
}

return introduced, allFixes, newIntroHashes, newFixedHashes
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: these variables are a bit confusing.

are the hashes the same commits as the other variable?

e.g. is introduced=newIntroHashes
and allFixes=newFixedHashes?

If not can we make them the same?

And if they are the same, can we update the name to make it clearer

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They're not really the same

introduced = intro from input + cherrypicked intro + roots resolved from intro=0
allFixes = fixed from input + children of lastAffected + cherrypicked fixed

The new___Hashes contains only the cherrypicked commit hashes (string) and it was added in #5101 because worker needs them to update the record (with minimal change to existing logic).

I've renamed new___Hashes -> cherrypicked___Hashes in an attempt to make it clearer(?)

@Ly-Joey Ly-Joey dismissed stale reviews from another-rex and michaelkedar via 4b24821 March 29, 2026 23:41
@Ly-Joey Ly-Joey requested a review from another-rex March 29, 2026 23:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants