Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix reflist diffs failing to compact when one of the inputs ends #1275

Merged
merged 2 commits into from
Apr 24, 2024

Conversation

neolynx
Copy link
Member

@neolynx neolynx commented Apr 17, 2024

Replaces #1233

Requirements

All new code should be covered with tests, documentation should be updated. CI should pass.

Description of the Change

The previous reflist logic would early-exit the loop body if one of the lists was empty, but that skips the compacting logic entirely.

Instead of doing the early-exit, we can leave a list's ref as nil when the list end is reached and then flip the comparison result, which will essentially treat it as being greater than all others. This should preserve the general behavior without omitting the compaction.


I suspect this might fix #282 and/or #287, but I haven't tested that.

An alternate approach in the same vein is to create a "sentinel" slice like:

sentinel := []byte{255}

and then use that when we reach the end of a reflist, which avoids the need to flip the comparison result. However, the code later on that checks if the ref is set would then look like:

if pl == nil && rl[0] != sentinel[0]

which felt worse to me while also being a tad wasteful (extra slice allocation every call!). That being said, omitting the comparison flip could certainly be seen as cleaner.

Checklist

  • unit-test added (if change is algorithm)
  • functional test added/updated (if change is functional)
  • man page updated (if applicable)
  • bash completion updated (if applicable)
  • documentation updated
  • author name in AUTHORS

Copy link

codecov bot commented Apr 17, 2024

Codecov Report

Attention: Patch coverage is 83.87097% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 74.81%. Comparing base (27013c0) to head (a599bb1).

Files Patch % Lines
deb/publish.go 70.58% 4 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1275      +/-   ##
==========================================
+ Coverage   74.78%   74.81%   +0.02%     
==========================================
  Files         144      144              
  Lines       16246    16248       +2     
==========================================
+ Hits        12150    12156       +6     
+ Misses       3154     3152       -2     
+ Partials      942      940       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@neolynx neolynx self-assigned this Apr 18, 2024
@neolynx neolynx changed the title Feature/publish perf reflist diff Fix reflist diffs failing to compact when one of the inputs ends Apr 20, 2024
@neolynx neolynx added needs review Ready for review & merge needs rebase The PR needs to be rebased on master labels Apr 20, 2024
@neolynx neolynx force-pushed the feature/publish-perf-reflist-diff branch 2 times, most recently from 8bc1b57 to 7d742f9 Compare April 21, 2024 09:32
@neolynx neolynx removed the needs rebase The PR needs to be rebased on master label Apr 21, 2024
@neolynx neolynx removed the needs review Ready for review & merge label Apr 24, 2024
The output doesn't actually depend on the reflists, and loading them for
every published repo starts to take substantial time and memory.

Signed-off-by: Ryan Gonzalez <ryan.gonzalez@collabora.com>
The previous reflist logic would early-exit the loop body if one of the
lists was empty, but that skips the compacting logic entirely.

Instead of doing the early-exit, we can leave a list's ref as nil when
the list end is reached and then flip the comparison result, which will
essentially treat it as being greater than all others. This should
preserve the general behavior without omitting the compaction.

Signed-off-by: Ryan Gonzalez <ryan.gonzalez@collabora.com>
@neolynx neolynx force-pushed the feature/publish-perf-reflist-diff branch from 7d742f9 to a599bb1 Compare April 24, 2024 14:52
@neolynx neolynx merged commit 79975bf into master Apr 24, 2024
9 checks passed
@neolynx neolynx deleted the feature/publish-perf-reflist-diff branch April 24, 2024 15:36
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.

aptly diff misses some packages
3 participants