Branching#2890
Conversation
|
@extrawurst Would love your feedback on this :) Quite large. OH! And I just noticed that I removed the missing docs lint, I'll add that back if we agree on this general direction. |
|
LGTM |
| * fix extremely slow status loading in large repositories by replacing time-based cache invalidation with generation counter [[@DannyStoll1](https://github.com/DannyStoll1)] ([#2823](https://github.com/gitui-org/gitui/issues/2823)) | ||
| * fix panic when renaming or updating remote URL with no remotes configured [[@xvchris](https://github.com/xvchris)] ([#2868](https://github.com/gitui-org/gitui/issues/2868)) | ||
| ### Added | ||
| * Now with a graph view! |
There was a problem hiding this comment.
please match the ususal format
|
I think we need to improve the visualization still, this is the same point in time of the history in three different tools: gitui, tig and fork: 
|
|
Being precise here, the only difference I can see in the lack of a bottom connector? Or is there something else?
▰▰▰▰▰
Miles Wirth 🙃
… From: extrawurst ***@***.***>
Sent: 29 March 2026 11:53
To: gitui-org/gitui ***@***.***>
Cc: Miles Wirht ***@***.***>, Author ***@***.***>
Subject: Re: [gitui-org/gitui] Branching (PR #2890)
extrawurst left a comment (gitui-org/gitui#2890)
I think we need to improve the visualization still, this is the same point in time of the history in three different tools: gitui, tig and fork:
<img width="623" height="291" alt="Screenshot 2026-03-29 at 19 50 36" src="https://github.com/user-attachments/assets/a05fed09-ce18-4f55-8915-e143b37b01d3" />
<img width="392" height="326" alt="Screenshot 2026-03-29 at 19 50 27" src="https://github.com/user-attachments/assets/8669dbeb-604b-4d25-ba8c-de5aa701ac95" />
<img width="895" height="227" alt="Screenshot 2026-03-29 at 19 51 51" src="https://github.com/user-attachments/assets/5cf8535b-3ce6-44a2-944d-a33c81c548ce" />
--
Reply to this email directly or view it on GitHub:
#2890?email_source=notifications&email_token=A3MQBNB5QFY6DKIO5JS2ZSD4TFWKZA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIMJVGA4DKNBWGE32M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2LK4DSL5RW63LNMVXHIX3POBSW4X3DNRUWG2Y#issuecomment-4150854617
You are receiving this because you authored the thread.
Message ID: ***@***.***>
|
|
I am looking forward to this change! By a quick look the source it seems like it works only with merge commits with 2 parents, but commit can have an arbitrary number of parents (see e.g. here). |
|
@VojtaStanek Yes! Octopus merges! That's not covered here. I couldn't find an implementation that handles them in a way that maps well to streaming. They don't crash anything, and happen incredibly rarely. If someone is able to extend what I have here, they're welcome to; but it shouldn't block merging. |
|
Thanks a lot for getting this started, that’s much appreciated! I just started reviewing the changes, but then I had a few more general questions. In its current form, I find it hard to see merge commits and how they connect to their respective parents. The image below shows a comparison between this branch and Also, in 
|
|
No, that's just how they look :sad:.
Good points -- I also wonder if people like the pressed texts to the facets of the branch lines? I could implement that as well.
I think symbols... look cooler; but there are readability improvements we could aim for regardless, that these screenshots are making clear to me. The glyphs could expose be exposed as configuration.
▰▰▰▰▰
Miles Wirth 🙃
… From: Christoph Rüßler ***@***.***>
Sent: 07 April 2026 12:37
To: gitui-org/gitui ***@***.***>
Cc: Miles Wirht ***@***.***>, Author ***@***.***>
Subject: Re: [gitui-org/gitui] Branching (PR #2890)
cruessler left a comment (gitui-org/gitui#2890)
Thanks a lot for getting this started, that’s much appreciated! I just started reviewing the changes, but then I had a few more general questions.
In its current form, I find it hard to see merge commits and how they connect to their respective parents. The image below shows a comparison between this branch and `tig` in the `gitoxide` repository that has quite a few merge commits. Is there anything I need to set up to make merge commits more prominent? Does my terminal font need to have certain glyphs?
Also, in `tig`, vertical lines don’t get cut by horizontal ones, as opposed to `guitar`. I think I like `tig`’s approach better. What do you think?
<img width="3440" height="1412" alt="Screenshot_20260407_212543" src="https://github.com/user-attachments/assets/c224b4a6-6a01-4942-be2f-fef91d63d40a" />
--
Reply to this email directly or view it on GitHub:
#2890 (comment)
You are receiving this because you authored the thread.
Message ID: ***@***.***>
|
Not for now.
Also not relevant for now. Lets find a visual style thats a good default and add customization in a later PR. THis one is big enough as is.
I agree with this. we either find better symbols or go for the letter Furthermore I think we should use different colored lines for parallel branches like tig does for more clear separation. |
With the updates, the PR is now looking like the above. Is this a better direction? @extrawurst I plan to submit a follow-up PR with selection-based highlighting which I think will clear up the remaining vie |
|
Hi all, if you need a reference for what looks good and works well in TUI then lazygit (go) and git-igitt (rust) are both very good at visualizing the graph. I think the table layout of lazygit makes more sense: short hash | timestamp | name | graph | (tags, branches) message it can also switch between --all mode and current branch https://github.com/jesseduffield/lazygit#commit-graph 
|
|
When I looked at the algo's of git-gitt there was some issues around their design as a second-pass over a complete input, which is counter to the streaming design of gitui. Definitely still some issues I realize I miss when I review, I think there's still some issues with connecting pipes, but the tradeoffs of the algorithm feel solid to me. |
|
@philocalyst thanks for your reply. I don't have any insight on the code of the above mentioned projects, my comment is purely from a user perspective. I'm a software professional and my main way of interacting with git is lazygit, before that I used the integration in vs code with the very popular git graph extension and before that I used source tree. I also briefly tried gitkraken and more recently gitui. I use the CLI when I need to or when it's simpler. While lazygit has some serious bugs and I disagree with some of their decisions which make me miss git graph from vs code, it is my favourite tool so far, and a big part of that is their graph view, and even if the first version in gitui isn't equivalent, I would like to suggest it as inspiration for what it could become, in addition to the also fantastic git graph extension in vs code and the visually appealing git-igitt. |
|
I'm with you !!
Yeah I do plan to do follow-up PRs, just adding in more of these complex previews might have to be done on a second pass on just the active window.
▰▰▰▰▰
Miles Wirth 🙃
… From: balintbarna ***@***.***>
Sent: 22 April 2026 15:05
To: gitui-org/gitui ***@***.***>
Cc: Miles Wirht ***@***.***>, Mention ***@***.***>
Subject: Re: [gitui-org/gitui] Branching (PR #2890)
balintbarna left a comment (gitui-org/gitui#2890)
@philocalyst thanks for your reply. I don't have any insight on the code of the above mentioned projects, my comment is purely from a user perspective.
I'm a software professional and my main way of interacting with git is lazygit, before that I used the integration in vs code with the very popular git graph extension and before that I used source tree. I also briefly tried gitkraken and more recently gitui. I use the CLI when I need to or when it's simpler.
While lazygit has some serious bugs and I disagree with some of their decisions which make me miss git graph from vs code, it is my favourite tool so far, and a big part of that is their graph view, and even if the first version in gitui isn't equivalent, I would like to suggest it as inspiration for what it could become.
--
Reply to this email directly or view it on GitHub:
#2890?email_source=notifications&email_token=A3MQBNCKMUOFIA5ZN45NX3L4XE64LA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIMZQGAZDKMRUHEYKM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJNLQOJPWG33NNVSW45C7N5YGK3S7MNWGSY3L#issuecomment-4300252490
You are receiving this because you were mentioned.
Message ID: ***@***.***>
|
|
@philocalyst Thanks for the changes! I’ve now started the proper first review which should be ready by today or tomorrow if all goes according to plan! 😀 |
|
Thanks so much :)
▰▰▰▰▰
Miles Wirth 🙃
… From: Christoph Rüßler ***@***.***>
Sent: 26 April 2026 23:24
To: gitui-org/gitui ***@***.***>
Cc: Miles Wirht ***@***.***>, Mention ***@***.***>
Subject: Re: [gitui-org/gitui] Branching (PR #2890)
cruessler left a comment (gitui-org/gitui#2890)
@philocalyst Thanks for the changes! I’ve now started the proper first review which should be ready by today or tomorrow if all goes according to plan! 😀
--
Reply to this email directly or view it on GitHub:
#2890 (comment)
You are receiving this because you were mentioned.
Message ID: ***@***.***>
|
|
Hi there. I'm still working on getting git-graph, now gleisbau, into gitui. The speed requirement from gitui can be solved by splitting the global branch trace algorithm off to a background thread, and then start at head. This will feel as if it starts instantly, but it will traverse the entire repo in due time. The split also addresses the memory issue seen in the gitui+gitgraph prototype from about a year go. This is a major refactoring, so it will take some time. git-igitt is part of the git-bahn family and uses git-graph/gleisbau under the hood. I think it is a good idea to get a graph feature into gitui as soon as possible. Everybody wants it. Maybe gleisbau can produce a visual improvement, but the major step is simply getting a graph. Congratulations to @philocalyst for reaching that goal. |
cruessler
left a comment
There was a problem hiding this comment.
I’m sorry it’s taking so long! In order to not make you wait longer, I’m now sending you my first batch of comments, hoping they are already useful. As soon as I’m done with the next batch, you’ll get the next round of comments. I hope that works for you!
|
Balls back in your court! @cruessler |
Really sorry it’s taking so long, I should be able to devote some time to this PR this week as I don’t have too many other obligations. |
|
@cruessler Appreciated! Have another PR I've been sitting on for LFS, but didn't want to overwhelm the team -- so I'll be monitoring my github notifications closely to make sure this week isn't wasted!! |
cruessler
left a comment
There was a problem hiding this comment.
I’m slowly getting myself familiar with the PR, in order to make sure I actually understand myself what is going on. 😀 I’ve got a couple of questions and remarks, some more fundamental than others. I’m also going to push minor refactorings that don’t warrant a comment to your branch as well, in order to make some areas more idiomatic. I hope that’s ok, if not, please let me know!
|
okay @cruessler you inspired me to do a pretty big refactor so sorry for the delay here! put a ton of effort into reducing the amount of redundant work as far as both clones and passing through the same commits/lanes twice or more. should be totally lazy as far as the graph goes, which i'm very happy to report! there's also a set of tests that should test the basics... (disclosure that i used $llm for the test writing, let me know if this goes against a policy and i will rewrite!) Also I kind of overpromised with stashes, although I will do in a follow-up if all goes well :) |
overly pessimistic collapsing rmeoved
i keep forgetting asyncigt is different
|
OH! Also I managed to make the graph rendering the nicest it's ever looked!!! Excited for your thoughts :) |
|
Also started working on some more improvements, like mega-merges and path highlighting, but I would rather submit that after we get this one in! |
Makes sense, this PR is already quite large. 😀 I hope to get to do some more reviewing over the weekend or at the start of next week! |
e936663 to
d58c135
Compare
| impl Default for Buffer { | ||
| fn default() -> Self { | ||
| Self::new() | ||
| } | ||
| } |
There was a problem hiding this comment.
Could this be replaced by #[derive(Default)] on Buffer?
| } | ||
|
|
||
| pub fn update(&mut self, new_chunk: &Chunk) { | ||
| // Phase 1: place the new chunk into the lane array. |
There was a problem hiding this comment.
The comments in update feel a bit redundant as the method names already carry some context.
| &self, | ||
| alias: Option<AliasId>, | ||
| ) -> Option<usize> { | ||
| let alias = alias?; |
There was a problem hiding this comment.
What do you think about turning this into alias.map(…)?
| } | ||
|
|
||
| fn place_chunk(&mut self, new_chunk: &Chunk) -> usize { | ||
| // Prefer a lane whose current occupant is waiting for this chunk as parent_a. |
There was a problem hiding this comment.
What does parent_a refer to?
| impl Default for GraphOids { | ||
| fn default() -> Self { | ||
| Self::new() | ||
| } | ||
| } |
There was a problem hiding this comment.
Could this be turned into #[derive(Default)] on GraphOids?
| /// The alias is a dense integer index created by [`GraphOids`](super::oids::GraphOids) | ||
| /// that avoids storing full [`CommitId`](crate::sync::CommitId)s inside the lane state. | ||
| #[derive(Clone, Copy, Debug, Default, PartialEq, Eq, Hash)] | ||
| pub struct AliasId(usize); |
There was a problem hiding this comment.
The name AliasId was not immediately obvious to me when I first saw it. Is there maybe a different name that carries more context? Maybe AliasForCommitId or is that too clunky?
| } | ||
|
|
||
| #[derive(Clone, Debug, PartialEq, Eq)] | ||
| pub struct Chunk { |
There was a problem hiding this comment.
Is there a way to make the name Chunk more descriptive? Even after reviewing this PR for a bit, it doesn’t really click. 😀
|
|
||
| #[derive(Clone, Debug, PartialEq, Eq)] | ||
| pub struct Chunk { | ||
| pub alias: Option<AliasId>, |
There was a problem hiding this comment.
Can you add some context on what alias being None means?
| /// | ||
| pub id: CommitId, | ||
| /// | ||
| pub parents: SmallVec<[CommitId; 2]>, |
There was a problem hiding this comment.
I’m wondering whether there is an easy way to also compute this information on demand. This could potentially also be explored in a follow-up.
| alias: AliasId, | ||
| skip_index: usize, | ||
| ) { | ||
| for index in 0..self.current.len() { |
There was a problem hiding this comment.
Is there a way to turn this loop into something like for (index, chunk) in self.current.iter().enumerate()? I’d like to avoid the extra bounds check inside the loop if possible.
| if let Some(second) = second_parent { | ||
| self.merge_parents.insert(commit_alias, second); | ||
|
|
||
| if first_parent.is_some() |
There was a problem hiding this comment.
Are there cases where second_parent.is_some(), but first_parent.is_none()? Apart from that, do you think something like match (first_parent, second_parent) could make the possible cases clearer?
There was a problem hiding this comment.
All my comments were supposed to part of a single review, but apparently I was not familiar enough yet with the tool I’m using to make that happen, so all of them arrived as individual comments. Sorry for causing that many notifications!
Also, would it be possible to add a couple more tests, so that all symbols in graphrow.rs are covered?
After having gone through maybe a third of half of the PR, I can say that I like the changes so far, but I was wondering whether there was a way to calculate parents on demand as well?
|
I’m not sure whether there’s something wrong with the algorithm. In my copy of https://github.com/GitoxideLabs/gitoxide, scrolling to the beginning of the history shows a few lanes that I think should be empty, but have lines in them. (This looks different in 
|
This Pull Request fixes/closes #81.
It changes the following:
I followed the checklist:
make checkwithout errors