Fix GithubReleasesUpdateInformation::buildUrl() #253
Merged
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.
Fixes #252
The release asset matching is currently broken on
main, as it compares against thebrowser_download_urlinstead ofname, which can make the AppImage's update pattern accidentally match the repo name in the download URL instead of just the release asset names, leading to a wrong asset selection when trying to update. I have described the issue in detail in #252.This incorrect matching in the URL is only possible because of the added asterisk char
*at the beginning of the pattern a few lines above my changes, here:AppImageUpdate/src/updateinformation/GithubReleasesZsyncUpdateInformation.cpp
Lines 49 to 50 in 673af3c
I was not sure if you'd want to keep this for compatibility reasons, so I didn't change it.
The AppImage spec however explicitly mentions the file name for matching, not the download URL, so maybe this should be removed, considering the spec's pattern example.
In regards to the sorting at the bottom that is based on the
browser_download_urlvalue, that should be fine, as it's the same sorting result after only adding the entries with a matchingnameto thematchingUrls.I have validated these changes and the current version from
mainon a temporary test repo (with a renamed repo while using themainversion, for proving the issue).