RE: [PATCH 1/2] revision: Denote root commits with '#'

["Jason Pyeron" <jpyeron@pdinc.us>]

Summary: --graph used with --oneline sometimes produces ambiguous output when more than one commit has no parents and are not yet merged

> From: Junio C Hamano
> Sent: Wednesday, January 20, 2021 4:52 PM
> 
> "Jason Pyeron" writes:
> 
> >> I actually do not see that as a problem.  In the past several years,
> >> I've never needed to see "log --graph" output that goes all the way
> >
> > I respect your needs, but they conflict with others' needs, while
> > this enhancement to resolve an ambiguity does not impede your
> > needs and solves others' needs.
> 
> I am questioning if such "needs" really exist in the first place.
> 
> Among 35k+ commits in the example project, if you had more than a
> few dozens of roots, then it may make sense to highlight them
> differently from ordinary commits whether they have parents in the
> shown part of the history.  It's like "log --decorate" shows branch
> tips marked specially.

That could work too.

> 
> Yes, I am saying that such a "this is root" marking, if it is
> valuable, should go on a part of "log --oneline" output that is
> shown even without "--graph", just like we annotate the commit with

I do not have any preferences beyond not "being lied to by git graph".

| * 22222
| * 11111
| * 33333
| * 44444

Implies that 11111 and 33333 have a parent / child relationship.

Quoting the man page, "--graph Draw a text-based graphical representation of the commit history on the left hand side of the output. This may cause extra lines to be printed in between commits, in order for the graph history to be drawn properly", would be preferable to add blank lines.

> "(branch name)" in the output, instead of painting the commit in the
> graph by replacing the '*' node with something else.
> 
> And how often do you really need to see commits near the root, say
> the earliest 100 commits, in the 35k+ commit history?  Is it really
> necessary to tell which among these 100 is the root?  

Yes, and the assumption that they are at the beginning is flawed too.

$ git log --oneline --graph --all | cat -n | egrep $(git rev-list --max-parents=0 --all | cut -c 1-8 | tr '\n' '|' | head -c -1)
    87  | | * be2c70b7 bug 2252 test case (e.g. for tomcat 9 with unpackWARs=false)
  2161  | | * 8ef73128 Add migrate-from-blackfat.sql
  2164  | | * 5505e019 initial
  2235  | | | | | | | | | | | | | * 83337c67 intial
  2921  | | | | * ca14dc49 Initial commit
  2931  | | | * cbdce824 initial commit
  2963  | | * 8f1828c1 Base applet
  2971  | * 658af21f parrent pom
  3026  * 8356af31 Initial commit from Create React App

git log --oneline --graph produces 3026 lines in this example.

> What problem does it solve?  

Avoiding confusion and non-compliance with the man page, which wastes human's time.

> Perhaps I am reacting to your solution without
> seeing the problem you are trying to solve?  First, I took the
> "replace <*> with {#}" as a solution for "parenthood becomes unclear
> in the --graph output" problem, and pointed out that the solution
> for that issue should apply to not just root commits but equally to
> the ones above the boundary.
> 

I have no objection to that either as it neither helps or hinders the solution to the real and initial issue.

> But it seems that I am hearing that it is not "graph showing false
> parenthood" problem that you were trying to solve, 

It is that graph is implying false parenthood. There was no intention for that (only) issue to morph.

> but "I want to
> see root differently for unspecified reason".

There is only one reason, the same reason that prompted the original email. Adjacent commits in the --graph formatted output were connected when they are actually not connected.

To quote earlier > From: Junio C Hamano
> Sent: Thursday, January 14, 2021 8:12 PM
> > | | | *  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)
> >
> ... is not so great in that it wastes a line, and the break
> won't be as noticeable when --graph is *not* used with --oneline.

No, because there would be no line connecting it.

| | | Date:   Tue Sep
| | |
| | |     Added defau
| | |
| | * commit 5505e019
| |   Author: xxxx
| |   Date:   Wed Jul
| |
| |       initial
| |
| | * commit 3e658f40
| | | Author: xxxx
| | | Date:   Tue Sep
| | |
| | |     Added defau
| | |
| | * commit ad148aaf
| | | Author: xxxx
| | | Date:   Tue Sep
| | |
| | |     Added defau
| | |

And to quote earlier > From: Junio C Hamano
> Sent: Thursday, January 14, 2021 8:12 PM
> > | | | #  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)
> This latter variant won't work.  Imagine we are showing --left-right
> for example.  Which side does '#' belong to?

There was no concerns or aversions about left/right. It was later clarified that being able to use the pagers search would be nice.

> 
> I am asking why, and if the reason is because there are nontrivial
> number of them sprinkled throughout the history, I am offering my
> opinion that something like how we show the commits at the tips of
> branches and tagged ones would be a better model than changing the
> letter used for the node in the graph.

Happy to take that solution too, but does it fix the bug in the graph when used with --oneline? And don’t misunderstand me, this is a bug in --graph with --oneline.

> 
> > Here are some messages:
> >
> > bug 2252 test case (e.g. for tomcat 9 with unpackWARs=false)
> > Add migrate-from-blackfat.sql
> > Initial commit from Create React App
> > parrent pom
> > initial commit
> > Base applet
> > intial
> > Initial commit
> > initial
> > import prod
> > import prod sql
> > import prod
> > import coop/dev
> > import prod CMIS.zip
> 
> You seem to have problems with not just root commits ;-)
> How many of these 5 "initial" commits are root?

100%, it was from:

git log --oneline $(git rev-list --max-parents=0 --all) | cut -c 10-

> 
> > I'll ask the following questions, besides the left right and test case issues:
> >
> > What quality issues exists with the patch (e.g. bugs, strategy, etc)?
> 
> By strategy I take that you mean design.  We've been talking about
> it, right?  Until that gets more or less settled, line-by-line bug
> hunting tends to become a waste of time, and I haven't had a chance
> to afford extra review bandwidth to dedicate to this topic.
> 
> Now the problem being solved seems to be changing, so I am not sure
> how close to be "done" the posted patch is to the real solution.
> Sorry.

There was no intention for change, but adjustments were made based on feedback. For example, quoting earlier > From: Junio C Hamano
> Sent: Thursday, January 14, 2021 8:12 PM
> It would be great to show it more like this:
> 
>  | | |   * 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)
> 

> From: Junio C Hamano
> Sent: Tuesday, January 19, 2021 5:10 PM
> 
> I do not mind if the graph rendering fix does not happen yet again;
> IIRC the past contributors couldn't implement it, either.

This was a good idea, but not readily feasible.

So to close the loop, I would love to support the creation and integration of a patch to ensure "graph history s/to be/is/ drawn properly" and not lying to the reader of the graph about the ancestry.

And thank you for spending time on this thread, I think we can find a feasible and usable solution.