Re: [PATCH v5 2/2] graph: indent visual root in graph

From: Pablo Sabater <pabloosabaterr@gmail.com>
Date: Jun 16, 2026, 13:06 UTC
Message-ID: <CAN5EUNR-o_sLzeWuy7M9UMFHBKxSuytNd=4p2svtFuv40E8vZg@mail.gmail.com>
In-Reply-To: <xmqqzf0vbyj8.fsf@gitster.g>

El lun, 15 jun 2026 a las 17:42, Junio C Hamano (<gitster@pobox.com>) escribió:

Show 14 quoted lines

>
> Pablo Sabater <pabloosabaterr@gmail.com> writes:
>
> > It does not make it unpredictable but it makes it not output what I
> > wanted to test, what I wanted to test is having an active column at
> > the same time that visual roots in different cases were being rendered
> > on another column.
>
> Oh, use of commit-graph changes the traversal order, which would
> affect how the graph is drawn, and there is no way to ensure that we
> traverse in the same way with or without commit-graph?  That's
> inconvenient.  But even without commit-graph, do we guarantee the
> same traversal order forever?  I doubt it.  So I suspect that it is
> a brittle workaround to disable commit-graph in the longer term.

Hi!

About the traversal order, aren't all the graph tests dependent on the traversal order? If it changed they would all need to be updated because the tests are hardcoded expects of the graph. I guess it might be more brittle than other graph tests specially because it also depends on removing files, I tried using "git config core.commitGraph false" or "--date-order" but I still get different results and removing the files fixed it. If someone knows a better way of doing it I'm happy to change it.

Show 27 quoted lines

>
> As long as the graph engine shows correct graph no matter what order
> the commits come out of the revision traversal engine, we won't hurt
> end-users, but we need our tests to be reproducible, so that is a
> bit unfortunate.
>
> Anyway, stepping back a bit,
>
> > However having GIT_TEST_COMMIT_GRAPH in the last
> > text for example changes from:
> >
> > * 41_octopus
> > | * 43_B
> > |  \
> > |   * 43_A
> > | * 42_B
> > | * 42_A
> > * 41_B
> > * 41_A
>
> Does the "vertically aligned * on 2nd and later columns do not mean
> any parent-child relationship" rule no longer apply in this version?
> IOW, does the above graph show that
>
>  - 41_A is a parent of 41_B, which is a parent of 41_octopus
>  - 42_A is a parent of 42_B, and
>  - 43_A is a parent of 43_B but is not related to 42_B

Yes, this means that all commits vertically adjacent are related, those who are not related and can cause that ambiguity get indented (43_A).

Show 5 quoted lines

>
> ?  Who are the parents of 41_octopus?  It has no relationship with
> 42_B and 43_B, and unlike what its name suggests, it has only 41_b
> as its parent (probably with history simplification that makes only
> these commits shown)?

On this test we are using "--first-parent" which excludes all the parents but the first one, but later we force its excluded parents to be shown. We exclude 42_* and 43_* branches and then force them to appear as unrelated branches.

Show 16 quoted lines

>
> > to:
> >
> > * 41_octopus
> > * 41_B
> >  \
> >   * 41_A
> > * 43_B
> >  \
> >   * 43_A
> > * 42_B
> > * 42_A
>
> And this graph shows the same inter-commit relationship.  So both
> are correctly showing what we want to express, but they show the
> same information differently, making test_cmp unhappy?

Yes they show the same information. On the second graph every commit is on the first column (or second if they get indented) but on the first graph we have:

*
| * <- visual root on second column
^
`----- first column remains active

If you tested v3 with this case you would see that it assumes that visual roots only happen to be rendered on the first column, therefore failing to correctly indent those visual roots on the second column, which this test proves that they can appear on other columns.

Back to the test:

  * 41_octopus
  | * 43_B
  |  \
  |   * 43_A
  | * 42_B
  | * 42_A
  * 41_B
  * 41_A

43_A is rendered on the second column (first column is active by the 41_* branch) and gets indented to the third one. With commit-graph it would be on the first and get indented to the second, making it the same as more general tests above in "t4218", it is an edge case but shows that indentation works correctly independently where the visual root is.

>
> Thanks.

Thanks,

Pablo