git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: James Coglan via GitGitGadget <gitgitgadget@gmail.com>
Cc: James Coglan <jcoglan@gmail.com>,
	git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH 00/11] Improve the readability of log --graph output
Date: Sun, 13 Oct 2019 03:15:01 -0400	[thread overview]
Message-ID: <20191013071500.GA30652@sigill.intra.peff.net> (raw)
In-Reply-To: <pull.383.git.gitgitgadget@gmail.com>

On Thu, Oct 10, 2019 at 09:13:42AM -0700, James Coglan via GitGitGadget wrote:

> A final addition to that set of changes fixes the coloring of dashes that
> are drawn next to octopus merges, in a manner compatible with all these
> changes. The early commits in this set are refactorings that make the
> functional changes easier to introduce.

As somebody who has pondered the octopus coloring code (for an
embarrassingly long time considering that it still has some bugs!), let
me just say thank you for taking this on. :)

Moreover, I'll echo Dscho's comments elsewhere on the quality of this
series. It's a tricky topic to explain, and the way you've broken it up,
along with the commit messages, comments, and diagrams made it much
easier to follow.

Others have already commented on things I saw while reading it, so I'll
just add a few more thoughts.

> This series of patches are designed to improve the output of the log --graph
> command; their effect can be summed up in the following diagram:
> 
>     Before                    After
>     ------                    -----
> 
>     *
>     |\
>     | *                       *
>     | |\                      |\
>     | | *                     | *
>     | | |                     | |\
>     | |  \                    | | *
>     | *-. \                   | * |
>     | |\ \ \                  |/|\|
>     |/ / / /                  | | *
>     | | | /                   | * |
>     | | |/                    | |/
>     | | *                     * /
>     | * |                     |/
>     | |/                      *
>     * |
>     |/
>     *

I wondered if anybody would prefer the sparseness of the "before"
diagram, and if that would merit having two modes that could selected at
runtime. I'm not sure I'd want to carry the code for both types, though;
it seems like a recipe for the non-default output format to accrue a
bunch of bugs (since the graph code has proven itself to be a magnet for
off-by-ones and other weirdness).

Diffing the output of "git diff --color --graph --oneline" on git.git
both before and after your patch, the changes all look generally
positive to me. The graph above is pretty dense, but that's not really
what real-world graphs look like; it was designed to show off the
changes.

That plus the fact that you're fixing real bugs (like the octopus
coloring) makes me inclined to just move to your suggested output.

-Peff

  parent reply	other threads:[~2019-10-13  7:18 UTC|newest]

Thread overview: 83+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-10 16:13 [PATCH 00/11] Improve the readability of log --graph output James Coglan via GitGitGadget
2019-10-10 16:13 ` [PATCH 01/11] graph: automatically track visible width of `strbuf` James Coglan via GitGitGadget
2019-10-10 21:07   ` Johannes Schindelin
2019-10-10 23:05     ` Denton Liu
2019-10-11  0:49       ` Derrick Stolee
2019-10-11  1:42       ` Junio C Hamano
2019-10-11  5:01         ` Denton Liu
2019-10-11 16:02           ` Johannes Schindelin
2019-10-11 17:20             ` James Coglan
2019-10-12  0:27               ` Junio C Hamano
2019-10-12 16:22                 ` Johannes Schindelin
2019-10-14 12:55                 ` James Coglan
2019-10-14 13:01                   ` James Coglan
2019-10-16  2:15                   ` Junio C Hamano
2019-10-11  1:40     ` Junio C Hamano
2019-10-11 17:08     ` James Coglan
2019-10-10 16:13 ` [PATCH 02/11] graph: reuse `find_new_column_by_commit()` James Coglan via GitGitGadget
2019-10-10 16:13 ` [PATCH 03/11] graph: reduce duplication in `graph_insert_into_new_columns()` James Coglan via GitGitGadget
2019-10-10 16:13 ` [PATCH 04/11] graph: remove `mapping_idx` and `graph_update_width()` James Coglan via GitGitGadget
2019-10-10 16:13 ` [PATCH 06/11] graph: tidy up display of left-skewed merges James Coglan via GitGitGadget
2019-10-10 17:19   ` Derrick Stolee
2019-10-11 16:50     ` James Coglan
2019-10-12  1:36       ` Derrick Stolee
2019-10-14 13:11         ` James Coglan
2019-10-10 16:13 ` [PATCH 05/11] graph: extract logic for moving to GRAPH_PRE_COMMIT state James Coglan via GitGitGadget
2019-10-10 16:13 ` [PATCH 07/11] graph: commit and post-merge lines for left-skewed merges James Coglan via GitGitGadget
2019-10-10 17:49   ` Derrick Stolee
2019-10-11 17:04     ` James Coglan
2019-10-13  6:56       ` Jeff King
2019-10-14 15:38         ` James Coglan
2019-10-14 17:41           ` Derrick Stolee
2019-10-14 20:42           ` Johannes Schindelin
2019-10-10 16:13 ` [PATCH 08/11] graph: rename `new_mapping` to `old_mapping` James Coglan via GitGitGadget
2019-10-10 16:13 ` [PATCH 09/11] graph: smooth appearance of collapsing edges on commit lines James Coglan via GitGitGadget
2019-10-10 16:13 ` [PATCH 10/11] graph: flatten edges that join to their right neighbor James Coglan via GitGitGadget
2019-10-10 16:13 ` [PATCH 11/11] graph: fix coloring of octopus dashes James Coglan via GitGitGadget
2019-10-10 18:16   ` Denton Liu
2019-10-10 18:28     ` Denton Liu
2019-10-13  7:22     ` Jeff King
2019-10-10 17:54 ` [PATCH 00/11] Improve the readability of log --graph output Derrick Stolee
2019-10-13  7:15 ` Jeff King [this message]
2019-10-14 15:49   ` James Coglan
2019-10-15 23:40 ` [PATCH v2 00/13] " James Coglan via GitGitGadget
2019-10-15 23:40   ` [PATCH v2 01/13] graph: automatically track display width of graph lines James Coglan via GitGitGadget
2019-10-15 23:40   ` [PATCH v2 02/13] graph: handle line padding in `graph_next_line()` James Coglan via GitGitGadget
2019-10-15 23:40   ` [PATCH v2 03/13] graph: reuse `find_new_column_by_commit()` James Coglan via GitGitGadget
2019-10-15 23:40   ` [PATCH v2 04/13] graph: reduce duplication in `graph_insert_into_new_columns()` James Coglan via GitGitGadget
2019-10-15 23:40   ` [PATCH v2 05/13] graph: remove `mapping_idx` and `graph_update_width()` James Coglan via GitGitGadget
2019-10-15 23:40   ` [PATCH v2 06/13] graph: extract logic for moving to GRAPH_PRE_COMMIT state James Coglan via GitGitGadget
2019-10-15 23:40   ` [PATCH v2 07/13] graph: example of graph output that can be simplified James Coglan via GitGitGadget
2019-10-15 23:40   ` [PATCH v2 08/13] graph: tidy up display of left-skewed merges James Coglan via GitGitGadget
2019-10-15 23:41   ` [PATCH v2 09/13] graph: commit and post-merge lines for " James Coglan via GitGitGadget
2019-10-15 23:41   ` [PATCH v2 10/13] graph: rename `new_mapping` to `old_mapping` James Coglan via GitGitGadget
2019-10-15 23:41   ` [PATCH v2 11/13] graph: smooth appearance of collapsing edges on commit lines James Coglan via GitGitGadget
2019-10-15 23:41   ` [PATCH v2 12/13] graph: flatten edges that fuse with their right neighbor James Coglan via GitGitGadget
2019-10-15 23:41   ` [PATCH v2 13/13] graph: fix coloring of octopus dashes James Coglan via GitGitGadget
2019-10-15 23:47   ` [PATCH v3 00/13] Improve the readability of log --graph output James Coglan via GitGitGadget
2019-10-15 23:47     ` [PATCH v3 01/13] graph: automatically track display width of graph lines James Coglan via GitGitGadget
2019-10-16  3:35       ` Junio C Hamano
2019-10-16  5:10         ` Junio C Hamano
2019-10-15 23:47     ` [PATCH v3 02/13] graph: handle line padding in `graph_next_line()` James Coglan via GitGitGadget
2019-10-16  3:37       ` Junio C Hamano
2019-10-15 23:47     ` [PATCH v3 03/13] graph: reuse `find_new_column_by_commit()` James Coglan via GitGitGadget
2019-10-15 23:47     ` [PATCH v3 04/13] graph: reduce duplication in `graph_insert_into_new_columns()` James Coglan via GitGitGadget
2019-10-15 23:47     ` [PATCH v3 05/13] graph: remove `mapping_idx` and `graph_update_width()` James Coglan via GitGitGadget
2019-10-15 23:47     ` [PATCH v3 06/13] graph: extract logic for moving to GRAPH_PRE_COMMIT state James Coglan via GitGitGadget
2019-10-15 23:47     ` [PATCH v3 07/13] graph: example of graph output that can be simplified James Coglan via GitGitGadget
2019-10-17 12:30       ` Derrick Stolee
2019-10-18 15:21       ` SZEDER Gábor
2019-11-12  1:08         ` [PATCH] t4215: don't put git commands upstream of pipe Denton Liu
2019-11-12  6:57           ` Junio C Hamano
2019-11-12 10:54             ` SZEDER Gábor
2019-11-12 18:56           ` [PATCH v3] t4215: use helper function to check output Denton Liu
2019-11-13  2:05             ` Junio C Hamano
2019-10-15 23:47     ` [PATCH v3 08/13] graph: tidy up display of left-skewed merges James Coglan via GitGitGadget
2019-10-16  4:00       ` Junio C Hamano
2019-10-17 12:34         ` Derrick Stolee
2019-10-18  0:49           ` Junio C Hamano
2019-10-15 23:47     ` [PATCH v3 09/13] graph: commit and post-merge lines for " James Coglan via GitGitGadget
2019-10-15 23:47     ` [PATCH v3 10/13] graph: rename `new_mapping` to `old_mapping` James Coglan via GitGitGadget
2019-10-15 23:47     ` [PATCH v3 11/13] graph: smooth appearance of collapsing edges on commit lines James Coglan via GitGitGadget
2019-10-15 23:47     ` [PATCH v3 12/13] graph: flatten edges that fuse with their right neighbor James Coglan via GitGitGadget
2019-10-15 23:47     ` [PATCH v3 13/13] graph: fix coloring of octopus dashes James Coglan via GitGitGadget

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20191013071500.GA30652@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=gitster@pobox.com \
    --cc=jcoglan@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).