-
-
Notifications
You must be signed in to change notification settings - Fork 359
Description
Current behavior 😯
There were three broken links found while working on #1624 that I didn't include fixes for in 65f69f7 because there was no clear best way to fix them. For two of them, which will probably require updating planning information. I am not at all sure what should be done, though I've tried to suggest broad possibilities. Those are the ones that motivate this issue. For the third, I'm pretty sure what the correct fix is and I'll go ahead and open a PR for it, but I've included it here as well so it can be easily reviewed.
Please note that the broken links described in this issue were effectively unaffected by the change of Byron/gitoxide
to GitoxideLabs/gitoxide
in 64ff0a7: although that part of their URLs changed, they were broken in exactly the same way both before and after that change to their URLs.
In crate-status.md
That link is a 404, because it specifies a gix-pack
path at a commit before git-pack
was renamed to gix-pack
:
Line 232 in 795962b
* [ ] [fast pack traversal works with ref-deltas](https://github.com/GitoxideLabs/gitoxide/blob/8f9a55bb31af32b266d7c53426bc925361a627b2/gix-pack/src/cache/delta/from_offsets.rs#L101-L105) |
Changing the path back to git-path
gives:
gitoxide/git-pack/src/cache/delta/from_offsets.rs
Lines 101 to 105 in 8f9a55b
// TODO: ref-deltas can also point to objects (by offset) which we haven't seen yet, so this should really be | |
// handled maybe by delaying those? Add-Child needs things ordered ascending though, so it's not trivial to do | |
// and probably requires tham offset-rewriting. | |
// Note that some packs created by libgit2 generally look like that, see the 'index-bare' repo that is operated on by git2 | |
// and try the 'pack-verify' with the less-time algorithm for reproduction. |
With more context, this is:
gitoxide/git-pack/src/cache/delta/from_offsets.rs
Lines 100 to 110 in 8f9a55b
RefDelta { base_id } => { | |
// TODO: ref-deltas can also point to objects (by offset) which we haven't seen yet, so this should really be | |
// handled maybe by delaying those? Add-Child needs things ordered ascending though, so it's not trivial to do | |
// and probably requires tham offset-rewriting. | |
// Note that some packs created by libgit2 generally look like that, see the 'index-bare' repo that is operated on by git2 | |
// and try the 'pack-verify' with the less-time algorithm for reproduction. | |
resolve_in_pack_id(base_id.as_ref()) | |
.ok_or(Error::UnresolvedRefDelta { id: base_id }) | |
.and_then(|base_pack_offset| { | |
tree.add_child(base_pack_offset, pack_offset, data).map_err(Into::into) | |
})?; |
But the code comment, which crate-status.md
links to, went away in 84ade1d. The commit message there is:
fix: Allow resolution of in-pack ref-deltas (#287)
This finally allows delta tree caches to be used on typical small packs returned by GitHub.
So maybe the item in crate-status.md
is (partially?) completed or otherwise should be revised?
In general-tasks.md
That link is a 404, both because it specifies gix-odb
at a commit before git-odb
was renamed to gix-odb
, and because the structure and content of the crate has changed such that I am not sure if any corresponding code still exists:
Lines 15 to 19 in 795962b
### gix-odb | |
* Finding an object in a loose object store [costs an extra disk IO operation][extra-iop] to pacify the borrow checker. This wouldn't be an issue with polonius. | |
[extra-iop]: https://github.com/GitoxideLabs/gitoxide/blob/2958145a0ae1ef582bbf88352f5567d5c2b5eaf0/gix-odb/src/store/linked/find.rs#L33 |
Changing the path back to git-odb
gives:
gitoxide/git-odb/src/store/linked/find.rs
Line 33 in 2958145
if db.loose.contains(id) { |
With more context, this is:
gitoxide/git-odb/src/store/linked/find.rs
Lines 11 to 14 in 2958145
impl crate::Find for linked::Store { | |
type Error = compound::find::Error; | |
fn find<'a>( |
gitoxide/git-odb/src/store/linked/find.rs
Line 22 in 2958145
match db.internal_find(id) { |
gitoxide/git-odb/src/store/linked/find.rs
Lines 32 to 35 in 2958145
None => { | |
if db.loose.contains(id) { | |
return db.loose.find(id, buffer).map_err(Into::into); | |
} |
As far as I can tell, this went away without being replaced by anything equivalent in 8c5ae77:
change!: Remove deprecated compound and linked object databases
The dynamic/general store is the only maintained can-do-it-all DB now.
So maybe this item should be removed (or heavily revised?) from general-tasks.md
?
In gix-diff/src/tree/function.rs
The git_cmp_rs
link in the gix_diff::tree::function::diff
function's documentation comment is a 404, because it specifies a gix-object
path at a commit before git-object
was renamed to gix-object
, and because the crate has been substantially reorganized:
gitoxide/gix-diff/src/tree/function.rs
Line 18 in 795962b
/// * Tree entries are expected to be ordered using [`tree-entry-comparison`][git_cmp_c] (the same [in Rust][git_cmp_rs]) |
gitoxide/gix-diff/src/tree/function.rs
Line 27 in 795962b
/// [git_cmp_rs]: https://github.com/GitoxideLabs/gitoxide/blob/a4d5f99c8dc99bf814790928a3bf9649cd99486b/gix-object/src/mutable/tree.rs#L52-L55 |
Changing the path back to git-object
gives:
gitoxide/git-object/src/mutable/tree.rs
Lines 52 to 55 in a4d5f99
fn cmp(&self, other: &Self) -> Ordering { | |
let len = self.filename.len().min(other.filename.len()); | |
self.filename[..len].cmp(&other.filename[..len]) | |
} |
With more context, this is:
gitoxide/git-object/src/mutable/tree.rs
Lines 50 to 56 in a4d5f99
impl Ord for Entry { | |
/// Entries compare by the common portion of the filename. This is critical for proper functioning of algorithms working on trees. | |
fn cmp(&self, other: &Self) -> Ordering { | |
let len = self.filename.len().min(other.filename.len()); | |
self.filename[..len].cmp(&other.filename[..len]) | |
} | |
} |
This changed substantially in ca37915 (#849). The comment went away, and the implementation was modified to fix #848. Although some code has moved around in the module, that implementation remains today:
gitoxide/gix-object/src/tree/mod.rs
Lines 263 to 273 in 795962b
impl Ord for Entry { | |
fn cmp(&self, b: &Self) -> Ordering { | |
let a = self; | |
let common = a.filename.len().min(b.filename.len()); | |
a.filename[..common].cmp(&b.filename[..common]).then_with(|| { | |
let a = a.filename.get(common).or_else(|| a.mode.is_tree().then_some(&b'/')); | |
let b = b.filename.get(common).or_else(|| b.mode.is_tree().then_some(&b'/')); | |
a.cmp(&b) | |
}) | |
} | |
} |
I'll open a PR that changes the git_cmp_rs
link in the gix_diff::tree::function::diff
documentation comment to point there. [Edit: I've opened #1628 for this.]
Expected behavior 🤔
I think references to code should either be current or otherwise that their significance to future development should be clear. For this reason, even though all links work when gix-*
is changed back to git-*
, I did not make any such changes in #1624 and I am not recommending them here, since it would then not be obvious how the text referencing the code ought to be understood.
Git behavior
Not applicable.
Steps to reproduce 🕹
See above.