Git development
 help / color / mirror / Atom feed
From: Mirko Faina <mroik@delayed.space>
To: Pablo Sabater <pabloosabaterr@gmail.com>
Cc: git@vger.kernel.org, ayu.chandekar@gmail.com,
	 chandrapratap3519@gmail.com, christian.couder@gmail.com,
	gitster@pobox.com,  jltobler@gmail.com, karthik.188@gmail.com,
	krka@spotify.com, peff@peff.net,  phillip.wood@dunelm.org.uk,
	siddharthasthana31@gmail.com
Subject: Re: [PATCH v8 4/4] graph: indent visual root in graph
Date: Fri, 10 Jul 2026 20:07:07 +0200	[thread overview]
Message-ID: <alEroo_DhFaWm3DH@exploit> (raw)
In-Reply-To: <20260710-ps-pre-commit-indent-v8-4-d3b636463bf4@gmail.com>

On Fri, Jul 10, 2026 at 12:37:07PM +0200, Pablo Sabater wrote:
> When rendering a graph, if the history contains multiple "visual roots",
> actual roots or commits that look like roots (i.e. have their parents
> filtered out) can end up being vertically adjacent to unrelated commits,
> falsely appearing to be related.
> 
> A fix for this issue was already attempted [1] a while ago.
> 
> This happens because the commits fill the space from left to right and
> when a visual root ends, its column becomes free for the following
> commit even if they are not related. Once this happens the unrelated
> commit is rendered below the visual root. Because there is no special
> character or way to identify when a visual root is rendered making the
> graph confusing.
> 
> By indenting the visual roots when there are still commits to show the
> vertical adjacency can be avoided.
> 
> Add is_visual_root flag to git_graph making it visible in all graph states,
> give graph_update() a new function, graph_is_visual_root() to know if the
> current commit is a visual root and set is_visual_root.
> The different handled cases are:
> 
> - If a visual root has children: similar to GRAPH_PRE_COMMIT state when
>   octopus merges need space, an edge row needs to be printed to connect
>   the child with the indented visual root. A new state GRAPH_PRE_ROOT is
>   needed to connect the child with the visual root:
> 
>     * child of the visual root
>      \ GRAPH_PRE_ROOT
>       * visual root indented
> 
> - If a visual root is child-less we can skip GRAPH_PRE_ROOT state and
>   render the indented commit directly.
> 
>       * visual root indented
>     * unrelated commit
> 
> - If two or more visual roots are adjacent: by having a lookahead to the
>   next commit that will be rendered, if the next commit is also a visual
>   root and we are on a visual root, meaning two visual root adjacent in
>   the history, the top one can omit the indent, making the one below to
>   indent only once, if there are more adjacent visual commits, the
>   indentation will increase for each adjacent one, cascading.
> 
>     * visual root
>       * visual root
>         * visual root
>     * last commit
> 
>   Even if the last commit is a root, because there is nothing that will be
>   rendered below we can omit the indentation on purpose.
> 
> [1]: https://lore.kernel.org/git/xmqqwnwajbuj.fsf@gitster.c.googlers.com/
> 
> Helped-by: Kristofer Karlsson <krka@spotify.com>
> Mentored-by: Karthik Nayak <karthik.188@gmail.com>
> Mentored-by: Chandra Pratap <chandrapratap3519@gmail.com>
> Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
> ---
>  graph.c                          | 235 +++++++++++++++++++
>  t/meson.build                    |   1 +
>  t/t4218-log-graph-indentation.sh | 473 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 709 insertions(+)

This doesn't seem to work for every visual root e.g.

    git log --graph --oneline --author="Mirko Faina"

The visual roots are not indented.

> +/*
> + * A commit can be a visual root when:
> + *
> + * - It has no parents.
> + *
> + * - It has parents but they are all filtered out and
> + *   commit->parents arrives NULL.
> + *
> + * - It is not a boundary commit. Boundary commits also have no visible
> + *   parents, but they are not selected as visual roots because they cannot
> + *   cause the ambiguity of being vertically adjacent because:
> + *
> + *   1. A boundary only appears because an included commit is its child.
> + *      Children are always above, and the renderer draws an edge down to
> + *      the boundary from that child. Rather than starting a column like a
> + *      visual root would do, it inherits its child column.
> + *
> + *   2. Included commits cannot appear below a boundary. Boundaries are
> + *      ancestors of the exclusion point; if an included commit were an
> + *      ancestor of the boundary it would be excluded and not rendered.
> + *      Boundaries therefore always sink to the bottom.
> + */
> +static int graph_is_visual_root_candidate(struct commit *c)
> +{
> +	return c->parents == NULL && !(c->object.flags & BOUNDARY);
> +}

I suspect this behaviour is due to these assumptions being too strict.

When we use the --author option the parents are not filtered out, so it
doesn't return NULL desipte being a visual root. We realize it is a
visual root only on the next commit, but once we are on the next commit
we can't indent as we have already printed this commit.

We realize only on the next commit after hitting simplify_commit(), it
calls get_commit_action() and checks if should keep the commit based on
the regex we provided. If the regex is not matched the commit is just
ignored (we do not filter parents based on regex when we expand a topo
walk).

At least that's what I gather, if anyone can confirm this...

  reply	other threads:[~2026-07-10 18:14 UTC|newest]

Thread overview: 118+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-04-02 21:17 [GSoC RFC PATCH 0/1] graph: add indentation for commits preceded by a root Pablo Sabater
2026-04-02 21:17 ` [GSoC RFC PATCH 1/1] " Pablo Sabater
2026-04-03 17:55   ` Junio C Hamano
2026-04-03 18:07     ` Pablo
2026-04-03  5:04 ` [GSoC RFC PATCH 0/1] " Junio C Hamano
2026-04-03  8:25   ` Pablo
2026-04-04  9:24 ` [GSoC RFC PATCH v2 0/1] graph: add indentation for commits preceded by a parentless commit Pablo Sabater
2026-04-04  9:24   ` [GSoC RFC PATCH v2 1/1] " Pablo Sabater
2026-04-10 16:25   ` [GSoC RFC PATCH v2 0/1] " Pablo
2026-04-10 16:54     ` Junio C Hamano
2026-04-27 10:28   ` [GSoC PATCH v3 " Pablo Sabater
2026-04-27 10:28     ` [GSoC PATCH v3 1/1] " Pablo Sabater
2026-05-13 23:02       ` Jeff King
2026-05-14 10:19         ` Pablo Sabater
2026-04-27 10:35     ` [GSoC PATCH v3 0/1] " Pablo
2026-06-12 13:48     ` [PATCH v4 0/2] graph: indent visual roots in graph Pablo Sabater
2026-06-12 13:48       ` [PATCH v4 1/2] lib-log-graph: move check_graph function Pablo Sabater
2026-06-12 13:48       ` [PATCH v4 2/2] graph: indent visual root in graph Pablo Sabater
2026-06-13  3:01       ` [PATCH v4 0/2] graph: indent visual roots " Junio C Hamano
2026-06-13 19:09       ` [PATCH v5 " Pablo Sabater
2026-06-13 19:09         ` [PATCH v5 1/2] lib-log-graph: move check_graph function Pablo Sabater
2026-06-13 19:09         ` [PATCH v5 2/2] graph: indent visual root in graph Pablo Sabater
2026-06-14  4:05           ` Junio C Hamano
2026-06-14  5:28             ` Pablo Sabater
2026-06-15 15:42               ` Junio C Hamano
2026-06-16 13:06                 ` Pablo Sabater
2026-06-16 16:36                   ` Junio C Hamano
2026-06-17 20:27           ` Jeff King
2026-06-18 12:42             ` Pablo Sabater
2026-06-18 13:31               ` Junio C Hamano
2026-06-18 16:05               ` Jeff King
2026-06-18 16:07                 ` Jeff King
2026-06-19  7:34                 ` Kristofer Karlsson
2026-06-21 18:05                   ` Jeff King
2026-06-22  8:29                     ` Kristofer Karlsson
2026-06-17 10:33         ` [PATCH v5 0/2] graph: indent visual roots " Chandra Pratap
2026-06-20 10:11         ` [PATCH v6 0/3] " Pablo Sabater
2026-06-20 10:11           ` [PATCH v6 1/3] lib-log-graph: move check_graph function Pablo Sabater
2026-06-20 10:11           ` [PATCH v6 2/3] revision: add peek functions for lookahead Pablo Sabater
2026-06-20 10:18             ` Pablo Sabater
2026-06-20 14:56             ` Junio C Hamano
2026-06-20 15:15               ` Junio C Hamano
2026-06-21  1:48             ` Junio C Hamano
2026-06-21  6:42             ` Chandra Pratap
2026-06-22  8:28             ` Kristofer Karlsson
2026-06-29 21:56               ` Junio C Hamano
2026-06-29 23:29                 ` Pablo Sabater
2026-06-20 10:11           ` [PATCH v6 3/3] graph: indent visual root in graph Pablo Sabater
2026-07-04  8:52           ` [PATCH v7 0/3] graph: indent visual roots " Pablo Sabater
2026-07-04  8:52             ` [PATCH v7 1/3] lib-log-graph: move check_graph function Pablo Sabater
2026-07-04  8:52             ` [PATCH v7 2/3] graph: add a 2 commit buffer for lookahead Pablo Sabater
2026-07-06  9:49               ` Chandra Pratap
2026-07-06 13:44                 ` Kristofer Karlsson
2026-07-06 15:33                   ` Chandra Pratap
2026-07-07  6:31                     ` Pablo Sabater
2026-07-07 18:12                       ` Pablo Sabater
2026-07-04  8:52             ` [PATCH v7 3/3] graph: indent visual root in graph Pablo Sabater
2026-07-10 10:37             ` [PATCH v8 0/4] graph: indent visual roots " Pablo Sabater
2026-07-10 10:37               ` [PATCH v8 1/4] lib-log-graph: move check_graph function Pablo Sabater
2026-07-10 10:37               ` [PATCH v8 2/4] revision: add next_commit_to_show() Pablo Sabater
2026-07-10 10:37               ` [PATCH v8 3/4] graph: add a 2 commit buffer for lookahead Pablo Sabater
2026-07-10 10:37               ` [PATCH v8 4/4] graph: indent visual root in graph Pablo Sabater
2026-07-10 18:07                 ` Mirko Faina [this message]
2026-07-10 20:29                   ` Pablo Sabater
2026-07-11 13:37               ` [PATCH v9 0/4] graph: indent visual roots " Pablo Sabater
2026-07-11 13:37                 ` [PATCH v9 1/4] lib-log-graph: move check_graph function Pablo Sabater
2026-07-11 13:37                 ` [PATCH v9 2/4] revision: add next_commit_to_show() Pablo Sabater
2026-07-11 13:37                 ` [PATCH v9 3/4] graph: add a 2 commit buffer for lookahead Pablo Sabater
2026-07-11 13:37                 ` [PATCH v9 4/4] graph: indent visual root in graph Pablo Sabater
2026-07-11 14:15                 ` [PATCH v9 0/4] graph: indent visual roots " Mirko Faina
2026-07-11 15:41                   ` Pablo Sabater
2026-07-11 16:25                     ` Mirko Faina
2026-07-12  5:56                       ` Chandra Pratap
2026-07-12 13:10                         ` Mirko Faina
2026-07-12 13:12                           ` Mirko Faina
2026-07-12 16:59                           ` Pablo Sabater
2026-07-12 19:33                             ` Mirko Faina
2026-07-13  7:41                             ` Chandra Pratap
2026-07-13 10:44                 ` [PATCH v10 0/7] " Pablo Sabater
2026-07-13 10:44                   ` [PATCH v10 1/7] lib-log-graph: move check_graph function Pablo Sabater
2026-07-13 10:44                   ` [PATCH v10 2/7] revision: add next_commit_to_show() Pablo Sabater
2026-07-13 10:44                   ` [PATCH v10 3/7] graph: add a 2 commit buffer for lookahead Pablo Sabater
2026-07-13 10:44                   ` [PATCH v10 4/7] graph: indent visual root in graph Pablo Sabater
2026-07-13 10:44                   ` [PATCH v10 5/7] graph: wrap cascading commits after 4 columns Pablo Sabater
2026-07-13 10:44                   ` [PATCH v10 6/7] graph: move config reading into graph_read_config() Pablo Sabater
2026-07-13 10:44                   ` [PATCH v10 7/7] graph: add --[no-]graph-indent and log.graphIndent Pablo Sabater
2026-07-13 14:06                     ` Mirko Faina
2026-07-13 16:16                       ` Pablo Sabater
2026-07-13 16:43                   ` [PATCH v11 0/7] graph: indent visual roots in graph Pablo Sabater
2026-07-13 16:43                     ` [PATCH v11 1/7] lib-log-graph: move check_graph function Pablo Sabater
2026-07-13 16:43                     ` [PATCH v11 2/7] revision: add next_commit_to_show() Pablo Sabater
2026-07-13 16:44                     ` [PATCH v11 3/7] graph: add a 2 commit buffer for lookahead Pablo Sabater
2026-07-13 16:44                     ` [PATCH v11 4/7] graph: indent visual root in graph Pablo Sabater
2026-07-13 16:44                     ` [PATCH v11 5/7] graph: wrap cascading commits after 4 columns Pablo Sabater
2026-07-13 16:44                     ` [PATCH v11 6/7] graph: move config reading into graph_read_config() Pablo Sabater
2026-07-13 16:44                     ` [PATCH v11 7/7] graph: add --[no-]graph-indent and log.graphIndent Pablo Sabater
2026-07-14 10:19                       ` Chandra Pratap
2026-07-14 11:47                         ` Pablo Sabater
2026-07-13 20:28                     ` [PATCH v11 0/7] graph: indent visual roots in graph Junio C Hamano
2026-07-13 20:51                       ` Pablo Sabater
2026-07-14 12:09                     ` [PATCH v12 " Pablo Sabater
2026-07-14 12:09                       ` [PATCH v12 1/7] lib-log-graph: move check_graph function Pablo Sabater
2026-07-14 12:09                       ` [PATCH v12 2/7] revision: add next_commit_to_show() Pablo Sabater
2026-07-14 12:09                       ` [PATCH v12 3/7] graph: add a 2 commit buffer for lookahead Pablo Sabater
2026-07-14 12:09                       ` [PATCH v12 4/7] graph: indent visual root in graph Pablo Sabater
2026-07-14 12:09                       ` [PATCH v12 5/7] graph: wrap cascading commits after 4 columns Pablo Sabater
2026-07-14 12:09                       ` [PATCH v12 6/7] graph: move config reading into graph_read_config() Pablo Sabater
2026-07-14 12:09                       ` [PATCH v12 7/7] graph: add --[no-]graph-indent and log.graphIndent Pablo Sabater
2026-05-14 15:15 ` [GSoC RFC PATCH 0/1] graph: add indentation for commits preceded by a root Phillip Wood
2026-05-14 17:45   ` Pablo Sabater
2026-05-15  9:33     ` Phillip Wood
2026-05-17  6:31       ` Chandra Pratap
2026-05-18 13:26         ` Pablo Sabater
2026-05-19  0:03           ` Junio C Hamano
2026-05-19  5:59             ` Pablo Sabater
2026-06-10 15:21               ` Junio C Hamano
2026-06-10 15:28                 ` Pablo Sabater
2026-05-19 10:39           ` Chandra Pratap

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=alEroo_DhFaWm3DH@exploit \
    --to=mroik@delayed.space \
    --cc=ayu.chandekar@gmail.com \
    --cc=chandrapratap3519@gmail.com \
    --cc=christian.couder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jltobler@gmail.com \
    --cc=karthik.188@gmail.com \
    --cc=krka@spotify.com \
    --cc=pabloosabaterr@gmail.com \
    --cc=peff@peff.net \
    --cc=phillip.wood@dunelm.org.uk \
    --cc=siddharthasthana31@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox