[00/11] Improve the readability of log --graph output
mbox series

Message ID pull.383.git.gitgitgadget@gmail.com
Headers show
Series
  • Improve the readability of log --graph output
Related show

Message

ryenus via GitGitGadget Oct. 10, 2019, 4:13 p.m. UTC
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

Comments

Derrick Stolee Oct. 10, 2019, 5:54 p.m. UTC | #1
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
Jeff King Oct. 13, 2019, 7:15 a.m. UTC | #2
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
James Coglan Oct. 14, 2019, 3:49 p.m. UTC | #3
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.