fix(doc): apply redirects to git history at load time#655
Conversation
Move the `GIT_HISTORY` static and `git_history()` accessor from `rari-types::globals` into a new `rari-doc::git_history` module. `HistoryEntry` stays in `rari-types`. Both existing call sites are already in `rari-doc`, so this is a pure relocation with no behavior change. Sets up a follow-up that needs to compose `git_history` with `rari-doc::redirects` at load time.
When `sync-translated-content` moves a translated doc to match an updated en-US slug, the move is local to the build environment and is never committed back to translated-content. As a result, the doc's new path has no entry in `_git_history.json`, the build pipeline's lookup misses, and the page footer shows `1970-01-01T00:00:00.000Z` for the last-modified date and an empty hash for the last-commit URL. Compose `git_history` with `redirects` at load time: for every `(old_url, new_url)` pair, if the corresponding old file path has a history entry and the new path doesn't, copy the entry under the new key. Entries that already exist at the new path (e.g. from a `content move` whose new location is committed) are preserved.
|
1fd8fbe was deployed to: https://rari-pr655.review.mdn.allizom.net/ |
LeoMcA
left a comment
LeoMcA
left a comment
There was a problem hiding this comment.
Code is good, I just think it's in the wrong place: see my last comment
| fn load_history_files() -> HashMap<PathBuf, HistoryEntry> { | ||
| let f = content_root().join("_git_history.json"); | ||
| let mut map = if let Ok(json_str) = fs::read_to_string(f) { | ||
| serde_json::from_str(&json_str).expect("unable to parse l10n json") |
There was a problem hiding this comment.
| serde_json::from_str(&json_str).expect("unable to parse l10n json") | |
| serde_json::from_str(&json_str).expect("unable to parse _git_history.json") |
| let f = translated_root.join("_git_history.json"); | ||
| if let Ok(json_str) = fs::read_to_string(f) { | ||
| let translated: HashMap<PathBuf, HistoryEntry> = | ||
| serde_json::from_str(&json_str).expect("unable to parse l10n json"); |
There was a problem hiding this comment.
| serde_json::from_str(&json_str).expect("unable to parse l10n json"); | |
| serde_json::from_str(&json_str).expect("unable to parse _git_history.json"); |
| //! Redirects from `_redirects.txt` are applied at load time so that pages whose | ||
| //! source file was moved (locally by `sync-translated-content`, or in a build | ||
| //! that ran before `git-history` could pick up a `content move`) still resolve |
There was a problem hiding this comment.
I'm confused by this comment, as git-history does run before the build:
There was a problem hiding this comment.
Oh, I see, we don't commit - so these redirects don't actually get put into _git_history.json, even though they exist on disk.
I'd suggest we do this resolution when we build that JSON, patching it at load time seems unnecessary and a bit confusing (that the on-disk representation differs from the in memory one).
If there's some reason we can't do it at JSON-build time, I'd at least rename the function here to clarify that it resolves to something different, maybe file_history or similar.
I'd also remove this comment and hang one off the git_history function, and make it assume a little less knowledge about the internals, something like:
Loads a map of history entries for each file, resolving redirects when files exist but aren't yet committed (as we'd expect to see during a build after
sync-translated-contentis run)
3 participants
Description
Update
git_history, applying redirects at load time so moved pages resolve to a real history entry instead of the 1970-01-01 default.GIT_HISTORY/git_history()fromrari-typesto a newrari-doc::git_historymodule.(old_url, new_url)redirect, copy the old path's history entry to the new path (preserving any existing entry at the new path).Motivation
Prevent the build from falling back to 1970-01-01 for translated docs that
sync-translated-contentmoved locally during the build.Additional details
Related issues and pull requests
Fixes #247