Repository navigation
Conversation
Contributor
|
@soloturn Can we have a PR which does only things related to the issue and nothing else please? |
Author
done. |
soloturn
force-pushed
the
feat/redirect-fragment
branch
from
September 16, 2026 08:30
e96cd3e to
453c648
Compare
…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>
soloturn
force-pushed
the
feat/redirect-fragment
branch
from
October 26, 2026 22:55
453c648 to
cebe2cb
Compare
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.
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.Usage
Writing (scrapers, zimwriterfs, python-libzim/libkiwix bindings): pass the new optional argument to point a redirect at a section of the target, not just the target itself.
Reading (kiwix-tools, kiwix-desktop, kiwix-android, libkiwix, or any client walking
Entry): ask a redirect entry for its fragment alongside the existing redirect-target accessors.Every existing caller of
addRedirection()is unaffected —targetFragmentdefaults to"", andgetRedirectFragment()returns""for every redirect written before this feature existed.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).