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

>
> Hi Chandra, Phillip,
>
> > > >
> > > > 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.
>
> Collapsing would be when branches move to the left, eg:
>
>   *
>   |\     <- merge
>   | *
>   |/     <- 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)
> >
> > It probably *does* look clearer here, but I have the same reservations
> > against this as Junio: the break won't be as noticeable when --graph is
> > *not* used with --oneline.
> >
> > > >>> 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
> >
> > Right, this would be ideal but that would require too much change to the
> > existing graphing logic, and should be its own patch.
>
> For the examples I'll use the term parentless instead of root, as
> boundary commits are excluded even if they are roots.
> By having is_parentless as a flag in 'git_graph' that every stage can
> access we could modify the rendering and maybe completely drop the
> commit placeholders, working on it for v4 but currently renders like
> this
>
>     * A parentless
>       * B parentless
>         * C parentless
>   * D1 child
>   * D parentless
>
> (A has indentation when it could not have, but that would require a
> lookahead if the next commit is also parentless)
> But definitely a step forward.
>
> Do we want cascading or just a fixed indentation?
>
>     * A parentless
>     * B parentless
>     * C parentless
>   * D1 child
>   * D parentless
>
> By being indented it indicates that it is parentless and that the one
> below doesn't relate to it, but cascading looks clearer.

> >
> > > > 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.
> >
> > I guess that would make the examples look something like this:
> >
> >   * A root
> >   * B root
> >   * C root
> > * D1 child
> > * D root
> >
> > No cascading, and no need for that massive _ / collapse line.
> >
> > * 10_A2
> > * 10_A1
> >  \
> >   * 10_A
> > *   10_M
> > | \ \
> > | | * 10_D
> > | * 10_C
> > * 10_B
> >
> > I say it looks better than the alternatives, but I'm not sure if this will
> > be easy to implement. The diagonal connection line (\) will need to
> > be printed before printing the actual root commit, which will require
> > lookahead logic.
> >
> > I'd prefer to avoid major surgery on the codebase.
>
> Octopus merges need a pre-commit phase where an additional row
> increases the space around a commit with multiple parents to make room
> for it.
> A new phase can be created similarly to pre-commit as pre-root where
> the connection edge (\) can be printed before the indented commit.
>
> So far this is the comparison:
>
> indentation at root:
>
>     * A parentless
>   * B1 child
>    \
>     * B parentless
>   * C1 child
>   * C parentless
>
> indentation AFTER the root (current v3):
>
>   * A parentless
>     * B1 child
>    /
>   * B parentless
>     * C1 child
>    /
>   * C parentless
>
> Karthik mentioned that by indenting the parentless, we lose the
> consistency of having the roots on their real column and now some are
> indented and some are not.
> The biggest winner having the parentless indented are the merge commits:
>
>   * A child
>   * A child
>     \
>       * A parentless
>   *-.   B child
>   | \ \
>   | |  * C parentless
>   | * D parentless
>   * E parentless
>
> which IMO looks clearer than the commit shower:
>
>   * A child
>   * A child
>   * A parentless
>     *   B child
>    /|\
>   | | * C parentless
>   | * D parentless
>   * E parentless

> >
> >
> > Thanks,
> > Chandra.
>
> Regards
>
> --
> Pablo