Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support equivalent of GIT_SORT_REVERSE | GIT_SORT_TOPOLOGICAL in rev walk #807

Closed
vlad-ivanov-name opened this issue Apr 8, 2023 · 5 comments · Fixed by #1610
Closed
Labels
enhancement New feature or request

Comments

@vlad-ivanov-name
Copy link

vlad-ivanov-name commented Apr 8, 2023

Summary 💡

There's gix_traverse::commit::Sorting::Topological, but I haven't found a way to reverse the traversal

Motivation 🔦

Used during filtering in https://github.com/josh-project/josh/

@vlad-ivanov-name vlad-ivanov-name added the enhancement New feature or request label Apr 8, 2023
@Byron
Copy link
Member

Byron commented Apr 9, 2023

After having looked at the source code of libgit2 it became clear that they aren't doing anything smart to perform this impossible feat. After all, commits are linked to their ancestors, but not to their descendants which means they do the equivalent of id.ancestors().all().collect::<Vec<_>>().into_iter().rev().

As Rust has combinators for this I believe these should be used instead to not hide potentially considerable latency and memory consumption behind a seemingly innocent flag. Thanks for your understanding.

@Byron Byron closed this as not planned Won't fix, can't repro, duplicate, stale Apr 9, 2023
@vlad-ivanov-name
Copy link
Author

Good point - thank you 🙏

@nrdxp
Copy link
Contributor

nrdxp commented Sep 25, 2024

would it be possible to implement this more performantly by implementing DoubleEndedIterator for a Walk?

My use-case is that I want to quickly calculate the origin commit (the oldest commit with no parents) as quickly as possible. If the iterator over a Walk sorted by commit time were efficiently reversible, this would essential be O(1) instead of O(n) as it is now to call last()

@nrdxp
Copy link
Contributor

nrdxp commented Sep 26, 2024

I believe with #1610 this would now be possible, without hurting performance, although its not for Topological searches, it is extremely fast when looking for ancient commits compared to the NewestFirst variants.

@Byron
Copy link
Member

Byron commented Sep 26, 2024

Thanks for making this happen! I wasn't aware that simply traversing by oldest first can mean such a huge saving, and is definitely something that should be made available.
I may also have been wrong about my initial assessment which lead to closing the issue, so reopening.

@Byron Byron reopened this Sep 26, 2024
@Byron Byron linked a pull request Sep 26, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants