Re: [BUG] A part of an edge from an octopus merge gets colored, even with --color=never

From: Jeff King <peff@peff.net>
Date: Oct 9, 2018, 04:51 UTC
Message-ID: <20181009045138.GA11376@sigill.intra.peff.net>
In-Reply-To: <CAM-tV-88J3ZAALwZeEqTuvKXRwLzb848G0AET2Ec6ic85=7o8Q@mail.gmail.com>

On Wed, Oct 03, 2018 at 06:32:06PM -0400, Noam Postavsky wrote:

Show 18 quoted lines

> > which is admittedly pretty horrible, too, but at least resembles a
> > graph. I dunno.
> 
> Yeah, but it's lossy, so it doesn't seem usable for the test. Maybe
> doubling up some characters?
> 
> **  left
> R|  **B-B-M-M.      octopus-merge
> R|  R|Y\  B\  M\
> R|R/  Y/  B/  M/
> R|  Y|  B|  **  4
> R|  Y|  **  M|  3
> R|  Y|  M|M/
> R|  **  M|  2
> R|  M|M/
> **  M|  1
> M|M/
> **  initial

Yeah, I tried something similar, but it's hard to read as a graph since the alignment is lost between lines. I agree the single-char version is lossy, but I think in combination with checking the literal, uncolored version, we'd be OK.

However, it may be best to just leave the original verbose version you had. It's hard to read and to modify, but we don't plan for people to do that very often. And it's at least simple.

Show 7 quoted lines

> > I'm also not thrilled that we depend on the exact sequence of default
> > colors, but I suspect it's not the first time. And it wouldn't be too
> > hard to update it if that default changes.
> 
> Well, it's easy enough to set the colors explicitly. After doing this
> I noticed that green seems to be skipped. Not sure if that's a bug or
> not.

Hmm, yeah, that is weird. I think it's an artifact of the way we increment the color selector, though, and not related to your patch (the same thing happens before your fix, as well).

Show 9 quoted lines

> > I think it's OK to have a dedicated script for even these two tests, if
> > it makes things easier to read. However, would we also want to test the
> > octopus without the problematic graph here? I think if we just omit
> > "left" we get that, don't we?
> 
> t4202-log.sh already does test a "normal" octopus merge (starting
> around line 615, search for "octopus-a"). But that is only a 3-parent
> merge. And adding another test is easy enough.
> [...]

Thanks, what you have here looks good.

> From cd9415b524357c2c8b9b20a63032c94e01d46a15 Mon Sep 17 00:00:00 2001
> From: Noam Postavsky <npostavs@users.sourceforge.net>
> Date: Sat, 1 Sep 2018 20:07:16 -0400
> Subject: [PATCH v5] log: Fix coloring of certain octupus merge shapes

This whole version looks good to me. "git am" is supposed to understand attachments, but it seems to want to apply our whole conversation as the commit message.

You may want to repost one more time with this subject in the email subject line to fix that and to get the maintainer's attention. Feel free to add my:

  Reviewed-by: Jeff King <peff@peff.net>

after your signoff. Thanks for sticking with this topic!

-Peff