| Message ID | pull.383.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Improve the readability of log --graph output | expand |
On 10/10/2019 12:13 PM, James Coglan via GitGitGadget wrote: > This series of patches are designed to improve the output of the log --graph > command; their effect can be summed up in the following diagram: > > Before After > ------ ----- > > * > |\ > | * * > | |\ |\ > | | * | * > | | | | |\ > | | \ | | * > | *-. \ | * | > | |\ \ \ |/|\| > |/ / / / | | * > | | | / | * | > | | |/ | |/ > | | * * / > | * | |/ > | |/ * > * | > |/ > * I took a brief look through your series, and I think this is a really cool improvement. I use "git log --graph" all the time and those kinks have bothered me, too. I'd give you extra bonus points if your first patch added the case on the left as an expected output and we watch it get simpler as you modify the behavior in pieces. I do really like how you isolated different transformations into different commits. I would have given more specific feedback if I was more familiar with graph.c. I'll need to play with it more to give more substantive feedback. The only thing I could complain about in the first glance is some test file formatting stuff, like how you split a test case into "setup" "create expected output" and "test the output". That would be better as one test. Also you add a space between your redirect character and your file ("> expect" instead of ">expect"). Those nits aside, I look forward to digging into the code more soon. Thanks, -Stolee
On Thu, Oct 10, 2019 at 09:13:42AM -0700, James Coglan via GitGitGadget wrote: > A final addition to that set of changes fixes the coloring of dashes that > are drawn next to octopus merges, in a manner compatible with all these > changes. The early commits in this set are refactorings that make the > functional changes easier to introduce. As somebody who has pondered the octopus coloring code (for an embarrassingly long time considering that it still has some bugs!), let me just say thank you for taking this on. :) Moreover, I'll echo Dscho's comments elsewhere on the quality of this series. It's a tricky topic to explain, and the way you've broken it up, along with the commit messages, comments, and diagrams made it much easier to follow. Others have already commented on things I saw while reading it, so I'll just add a few more thoughts. > This series of patches are designed to improve the output of the log --graph > command; their effect can be summed up in the following diagram: > > Before After > ------ ----- > > * > |\ > | * * > | |\ |\ > | | * | * > | | | | |\ > | | \ | | * > | *-. \ | * | > | |\ \ \ |/|\| > |/ / / / | | * > | | | / | * | > | | |/ | |/ > | | * * / > | * | |/ > | |/ * > * | > |/ > * I wondered if anybody would prefer the sparseness of the "before" diagram, and if that would merit having two modes that could selected at runtime. I'm not sure I'd want to carry the code for both types, though; it seems like a recipe for the non-default output format to accrue a bunch of bugs (since the graph code has proven itself to be a magnet for off-by-ones and other weirdness). Diffing the output of "git diff --color --graph --oneline" on git.git both before and after your patch, the changes all look generally positive to me. The graph above is pretty dense, but that's not really what real-world graphs look like; it was designed to show off the changes. That plus the fact that you're fixing real bugs (like the octopus coloring) makes me inclined to just move to your suggested output. -Peff
On 13/10/2019 08:15, Jeff King wrote: > On Thu, Oct 10, 2019 at 09:13:42AM -0700, James Coglan via GitGitGadget wrote: > >> A final addition to that set of changes fixes the coloring of dashes that >> are drawn next to octopus merges, in a manner compatible with all these >> changes. The early commits in this set are refactorings that make the >> functional changes easier to introduce. > > As somebody who has pondered the octopus coloring code (for an > embarrassingly long time considering that it still has some bugs!), let > me just say thank you for taking this on. :) > > Moreover, I'll echo Dscho's comments elsewhere on the quality of this > series. It's a tricky topic to explain, and the way you've broken it up, > along with the commit messages, comments, and diagrams made it much > easier to follow. > > Others have already commented on things I saw while reading it, so I'll > just add a few more thoughts. > >> This series of patches are designed to improve the output of the log --graph >> command; their effect can be summed up in the following diagram: >> >> Before After >> ------ ----- >> >> * >> |\ >> | * * >> | |\ |\ >> | | * | * >> | | | | |\ >> | | \ | | * >> | *-. \ | * | >> | |\ \ \ |/|\| >> |/ / / / | | * >> | | | / | * | >> | | |/ | |/ >> | | * * / >> | * | |/ >> | |/ * >> * | >> |/ >> * > > I wondered if anybody would prefer the sparseness of the "before" > diagram, and if that would merit having two modes that could selected at > runtime. I'm not sure I'd want to carry the code for both types, though; > it seems like a recipe for the non-default output format to accrue a > bunch of bugs (since the graph code has proven itself to be a magnet for > off-by-ones and other weirdness). You're probably right about the non-default mode hiding bugs, but if you did want to do this, I believe the original rendering can be preserved by the following small switches: - always set `merge_layout = 1`; this will prevent skewing to the left - do not modify `edges_added` if the last parent fuses with its neighbor I believe all the layout changes are driven by these values, so you should be able to set in such a way as to preserve the original rendering by ignoring the special cases that lead to them having different values.
This series of patches are designed to improve the output of the log --graph command; their effect can be summed up in the following diagram: Before After ------ ----- * |\ | * * | |\ |\ | | * | * | | | | |\ | | \ | | * | *-. \ | * | | |\ \ \ |/|\| |/ / / / | | * | | | / | * | | | |/ | |/ | | * * / | * | |/ | |/ * * | |/ * These changes aim to make the edges in graph diagrams easier to read, by straightening lines and making certain kinds of topologies display more compactly. Three distinct changes are included. First, if the first parent of a merge fuses with an edge to the left of the commit, then we display that by making the edges fuse immediately rather than by drawing a line straight down and then having it track to the left. That is, where we currently display these graphs: | * | | | * | |\ | | | |\ |/ / | |_|/ / | | |/| | | We will now display these merges as follows: | * | | | * |/| | |_|/| | | |/| | | This transformation is applied to merges with any number of parents, for example we currently display 3-parent merges like this: | *-. | | | *-. | |\ \ | | | |\ \ |/ / / | |_|/ / / | | | |/| | | | And we will now display them like this: | * | | | * |/|\ | |_|/|\ | | | |/| | | | If the edge the first parent fuses with is separated from the commit by multiple columns, a horizontal edge is drawn just as we currently do in the 'collapsing' state. This change also affects the display of commit and post-merge lines in subtle ways that are more thoroughly described in the relevant commits. The second change is that if the final parent of a merge fuses with the edge to the right of the commit, then we can remove the zig-zag effect that currently results. We currently display these merges like this: * | |\ \ | |/ | * After these changes, this merge will now be displayed like so: * | |\| | * If the final parent fuses with an edge that's further to the right, its display is unchanged and it will still display like this: * | | | |\ \ \ \ | | |_|/ | |/| | | * | | The final structural change smooths out lines that are collapsing through commit lines. For example, consider the following history: *-. \ |\ \ \ | | * | | * | | | |/ / * | | |/ / * | |/ * This is now rendered so that commit lines display an edge using / instead of |, if that edge is tracking to the left both above and below the commit line. That results in this improved display: *-. \ |\ \ \ | | * | | * | | | |/ / * / / |/ / * / |/ * Taken together, these changes produce the change shown in the first diagram above, with the current rendering on the left and the new rendering on the right. A final addition to that set of changes fixes the coloring of dashes that are drawn next to octopus merges, in a manner compatible with all these changes. The early commits in this set are refactorings that make the functional changes easier to introduce. James Coglan (11): graph: automatically track visible width of `strbuf` graph: reuse `find_new_column_by_commit()` graph: reduce duplication in `graph_insert_into_new_columns()` graph: remove `mapping_idx` and `graph_update_width()` graph: extract logic for moving to GRAPH_PRE_COMMIT state graph: tidy up display of left-skewed merges graph: commit and post-merge lines for left-skewed merges graph: rename `new_mapping` to `old_mapping` graph: smooth appearance of collapsing edges on commit lines graph: flatten edges that join to their right neighbor graph: fix coloring of octopus dashes graph.c | 499 ++++++++++++--------- strbuf.h | 8 +- t/t3430-rebase-merges.sh | 2 +- t/t4202-log.sh | 2 +- t/t4214-log-graph-octopus.sh | 85 +++- t/t4215-log-skewed-merges.sh | 267 +++++++++++ t/t6016-rev-list-graph-simplify-history.sh | 30 +- 7 files changed, 649 insertions(+), 244 deletions(-) create mode 100755 t/t4215-log-skewed-merges.sh base-commit: 5fa0f5238b0cd46cfe7f6fa76c3f526ea98148d9 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-383%2Fjcoglan%2Fjc%2Fsimplify-graph-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-383/jcoglan/jc/simplify-graph-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/383