Re: log --graph: extra space with --pretty=oneline

[Adam Simpkins <adam@adamsimpkins.net>]

On Wed, May 28, 2008 at 02:24:05PM +0300, Teemu Likonen wrote:
> Sometimes "log --graph --pretty=oneline" prints a sort of broken graph
> line. In the git repository try this:
> 
> $ git log --graph --pretty=oneline --abbrev-commit -4 8366b7b
> 
> [current graph output removed]
> 
> Extra spaces there. I don't mind that myself but to some users it may
> look like a bug. Maybe one would expect output like this:
> 
> 
> M   8366b7b... Merge branch 'maint'
> |\  
> | M   a2f5be5... Merge branch 'jk/maint-send-email-compose' into maint
> | |\  
> | | \
> | M  \  93c7b9c... Merge branch 'hb/maint-send-email-quote-recipients' into maint
> | |\  \ 
> | | \  \
> | M  \  |  6abf189... Merge branch 'maint-1.5.4' into maint
> | |\  | |
> 
> It requires more lines though.


Hmm.  Yes, the output could definitely be improved.  This problem
occurs for merges with 2 parents (not octopus merges), and only when
the branch line to the right of the merge was moving right at the end
of the previous commit (i.e., it was displayed with '\' instead of '|'
or '/').

Note that this doesn't require extra lines to fix:

M   8366b7b... Merge branch 'maint'
|\  
| M   a2f5be5... Merge branch 'jk/maint-send-email-compose' into maint
| |\  
| M \  93c7b9c... Merge branch 'hb/maint-send-email-quote-recipients' into maint
| |\ \ 
| M \ \  6abf189... Merge branch 'maint-1.5.4' into maint
| |\ \ \

This can easily be implemented by changing the output for 2-way merges
from something like this:

| M  \
| |\  |

to this:

| M \
| |\ \

However, I find this makes the graph slightly uglier when the incoming
branch to the right of the merge wasn't '\' on the previous line.  The
following change seems to look better when the branch line was '|' or
'/' on the previous line:

| M |
| |\ \

For comparison, here's a comparison of several scenarios of how the
output looks now, and how it would look with these fixes.

Current behavior        Option 1                 Option 2

| |\                    | |\                     | |\
| M  \                  | M \                    | M |
| |\  |                 | |\ \                   | |\ \

| | |                   | | |                    | | |
| M  \                  | M \                    | M |
| |\  |                 | |\ \                   | |\ \

| |  /                  | |  /                   | |  /
| M  \                  | M \                    | M |
| |\  |                 | |\ \                   | |\ \

| | |/                  | | |/                   | | |/
| M  \                  | M \                    | M |
| |\  |                 | |\ \                   | |\ \

Note that in the case of octopus merges, the current code already
produces output like that of option 1.

However, I find that both options 1 and 2 look a little uglier than
the current behavior when the branch lines need to be collapsed again
after the merge.  The graph has more pointy angles, but it is still
readable in all cases:

Current behavior        Option 1                 Option 2

| * | |                 | * | |                  | * | |
| M  \ \                | M \ \                  | M | |
| |\  | |               | |\ \ \                 | |\ \ \
|/ / / /                |/ / / /                 |/ / / /

For sections of the graph where there are several merge commits in a
row, I think option 1 looks the best:

Current behavior        Option 1                 Option 2

* |                     * |                      * |
M  \                    M \                      M |
|\  |                   |\ \                     |\ \
M  \ \                  M \ \                    M | |
|\  | |                 |\ \ \                   |\ \ \
M  \ \ \                M \ \ \                  M | | |
|\  | | |               |\ \ \ \                 |\ \ \ \


What is everyone's preference between the 3 options?  Personally, I'm
leaning towards Option 2.

I'll send out some informal patches of both option 1 and option 2, for
comparison.

In the future, the code could even be improved to dynamically choose
between these three options based on the output printed for the
previous commit.  Currently it doesn't store enough information from
the previous commit to do this.

-- 
Adam Simpkins
adam@adamsimpkins.net