rbong
commented
5 months ago I tried this style of commit character while I was writing v2, but for me, it's way too hard to visually identify the horizontal position of commits on branches quickly in large graphs, and I find coloring these characters white doesn't help with legibility much and is quite ugly personally.
(The choice to use the circle character for commits also lead to the decision to put a space between disconnected commits - I just wanted to make you aware of this, but you can choose to include spaces or not with these changes.)
Because this is a change to the graph drawing function and I don't have a lot of interest in this myself, I have to ask for a couple things before I approve this PR:
1) An opt-in option to turn this on, with the current style preserved if the setting is off.
2) Tests.
Since the graph drawing function is optimized and isn't really designed for easy maintenance, it can be hard to make changes to just one spot without impacting other things. Tests are important for all new graph drawing functionality to make sure everything works and continues to work when future changes are made.
I would like to see these test cases:
- A new test case in
t/t_graph_tangle.shthat compares output with a copy oft/data/graph_tangle_outwith the setting on. This will test your changes in combination with all different types of merges, which can catch tricky bugs. - A more simple test with all the new commit orientations: "top", "middle", "bottom", and "disconnected" commits. This will test your changes in isolation, which will help me narrow down exactly where things are going wrong if tests fail. Please make one test case with the setting on and one with it off.
(Actually, your changes as they exist right now will break tests I believe. Not sure why pipelines aren't catching that.)
3) A performance comparison after you finish your changes.
Measure performance with: time vim <file> -c 'Flog -all -max-count=10000' -c 'qa'
Please use a reasonably large repository with complex branches (I think I use either the Linux or Git repo typically) and give your recorded time before/after the changes with the setting off (you can test with it on if you'd like, but my primary concern is the default behaviour). I have no preference if you use VIM or nvim, as long as you use LuaJIT.
The graph drawing function is hyper-optimized - if any changes are made to the function it needs to remain hyper-optimized, so performance needs to be checked.
Sorry, I know tests and performance comparisons are annoying, but this is the price for an optimized custom graph drawing function. I should warn you as well... after you make these changes, I can't guarantee I won't have more feedback, although I hope I don't.
TamaMcGlinn
I like to use ┌ ├ └ for the commits, because it gives me lines to follow visually. With flog v1 this works:
Plug 'rbong/vim-flog', { 'branch': 'v1' } Plug 'TamaMcGlinn/flog-forest', { 'branch': 'v1' }
But with v2 the ability to customize the forest used in the background was lost, so I have re-added this by checking if the commit has child commits already drawn, and whether it has children that will later be drawn, and still falling back to the • if neither (disconnected commits).
The change to the MergeStart line just makes it a little more strict, as it must be followed by a non-space character such as ──╮. Otherwise, it thinks each middle-commit line is actually a mergestart line and those commit lines become all-white.
Here's a video of me switching between this commit and its parent. I have checked and this works without any merge conflicts on current rbong/master. I have also been using this since the commit date one year ago, across a wide variety of repos, including very large ones, with no issues whatsoever.
flog_top_bottom_middle.webm