`Tree::peel_to_entry{_by_path}` does not point to to "last seen tree" if leaf entry is a `Tree`

[datdenkikniet]

Current behavior 😯

Calling Tree::peel_to_entry{_by_path} does not update the tree itself to "the last seen tree" if the returned entry itself is a tree. For example, looking up the entry path/to/a/tree/ causes the tree that is being peeled to become the tree at path/to/a/ instead of the tree at path/to/a/tree/, but from the documentation that is not entirely clear.

I feel like the behaviour is intentional, but the documentation is ambiguous in my opinion.

Naturally I am more than willing to contribute a documentation change to clarify this, but I wanted to make sure it was intentional first.

Expected behavior 🤔

Peeling takes all "seen" elements into account, including the returned Entry.

Git behavior

N/A

Steps to reproduce 🕹

1. Obtain a tree tree that contains a tree sub_tree.
2. Call tree.peel_to_entry_by_path("sub_tree")
3. tree is unchanged, despite peel_to_entry_by_path having "seen" sub_tree.

#[test]
fn repro() -> Result<(), Box<dyn std::error::Error>> {
    use gix::object::tree::EntryKind;

    let repo = gix::open(".")?;
    let mut middle_tree = repo.empty_tree().edit()?;
    middle_tree.upsert("sub_tree", EntryKind::Tree, repo.empty_tree().id)?;
    let middle_tree_id = middle_tree.write()?;

    let mut top_tree = repo.empty_tree().edit()?;
    top_tree.upsert("sub_tree", EntryKind::Tree, middle_tree_id)?;
    let top_tree_id = top_tree.write()?;

    let mut top_tree = repo.find_tree(top_tree_id)?;

    // Peeling into more than one level of trees changes
    // `peel_to_bottom` (though only to `middle_tree`, and
    // not `empty_tree()` as one could expect.
    let mut peel_to_bottom_tree = top_tree.clone();
    peel_to_bottom_tree.peel_to_entry_by_path("sub_tree/sub_tree")?;
    assert_ne!(peel_to_bottom_tree.id, top_tree_id);

    // But peeling into a single level does not change
    // `top_tree`, despite the fact that `peel_to_entry_by_path`
    // has "seen" `sub_tree`.
    top_tree.peel_to_entry_by_path("sub_tree")?;
    assert_ne!(
        top_tree_id, top_tree.id,
        "We saw(?) \"sub_tree\", but \"tree\" is unchanged"
    );

    Ok(())
}

[datdenkikniet]

And, to clarify my use-case: I'm effectively trying to use fn peel_to_entry(&mut self) -> Result<Entry> as a sort of fn peel_to_tree(self) -> Result<Tree>. However, with the current solution I have to convert the returned Entry into a Tree manually, while the work peel_to_entry does seems a lot "lighter" (or perhaps peel to a non-existent sub-path of the subtree, but that feels hacky and wrong).

I.e.

// Current approach (also minor note: can't reuse `tree` buffer for `to_tree`
let to_tree = tree
    .lookup_entry_by_path(path)
    .unwrap() // Result
    .unwrap() // Option
    .object()
    .unwrap()
    .try_into_tree()
    .unwrap();

// Ideal
tree // optional: `peel_to_tree_by_path` moves `tree` and returns result
    .peel_to_tree_by_path(path)
    .unwrap() // Result
    .unwrap(); // Option