Misrendering of git 2.24 log --graph

Misrendering of git 2.24 log --graph

[Jan Engelhardt]

Greetings.

I have observed git 2.24 outputting a garbage graph element for a 
particular history. The issue does not appear in 2.25, but the 
underlying bug may still be in there; it is just that _this particular 
history_ does not expose it anymore due to the new, more compact tree 
rendering that 2.25 seems to be shipping.

Reproducer:

$ git clone git://github.com/jengelh/git-issue-20200123 gi
$ cd gi; git log --oneline --graph --all --topo-order | head -n 74
[...]
| | * |   ba85ad93c Merge branch 'kc-8.7.x'
| | |\ \  
| |/ / /  
| | | _   
| | * 79106b731 doc: update 8.7.x news

See that underscore there in line 73. The connection between commit 
610d621dd and its parent 79106b731 is not properly connected (visually). 
I think this should have been [diff notation follows]

-| | | _   
+| | | /
+| | |/
 | | * 79106b731 doc: update 8.7.x news

or something along those lines (pun intended).

Re: Misrendering of git 2.24 log --graph

[Derrick Stolee]

On 1/23/2020 6:12 PM, Jan Engelhardt wrote:
> Greetings.

Hello, Jan. Thanks for sending this report.

> I have observed git 2.24 outputting a garbage graph element for a 
> particular history. The issue does not appear in 2.25, but the 
> underlying bug may still be in there; it is just that _this particular 
> history_ does not expose it anymore due to the new, more compact tree 
> rendering that 2.25 seems to be shipping.

I initially thought you were right, as 2.25 did include some new
rendering, and we've already found and fixed a regression [1] and
a style issue [2].

[1] https://lore.kernel.org/git/pull.517.git.1578408947.gitgitgadget@gmail.com/
[2] https://lore.kernel.org/git/pull.518.git.1578457675.gitgitgadget@gmail.com/

> Reproducer:
> 
> $ git clone git://github.com/jengelh/git-issue-20200123 gi

I needed to use "--mirror" in my clone to create local copies
of all the remote refs.

> $ cd gi; git log --oneline --graph --all --topo-order | head -n 74
> [...]
> | | * |   ba85ad93c Merge branch 'kc-8.7.x'
> | | |\ \  
> | |/ / /  

Interesting, since the compact graph output in 2.25 intentionally
wanted this merge to have the first edge go immediately to the left
(see my output below).

> | | | _   
> | | * 79106b731 doc: update 8.7.x news
> 
> See that underscore there in line 73. The connection between commit 
> 610d621dd and its parent 79106b731 is not properly connected (visually). 
> I think this should have been [diff notation follows]
> 
> -| | | _   
> +| | | /
> +| | |/
>  | | * 79106b731 doc: update 8.7.x news
> 
> or something along those lines (pun intended).

You are correct about the expected output here.

Here is something I noticed: this does not reproduce without a
commit-graph! That's likely because it changes the order of the
initial refs. Keep that in mind for anyone trying to repro this.

However, I see this output with 2.24.1:

| | | | * d95d49694 freebusy: add missing "else" in HrGetHumanReadableString
| | | |/  
| | |/|   
| | * |   ba85ad93c Merge branch 'kc-8.7.x'
| | |\ \  
| |/ / /  
| | | _   
| | * 79106b731 doc: update 8.7.x news

and this output in 2.25.0:

| | | | * d95d49694 freebusy: add missing "else" in HrGetHumanReadableString
| | | |/  
| | |/|   
| | * | ba85ad93c Merge branch 'kc-8.7.x'
| |/| | 
| | |/  
| | * 79106b731 doc: update 8.7.x news

And comparing v2.25.0 versus ds/graph-horizontal-edges has
no difference in output.

Is it possible that you are not running the version you think you are?

Thanks,
-Stolee

Re: Misrendering of git 2.24 log --graph

[Derrick Stolee]

On 1/24/2020 9:05 AM, Derrick Stolee wrote:
> On 1/23/2020 6:12 PM, Jan Engelhardt wrote:
>> Greetings.
> 
> Hello, Jan. Thanks for sending this report.
> 
>> I have observed git 2.24 outputting a garbage graph element for a 
>> particular history. The issue does not appear in 2.25, but the 
>> underlying bug may still be in there; it is just that _this particular 
>> history_ does not expose it anymore due to the new, more compact tree 
>> rendering that 2.25 seems to be shipping.

I completely misread your ordering here. You are clear that this
is an issue in 2.24 and NOT 2.25. Sorry.

I'm not sure that there is anything to do since the graph rendering
has changed so much, and we intend to keep the new version instead.

Thanks,
-Stolee

Re: Misrendering of git 2.24 log --graph

[Jan Engelhardt]

On Friday 2020-01-24 15:05, Derrick Stolee wrote:
>> I have observed git 2.24 outputting a garbage graph element for a 
>> particular history.
>
>I initially thought you were right, as 2.25 did include some new
>rendering, and we've already found and fixed a regression [1] and
>a style issue [2].
>[...]
>Here is something I noticed: this does not reproduce without a
>commit-graph! That's likely because it changes the order of the
>initial refs. Keep that in mind for anyone trying to repro this.

Curiously, I need no --mirror for `git clone`, the default clone
operation is just fine. It has, as far as I can remember back,
always downloaded all origin:refs/heads/* and refs/tags/*.

>However, I see this output with 2.24.1:

>| | | _   
>| | * 79106b731 doc: update 8.7.x news
>
>and this output in 2.25.0 [...]
>
>Is it possible that you are not running the version you think you are?

I rerun my checks as I write this mail. The observations:

 * Built v2.23.0 from source, ran it via `../tmpdir/git/git log --andsoon`,
   shows said underscore.
 * Built v2.24.0 from source, shows underscore
 * Built v2.24.1 from source, shows underscore
 * Used v2.24.1 from distro (/usr/bin/git), renders underscore
 * Built ead10bacc466a409490fb743d7805893976f8f53 (most recent 
   branch join not having v2.25 as an ancestor) from src, 
   shows underscore
 * Built v2.25.0 from source, new rendering
 * Built 631a7dec11, same as v2.25.0
 * Built 631a7dec11^, same as v2.25.0

Re: Misrendering of git 2.25 log --graph

[Jan Engelhardt]

On Friday 2020-01-24 15:06, Derrick Stolee wrote:
>
>I completely misread your ordering here. You are clear that this
>is an issue in 2.24 and NOT 2.25. Sorry.
>
>I'm not sure that there is anything to do since the graph rendering
>has changed so much, and we intend to keep the new version instead.

I now have a minimized reproducer, for *2.25*:

» ../git/git log --oneline --graph --all --decorate --topo-order
*   e1ee7b8 (HEAD -> master) Merge branch 'k9' into k10
|\  
| *   7b48214 Merge branch 'k8' into k9
| |\  
| | *   ed02a51 Merge branch 'blah' into k8
| | |\  
| | | * 44279cf blah
| | * |   4053c4d Merge branch 'k7' into k8
| | |\ \  
| |/ / /  
| | | _   
| | * 5b449d6 update 8.7 news
| |/  
| * 43a324f foo
|/  
* 5932a51 root

» ../git/git --version
git version 2.25.0.24.gbc7a3d4dc0

The repository https://github.com/jengelh/git-issue-20200123 has been
re-populated with the new contents.

Re: Misrendering of git 2.25 log --graph

[Derrick Stolee]

On 1/24/2020 11:34 AM, Jan Engelhardt wrote:
> On Friday 2020-01-24 15:06, Derrick Stolee wrote:
>>
>> I completely misread your ordering here. You are clear that this
>> is an issue in 2.24 and NOT 2.25. Sorry.
>>
>> I'm not sure that there is anything to do since the graph rendering
>> has changed so much, and we intend to keep the new version instead.
> 
> I now have a minimized reproducer, for *2.25*:
> 
> » ../git/git log --oneline --graph --all --decorate --topo-order
> *   e1ee7b8 (HEAD -> master) Merge branch 'k9' into k10
> |\  
> | *   7b48214 Merge branch 'k8' into k9
> | |\  
> | | *   ed02a51 Merge branch 'blah' into k8
> | | |\  
> | | | * 44279cf blah
> | | * |   4053c4d Merge branch 'k7' into k8
> | | |\ \  
> | |/ / /  
> | | | _   
> | | * 5b449d6 update 8.7 news
> | |/  
> | * 43a324f foo
> |/  
> * 5932a51 root

Hm. I get the above output for v2.24.1. Since you are using a
specific path for your execution, then perhaps that version
disagrees with the one on your PATH that reported "git version"?

For v2.25.0 I get this:

*   e1ee7b8 (HEAD -> master, origin/master, origin/HEAD) Merge branch 'k9' into k10
|\  
| *   7b48214 Merge branch 'k8' into k9
| |\  
| | *   ed02a51 Merge branch 'blah' into k8
| | |\  
| | | * 44279cf blah
| | * | 4053c4d Merge branch 'k7' into k8
| |/| | 
| | |/  
| | * 5b449d6 update 8.7 news
| |/  
| * 43a324f foo
|/  
* 5932a51 root

Again, the key is at 4053c4d where the first edge out
of the merge commit moves to the left.

Thanks,
-Stolee

Re: Misrendering of git 2.25 log --graph

[SZEDER Gábor]

On Fri, Jan 24, 2020 at 01:45:34PM -0500, Derrick Stolee wrote:
> On 1/24/2020 11:34 AM, Jan Engelhardt wrote:
> > On Friday 2020-01-24 15:06, Derrick Stolee wrote:
> >>
> >> I completely misread your ordering here. You are clear that this
> >> is an issue in 2.24 and NOT 2.25. Sorry.
> >>
> >> I'm not sure that there is anything to do since the graph rendering
> >> has changed so much, and we intend to keep the new version instead.
> > 
> > I now have a minimized reproducer, for *2.25*:
> > 
> > » ../git/git log --oneline --graph --all --decorate --topo-order
> > *   e1ee7b8 (HEAD -> master) Merge branch 'k9' into k10
> > |\  
> > | *   7b48214 Merge branch 'k8' into k9
> > | |\  
> > | | *   ed02a51 Merge branch 'blah' into k8
> > | | |\  
> > | | | * 44279cf blah
> > | | * |   4053c4d Merge branch 'k7' into k8
> > | | |\ \  
> > | |/ / /  
> > | | | _   
> > | | * 5b449d6 update 8.7 news
> > | |/  
> > | * 43a324f foo
> > |/  
> > * 5932a51 root
> 
> Hm. I get the above output for v2.24.1. Since you are using a
> specific path for your execution, then perhaps that version
> disagrees with the one on your PATH that reported "git version"?
> 
> For v2.25.0 I get this:
> 
> *   e1ee7b8 (HEAD -> master, origin/master, origin/HEAD) Merge branch 'k9' into k10
> |\  
> | *   7b48214 Merge branch 'k8' into k9
> | |\  
> | | *   ed02a51 Merge branch 'blah' into k8
> | | |\  
> | | | * 44279cf blah
> | | * | 4053c4d Merge branch 'k7' into k8
> | |/| | 
> | | |/  
> | | * 5b449d6 update 8.7 news
> | |/  
> | * 43a324f foo
> |/  
> * 5932a51 root

I get this good-looking graph with v2.25.0, too.

A few notes to add:

  - We don't need the topmost commit e1ee7b8 to reproduce that strange
    underscore below the kink with v2.24.0:

      $ git --no-pager log --oneline --graph --topo-order 7b48214
      *   7b48214 Merge branch 'k8' into k9
      |\  
      | *   ed02a51 Merge branch 'blah' into k8
      | |\  
      | | * 44279cf blah
      | * |   4053c4d Merge branch 'k7' into k8
      | |\ \  
      |/ / /  
      | | _   
      | * 5b449d6 update 8.7 news
      |/  
      * 43a324f foo
      * 5932a51 root

  - Git behaved like that up until about the middle of the patch
    series simplifying the graph output, namely until commit
    458152cce1 (graph: example of graph output that can be simplified,
    2019-10-15).  The next commit 0f0f389f12 (graph: tidy up display
    of left-skewed merges, 2019-10-15) changed the bahavior for the
    worse:

      *   7b48214 Merge branch 'k8' into k9
      |\  
      | *   ed02a51 Merge branch 'blah' into k8
      | |\  
      | | * 44279cf blah
      | * | 4053c4d Merge branch 'k7' into k8
      |/|\  
      | |/  
      | * 5b449d6 update 8.7 news
      |/  
      * 43a324f foo
      * 5932a51 root

    Notice how 4053c4d looks like an octopus merge, and 44279cf
    seemingly comes out of nowhere.

    And then the next commit d62893ecc1 (graph: commit and post-merge
    lines for left-skewed merges, 2019-10-15) seems to have fixed this
    issue, and we see the same good-looking graph that we get with
    v2.25.0 as well:

      *   7b48214 Merge branch 'k8' into k9
      |\  
      | *   ed02a51 Merge branch 'blah' into k8
      | |\  
      | | * 44279cf blah
      | * | 4053c4d Merge branch 'k7' into k8
      |/| | 
      | |/  
      | * 5b449d6 update 8.7 news
      |/  
      * 43a324f foo
      * 5932a51 root

  - Interestingly, the issue of an underscore below the kink is
    mentioned in the later commit 92beecc136 (graph: flatten edges
    that fuse with their right neighbor, 2019-10-15), quoting the last
    part of its log message:

      One of the test cases here cannot be correctly rendered in Git v2.23.0;
      it produces this output following commit E:
      
              | | *-. \   5_E
              | | |\ \ \
              | |/ / / /
              | | | / _
              | |_|/
              |/| |
      
      The new implementation makes sure that the rightmost edge in this
      history is not left dangling as above.

    This is part of the new test case 'log --graph with octopus merge
    with column joining its penultimate parent' in t4215 that was
    added in 92beecc136.  Usually when a commit both changes the code
    and add new tests, then that means that that new test would have
    failed on previous versions.  However, that is not the case here:
    I tried this new test case on earlier commits in that series, and
    it already succeed with commit d62893ecc1, i.e. the same commit
    that fixes Jan's cases.