Re: [GSoC RFC PATCH 0/1] graph: add indentation for commits preceded by a root

From: Phillip Wood <phillip.wood123@gmail.com>
Date: May 15, 2026, 09:33 UTC
Message-ID: <2e8b9b1b-6a69-4e94-95ea-7f587435bfce@gmail.com>
In-Reply-To: <CAN5EUNQCsKD0CJqDi43i2JVBQQChAZVt_THQ1wGpdeydNHHCFw@mail.gmail.com>

On 14/05/2026 18:45, Pablo Sabater wrote:

Show 49 quoted lines

> El jue, 14 may 2026 a las 17:15, Phillip Wood
> (<phillip.wood123@gmail.com>) escribió:
>> On 02/04/2026 22:17, Pablo Sabater wrote:
>>> When having a history with multiple root commits and drawing the history
>>> near the roots, the graphing engine renders the commit one below the other,
>>> seeming that they are related, which makes the graph confusing.
>>>
>>> This issue was reported by Junio at:
>>>     https://lore.kernel.org/git/xmqqikaawrpx.fsf@gitster.g/
>>>
>>> e.g.:
>>>
>>>     * root-B
>>>     * child-A2
>>>     * child-A1
>>>     * root-A
>>>
>>> [...]
>>   >
>>>     * root-B
>>>       * child-A2
>>>      /
>>>     * child-A1
>>>     * root-A
>>
>> I'm rather late to the party here, but personally I find the indentation
>> a bit confusing, it would be clearer to me if we had a blank line after
>> a root commit
> 
> Hi,
> 
>>
>>       * root-B
>>
>>       * child-A2
>>       * child-A1
>>       * root-A
>>
>> It takes the same amount of vertical space but keeps the children of
>> root-A together.
> 
> I have mixed feelings about which approach to choose.
> The idea of a blank line was thought at
> https://lore.kernel.org/git/xmqq8s8vvw9m.fsf@gitster.c.googlers.com/
> but Junio argued against it for having an extra row because the
> indentation he proposed didn't collapse, however I find indentation +
> no collapse the most confusing one.
> I'd say that I'm fine with both approaches, blank line or indentation
> + collapse.

I'm afraid I don't understand this - what does it mean for the indentation to collapse, or not collapse. Looking at the examples Junio gave they look quite nice to me, though I'd find it clearer if

  | | *  12345678 2021-01-14 merge xxxxx@xxxx into the history
  | | |\
  | | | \
  | | *  \  23456789 2021-01-12 merge citest into the main history
  | | |\  * 5505e019c2 2014-07-09 initial xxxxxx@xxxx
  | | | *  3e658f4085 2019-09-10 (wiki/wip-citest, origin/wip-citest) 
Added defau
  | | | *  ad148aafe6 2019-09-10 Added default CI/CD Jenkinsfile (from 
f7daf088)

was rendered as

  | | *  12345678 2021-01-14 merge xxxxx@xxxx into the history
  | | |\
  | | | *  5505e019c2 2014-07-09 initial xxxxxx@xxxx
  | | *    23456789 2021-01-12 merge citest into the main history
  | | |\
  | | | *  3e658f4085 2019-09-10 (wiki/wip-citest, origin/wip-citest) 
Added defau
  | | | *  ad148aafe6 2019-09-10 Added default CI/CD Jenkinsfile (from 
f7daf088)

Show 32 quoted lines

>>> without the patch:
>>>
>>>     * A root
>>>     * B root
>>>     * C root
>>>     * D1 child
>>>     * D root
>>>
>>> with the patch, the indentation cascades:
>>>
>>>     * A root
>>>       * B root
>>>         * C root
>>>           * D1 child
>>>        _ /
>>>       /
>>>      /
>>>     * D root
> 
>    * A root
> 
>    * B root
> 
>    * C root
> 
>    * D1 child
> 
>    * D root
> 
> Here I think a blank line looks worse, too much space for just 5
> commits and becomes one extra line which if this were like up to 7 or
> more parentless commits one after the other would be more noticeable.

But there shouldn't be a blank line between D and D1 so the two alternatives take up the same amount of vertical space, the main difference being whether D1 appears next to D

     * A root     * A root
                    * B root
     * B root         * C root
                        * D1 child
     * C root         _/
                    /
     * D1 child    /
     * D root     * D root

Of course if the indentation was smarter it would take up less room and look better than having blank lines

     * A root
       * B root
         * C root
     * D1 child
     * D root

Show 12 quoted lines

> But there are cases that blank line might be better:
> 
>    * 10_A2
>    * 10_A1
>    * 10_A
>      *   10_M
>     /|\
>    | | * 10_D
>    | * 10_C
>    * 10_B
> 
> Feels like a shower of commits instead of an indented merge.

Yes, that is a bit confusing. I think the thing I find confusing with this approach is that we're treating the commit rendered below the root commit specially, rather than treating the root commit itself specially. To me it is the root commit that's the odd one out because it does not have any parents, but we treat the commit that's rendered below as the odd one by indenting it relative to its parents.

Show 6 quoted lines

> Pro to the blank line, the parentless check is the same and it's just
> printing a '\n' at the right spot, while indent i'm mimicking like if
> there was a commit there.
> Anyways, I think in the majority of the cases the indentation +
> collapsing looks better.
> Sorry for the brief reply, I'm busy today.

No need to apologize, it seemed quite comprehensive to me

Thanks

Phillip

Show 69 quoted lines

> Regards,
> 
> --
> Pablo
> 
>>
>> Thanks
>>
>> Phillip
>>
>>> This is done by adding a is_placeholder flag to the columns, the root commit
>>> is actually there but marked as a placeholder
>>>
>>> e.g.:
>>>
>>>      * root-B
>>>     (B) * child-A2
>>>       /
>>>      * child-A1
>>>      * root-A
>>>
>>> (B) would be root-B column with the placeholder flag active.
>>>
>>> Then teaching the rendering function to print a padding ' ' when meeting a
>>> placeholder column outputs the second example.
>>>
>>> There could also be the case where there are multiple roots
>>>
>>> without the patch:
>>>
>>>     * A root
>>>     * B root
>>>     * C root
>>>     * D1 child
>>>     * D root
>>>
>>> with the patch, the indentation cascades:
>>>
>>>     * A root
>>>       * B root
>>>         * C root
>>>           * D1 child
>>>        _ /
>>>       /
>>>      /
>>>     * D root
>>>
>>> the _ / might look weird but that's how the collapsing rendering does it
>>> for big gaps, this case being from the 4th column to the 0th column.
>>> Another patch could change the collapsing rendering for placeholders ?
>>> I haven't done it to keep it minimal, but a follow up could make it
>>> to be straight '/'. This would make it bigger but easier for the eye to follow.
>>> IMO is not worth it, but opinions are welcome.
>>>
>>> The patch also adds tests for different cases like a root preceding multiple
>>> parents merges and the examples above.
>>>
>>> There could be some edge cases still so any testing is very welcome.
>>>
>>> Pablo Sabater (1):
>>>     graph: add indentation for commits preceded by a root
>>>
>>>    graph.c                      |  68 ++++++++++++++++--
>>>    t/t4215-log-skewed-merges.sh | 136 +++++++++++++++++++++++++++++++++++
>>>    2 files changed, 198 insertions(+), 6 deletions(-)
>>>
>>>
>>> base-commit: 256554692df0685b45e60778b08802b720880c50
>>