feat: optional fragment on redirects, per kelson42's dirent-parameter design - #1125
Open
soloturn wants to merge 75 commits into
Open
feat: optional fragment on redirects, per kelson42's dirent-parameter design#1125soloturn wants to merge 75 commits into
soloturn wants to merge 75 commits into
Conversation
Release 9.7.0
Multiple CI/CD deps updates
Fix Typos in ICU dep part of meson.build
This reduces the risk of losing the main results of ZIM creation because of an IO error during the checksum computation step. Now, in such a situation the checksum can be added to the quasi-invalid ZIM file by a simple external script.
zim::writer::BinaryFile is introduced with the purpose of encapsulating all file operations (currently happening via syscalls on a file descriptor) in a class, that, upon completion, can be trivially reimplemented via buffered IO.
Didn't apply the new method in src/writer/cluster.cpp since it is used a little differently there.
This slightly changes the chunked writing of large non-compressed clusters: - the chunk size is reduced from 4GiB to 16MiB, - and a failure to fully write the current chunk results in an exception
…estions_for_xapian_2.0 Force single-letter suggestions for Xapian 2.0
…r jammy-x86_64-dyn
Conditionally exclude writer from build
The non-orchestrated flow in the Log.concurrencyOrchestration unit-test is unstable by its nature due to the absence of any guarantees on the order of unsynchronized concurrent operations performed in different threads. Suspensions of threads by a given duration only increase the probability of achieving the desired scenario, with longer wait times having higher chances of working as intended even on slow hardware or under high load.
A little stabler unit-test Log.concurrencyOrchestration
Properly handle libicu 76+ in Meson
Release 9.8.0
Previous implementation of millisleep() could overshoot significantly when the thread was not given enough CPU time. Suppose that the code is running in a high contention environment, competing for CPU time with a lot of other processes/tasks, so that the thread only wakes up once every N milliseconds. Then a call to the old implementation of millisleep(T) could take N*T milliseconds! With the new implementation the same call will take at most T+N+O(1) milliseconds (where O(1) stands for the fixed duration that it takes to enter and exit the function; it applies to the old implementation too, but there it was not worth mentioning).
Fixed the test utility millisleep()
Rename to .zim file, after the checksum append
Release 9.8.1
Made the stemmer set-up code identical across SuggestionDataBase::initXapianDb() and XapianDbMetadata::XapianDbMetadata().
This is a quick fix to zimcheck's output being polluted by libzim's own unsolicited messages. Writer code keeps printing to stdout but it's less of a concern
…lution Quick fix for pollution of zimcheck output
Release 9.8.2
…arameter Implements the design kelson42 settled on for openzim#1001 (redirect with fragment support), replacing the approach in openzim#1082 ([WIP] Support for URL redirects / resolveTargetURL), which kelson42 rejected on this thread and on the issue: openzim#1001 (comment) openzim#1082 (comment) His stated requirements, and how this meets each of them: - "Against any modification of the meaning of # in the path" -- this adds no path parsing at all. No '#'/'?' component-splitting utilities. - "We have a way to store the fragment string in the corresponding dirent parameter and we should use that." -- the redirect dirent's existing "parameter" field (present in the format since the beginning, always empty for redirects until now) now optionally carries the fragment. - "Very important is to start a proper/robust (json?) structure in this dirent parameter... allow further extensions in the future" -- rather than pull in a JSON dependency for one optional string, the parameter is a minimal tagged (TLV) record: 1 byte tag + 1 byte length + value. A tag this version doesn't recognize is skipped, not misread, so a future record type (e.g. openzim#317's keywords) can share the same field without breaking older readers. - "We should not create a new type or new naming of redirects... just allow to get/set an optional fragment with the current standard redirection related API primitives." -- Creator::addRedirection() gained one new optional parameter (targetFragment, default ""), and Entry gained one new method (getRedirectFragment()) alongside the existing getRedirectEntry()/getRedirectEntryIndex(). No new redirect type, no "smart" resolution, no mini-HTML inspection anywhere in libzim. A redirect that doesn't use a fragment is byte-for-byte identical to one written before this feature existed: encodeRedirectParameter("") returns "", exactly the empty parameter every redirect has always had. ## Changes - src/tools.h/.cpp: encodeRedirectParameter()/decodeRedirectFragment(), the TLV codec described above, MAX_REDIRECT_FRAGMENT_SIZE (253 bytes = the 255-byte on-disk parameter limit minus the record's own 2-byte header). - src/writer/_dirent.h, src/writer/dirent.cpp: writer::Dirent gained set/getRedirectFragment() and now actually writes non-zero parameter bytes for the first time (this field existed in the format but the writer always hardcoded its length to 0). Stored as shared_ptr<const string>, not an inline string: DirentTest.size exists specifically because Dirent's in-memory size matters at the scale of the millions of entries a large ZIM can have, and a field almost no dirent uses shouldn't cost more than a pointer on the ones that don't (shared_ptr, not unique_ptr, because Dirent is copy-constructed into DirentPool's storage - confirmed by an actual compile error when I first tried unique_ptr). This does grow Dirent by one pointer's worth of bytes (29 -> 45 on 64-bit here) for every dirent, not just redirects with fragments; flagging this plainly since it's a real, measured memory cost of the feature, worth a maintainer's eyes on the tradeoff. - include/zim/writer/creator.h, src/writer/creator.cpp: addRedirection() gained targetFragment as a new trailing defaulted parameter (after hints, not before, to avoid an overload-resolution ambiguity against braced-init-list Hints arguments already used throughout the existing test suite - confirmed by hitting that exact ambiguity with a first attempt that added a new overload instead). - include/zim/entry.h, src/entry.cpp: Entry::getRedirectFragment(), same InvalidType-on-non-redirect convention as the existing getRedirectEntryIndex(). - test/tooltesting.cpp: encode/decode round-trip, empty-fragment, overlong-fragment-throws, and forward-compatibility (unknown record tag skipped, truncated parameter degrades to "" rather than crashing) tests for the codec directly. - test/dirent.cpp: writer/reader round-trip test with a fragment set, a same-as-before-this-feature round-trip test with none, and an updated DirentTest.size (see above -- the Windows/32-bit constants are recalculated from sizeof(shared_ptr) on those platforms, not measured; only the 64-bit non-Windows value has actually been run). - test/archive.cpp: end-to-end Creator -> Archive -> Entry test (redirectWithFragment) covering a redirect with a fragment, one without, and the InvalidType-on-non-redirect case. ## Verification Full test suite (31/31) passes. Confirmed via gcovr (llvm-cov gcov backend) that every line touched by this diff is exercised: 100% on src/writer/_dirent.h, and on src/entry.cpp/src/tools.cpp/ src/writer/creator.cpp/src/writer/dirent.cpp the only uncovered lines reported are pre-existing code untouched by this diff (verified by line number against the actual patch, not just the file-level percentage). ## Not attempted Anything from openzim#1082 that depended on inspecting item content (mini-HTML detection/resolution) or interpreting '#'/'?' as meaningful path characters - both are exactly what kelson42's comments rule out, so none of that carries over here. The mini-HTML workaround remains the only option for readers that don't yet understand this fragment field, same as kelson42's stated interim plan. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
|
@soloturn Can we have a PR which does only things related to the issue and nothing else please? |
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
6 participants
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.
Alternative to this PR's
resolveTargetURL/UrlUtilsapproach, implementing the design @kelson42 actually asked for:and, from the linked issue:
What this does instead
#/?component splitting, nothing that gives those characters special meaning in a ZIM path.parameterfield (present in the format from the start, always empty for redirects until now) optionally carries the fragment, as a minimal tagged record (1-byte tag + 1-byte length + value) rather than pulling in a JSON dependency for one optional string — a tag this version doesn't recognize is skipped, not misread, so a future record type (e.g. Feature request: Add keyword/tag support #317's keywords) can share the field without breaking older readers.Creator::addRedirection()gained one new optional trailing parameter (targetFragment, default"");Entrygained one new method (getRedirectFragment()) alongside the existinggetRedirectEntry()/getRedirectEntryIndex().A redirect that doesn't use a fragment is byte-for-byte identical to one written before this existed —
encodeRedirectParameter("")returns"", the same empty parameter every redirect has always had.Worth a maintainer's eyes on
Storing the fragment as a
shared_ptr<const string>onwriter::Dirent(not an inlinestring, notunique_ptr—Direntis copy-constructed intoDirentPool's storage, which rules out move-only types) still growsDirentby one pointer's worth of bytes — 29 → 45 on 64-bit here — for every dirent, not just redirects with fragments.DirentTest.sizeexists specifically because this matters at the scale of a large ZIM's entry count, so flagging this plainly rather than glossing over it.Verification
src/writer/_dirent.h; on the other touched files, the only uncovered lines reported are pre-existing code outside this diff (checked by line number against the actual patch, not just the file-level percentage).test/tooltesting.cpp), writer/reader dirent round-trip with and without a fragment (test/dirent.cpp), and an end-to-endCreator→Archive→Entrytest (test/archive.cpp).Not attempted
Anything depending on inspecting item content or interpreting
#/?as meaningful path characters — both are exactly what's ruled out above, so nothing from this PR'sUrlUtils/mini-HTML-parsing approach carries over. The mini-HTML workaround remains the only option for readers that don't understand this new fragment field yet, matching @kelson42's stated interim plan on #1001.Related: #1001, #317 (future dirent-parameter extension mentioned there).