Original fixes#413
Closed
huimouG wants to merge 19 commits into
Closed
Conversation
…er/status Implements Git-compatible maintenance command for Libra: - run: executes gc, loose-objects, pack-refs, incremental-repack, commit-graph, and prefetch tasks - register/unregister: manage repo maintenance registration via ConfigKv - status: show maintenance state with JSON support - dry-run mode for safe preview of changes - simplified pack file creation for loose-objects and incremental-repack Signed-off-by: Student <student@example.com>
Coverage includes: - 8 basic functionality tests (run, gc, register, status, dry-run, etc.) - 9 boundary condition tests (empty repo, few objects, skipped tasks, etc.) - 9 error handling tests (outside repo, invalid flags, dangling objects, etc.) - 12 unit tests for helpers (hash parsing, size encoding, header parsing) All tests pass with cargo test --test command_test maintenance Signed-off-by: Student <student@example.com>
- Add docs/commands/maintenance.md with full usage and examples - Update CHANGELOG.md with maintenance feature entry - Update README.md to mark maintenance as implemented Signed-off-by: Student <student@example.com>
…mands Adds maintenance to the VISIBLE_COMMANDS allowlist so the compat_help_examples_banner regression test passes. Signed-off-by: Student <student@example.com>
…er tasks - Append .pack extension to generated pack files so LocalStorage can discover them. - Build standard .idx index files after creating packs. - Verify every packed object is readable before deleting source loose objects. - Verify consolidated pack before deleting source packs in incremental-repack. - Return non-zero exit code when JSON output reports task failures. - Add regression tests for pack readability, GC reachability, and JSON exit codes. Signed-off-by: Student <student@example.com>
- Add maintenance to ROOT_AFTER_HELP group listing - Propagate config lookup failures in maintenance status - Use repository hash algorithm for pack trailers - Handle delta entries during incremental repack via Pack::decode - Count dry-run GC removals before branching - Move maintenance to COMPATIBILITY.md matrix and out of README pending list Signed-off-by: Student <student@example.com>
…repack rollback - Use nanosecond-based unique pack file names to avoid collisions - Write fully-qualified refnames in packed-refs and verify the file - Keep reflog old_oid and annotated tag targets reachable for GC - Roll back staged packs and clean up partial packs on incremental-repack failure - Handle mutex poison errors in pack decode callback - Add regression tests for refnames, annotated tags, reflog old_oid Signed-off-by: Student <student@example.com>
- Walk annotated tag targets during GC reachability analysis - Use nanosecond-unique pack names to avoid collision overwrites - Roll back staged packs when incremental-repack staging/verification fails - Replace unwrap on pack decode mutex with Result-based error handling - Treat reflog old OIDs as GC roots - Write fully-qualified refnames into packed-refs - Add GC grace period based on object mtime to avoid concurrent-write races - Write packed-refs atomically via temp file + rename - Fix v2 index large-offset seek in read_idx_entries Signed-off-by: Student <student@example.com>
…stream repacks Signed-off-by: Student <student@example.com>
…tream repack Also fix stash push to persist the index tree object; without it the stash index commit referenced a missing tree, causing GC reachability walks to fail. Signed-off-by: Student <student@example.com>
… objects, propagate last-run errors Signed-off-by: Student <student@example.com>
Teach collect_reachable_objects to read packed-refs and scan file-backed loose refs under refs/ so that pack-refs+g c does not delete objects reachable only through packed refs. Without this, after pack-refs removes loose ref files, a subsequent gc treats those commits as unreachable once the mtime grace period expires. - Add collect_file_ref_hashes helper to recursively scan refs/ directory - Walk every hash found in packed-refs (skip comments, empty lines, peeled markers) - Walk every hash found in file-backed loose refs under refs/ Co-Authored-By: Claude <noreply@anthropic.com>
…d-refs replacement Address three Codex review findings: - Fail GC when file-backed refs are unreadable or contain non-hex content instead of silently skipping them (collect_file_ref_hashes). - Create pack staging temp dirs inside objects/pack/ instead of the system /tmp to avoid cross-device rename failures when /tmp is a separate mount. - Remove the old packed-refs before renaming the new one into place so that the install works portably on Windows where std::fs::rename fails when the destination already exists. Co-Authored-By: Claude <noreply@anthropic.com>
…ot validation Address three Codex review findings: - Keep a packed-refs.bak backup during replacement so that a rename failure after the old file is moved aside does not lose all refs that exist only in packed-refs. Restore from backup on failure, remove the backup on success. - Abort GC when the stash reflog exists but cannot be read, instead of silently skipping it and potentially pruning older stash entries reachable only through that reflog. - Abort GC when a non-comment packed-refs line contains a malformed or truncated object hash, instead of silently skipping the line and potentially deleting objects reachable only from that ref. Co-Authored-By: Claude <noreply@anthropic.com>
….md link Address two Codex review findings: - Stop deleting loose ref files after writing packed-refs. Normal ref-resolution paths (show-ref, rev-parse, revision walking) currently only query SQLite-backed refs and file-backed loose refs under refs/ — they do not yet read packed-refs. Deleting the loose files makes those refs invisible to everyday commands. Keep the loose files in place until the ref-resolution layer learns to read packed-refs. The packed-refs file is still written so GC can use it. - Remove dead 'libra gc' cross-reference link in docs/commands/maintenance.md. The gc.md page does not exist yet, so linking to it would send users to a missing page. Remove now-unused remove_packed_refs function.
…ing GC When maintenance gc deletes an unreachable loose object, the corresponding row in object_index must also be removed. In repos with cloud sync enabled, a pruned object that still has is_synced = 0 will continue to be selected for upload, and sync_single_object will fail reading it from local storage, causing repeated cloud sync failures after a successful maintenance run. Add gc_drop_object_index helper that deletes the object_index row by o_id and repo_id. When libra.repoid is not set (no cloud sync configured), the helper returns early without error.
…orted Address two Codex review findings: - Root all index entry stages (0-3) during GC instead of only stage 0. When a merge conflict is left unresolved, Libra stores base/ours/theirs blobs as index stages 1-3. Previously only stage-0 entries were rooted, so objects needed for conflict recovery could be pruned as unreachable. - Make prefetch check for configured remotes and return success=false with 'not yet implemented' when remotes exist. Previously the task unconditionally returned a successful skipped result without checking, causing a silent no-op in repos with remotes configured.
…ck all ref categories Address three Codex review findings: - Always build v2 pack indexes for maintenance-generated packs. V1 stores offsets as u32 so objects past 4 GiB get truncated offsets, making them unreadable after source packs are deleted. V2 includes a large-offset table for 64-bit offsets. - Return success:true for the prefetch task even when remotes are configured. The default libra maintenance run includes prefetch, and cloned repos almost always have remote.origin.*, so success:false caused the advertised run-all command to fail by default. Prefetch now reports 'not yet implemented; skipping' as a non-fatal notice. - Scan all well-known ref categories (heads/, tags/, remotes/, notes/) instead of only refs/heads in the pack-refs task. File-backed tags and remote-tracking refs were previously silently ignored.
Signed-off-by: huimouG <jhdivdihw@gmail.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5eb77a435a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Comment on lines
+1592
to
+1600
| let hash = parse_object_hash(content.trim()).ok_or_else(|| { | ||
| io::Error::new( | ||
| io::ErrorKind::InvalidData, | ||
| format!( | ||
| "loose ref {} does not contain a valid object hash", | ||
| path.display() | ||
| ), | ||
| ) | ||
| })?; |
There was a problem hiding this comment.
In repositories that contain valid file-backed symbolic refs such as refs/remotes/origin/HEAD with content ref: refs/remotes/origin/main (the new pack-refs coverage already creates and skips this shape), GC treats the file as a malformed object hash and aborts the default maintenance run during the ref-root scan. Handle ref: files by resolving their target, or skip the symref after ensuring the target ref is collected, instead of failing maintenance for common remote HEAD refs.
Useful? React with 👍 / 👎.
| // cannot leave the previous packed-refs truncated. | ||
| let temp_path = packed_refs_path.with_extension("tmp"); | ||
| let mut file = fs::File::create(&temp_path) | ||
| .map_err(|e| CliError::fatal(format!("failed to create packed-refs temp: {e}")))?; | ||
| if let Err(e) = writeln!(file, "# packed-refs with peeled tags") { |
There was a problem hiding this comment.
This header is not accepted by Git's packed-refs parser; the standard header form is # pack-refs with: ... (or omit capabilities), while # packed-refs with peeled tags makes tools such as git show-ref reject the file as an unexpected line. Since this command advertises Git-compatible pack-refs output and rewrites any existing packed-refs file, use the standard header before installing the file.
Useful? React with 👍 / 👎.
Comment on lines
+851
to
+857
| if let Err(e) = fs::rename(old_pack, &backup_pack) { | ||
| stage_error = Some(format!( | ||
| "failed to remove old pack {}: {e}", | ||
| old_pack.display() | ||
| )); | ||
| break; | ||
| } |
There was a problem hiding this comment.
When incremental-repack overlaps a normal command that is reading packed objects, this rename removes a pack path that LocalStorage::get_from_pack may already have enumerated; that reader propagates the missing idx/pack read error instead of retrying against the newly created pack, so commands like log/checkout can fail mid-read even though the object was repacked. Keep old packs available until no readers can have observed them, or add a shared pack lock/retry-on-ENOENT before moving them out of objects/pack.
Useful? React with 👍 / 👎.
Author
|
@genedna 您好，我想要实现的功能maintenance似乎已经被您实现，请问能否麻烦您抽空review一下我的实现，然后看看是否值得被合并？ |
Contributor
|
This PR has been simplified and replaced by PR #414. The original PR contained many changes that were already implemented in the main branch. PR #414 focuses only on the two critical bug fixes that were actually missing:
Please use PR #414 instead. |
Contributor
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
2 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.
No description provided.