Git Mailing List Archive on lore.kernel.org
 help / color / Atom feed
From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: Eyal Soha <shawarmakarma@gmail.com>, git@vger.kernel.org
Subject: Re: [PATCH 1/3] color.c: Refactor color_output to use enums
Date: Thu, 16 Jan 2020 13:23:31 -0500
Message-ID: <20200116182331.GA2946050@coredump.intra.peff.net> (raw)
In-Reply-To: <xmqq5zhbi8l3.fsf@gitster-ct.c.googlers.com>

On Thu, Jan 16, 2020 at 10:01:44AM -0800, Junio C Hamano wrote:

> Not that I agree with the (untold) reasoning why we chose to use
> 30-37 instead of 0-7, though.  If this were up to me, I would have
> rather defined COLOR_BACKGROUND_ANSI = 40, kept .value to 0-7 and
> passed COLOR_{FORE,BACK}GROUPD_ANSI to callers of color_output().
> 
> Since I haven't seen 2/3 and 3/3, perhaps there is a good reason why
> this step was done this way instead, though, I guess.

Yeah, it becomes more clear in patch 2, where the value can be either
"31" or "91", for the bright or non-bright variant, and adding "30" is
wrong. (But certainly I agree this needs to be explained here).

Another way to write it would be to store 0-7 in the value as before,
and then add a separate "bright" flag to "struct color". And then the
output becomes:

  COLOR_FOREGROUND_OFFSET + c->value + (c->bright ? COLOR_BRIGHT_OFFSET : 0)

or similar. One minor confusion there is that COLOR_256 and COLOR_RGB
would ignore the "bright" field.

-Peff

  reply index

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <CANsz78+ugmd62F4Qk+VT7Pi=ZPtMSkZjXOwLNRCFhoS9jrOkeQ@mail.gmail.com>
     [not found] ` <CANsz78K-BiswWPdhd_N25NuApcv7zSb2cw2Y9DSinkpNpuogYw@mail.gmail.com>
2020-01-07 15:36   ` Fwd: Add support for axiterm colors in parse_color Eyal Soha
2020-01-08  9:52     ` Jeff King
2020-01-10  0:20       ` Eyal Soha
2020-01-10 11:15         ` Jeff King
2020-01-10 15:02           ` Eyal Soha
2020-01-15 15:32             ` Eyal Soha
2020-01-10 15:05           ` [PATCH 1/3] color.c: Refactor color_output to use enums Eyal Soha
2020-01-10 15:05             ` [PATCH 2/3] color.c: Support bright aixterm colors Eyal Soha
2020-01-15 22:42               ` Jeff King
2020-01-10 15:05             ` [PATCH 3/3] color.c: Alias RGB colors 8-15 to " Eyal Soha
2020-01-15 22:45               ` Jeff King
2020-01-15 22:33             ` [PATCH 1/3] color.c: Refactor color_output to use enums Jeff King
2020-01-16 18:01             ` Junio C Hamano
2020-01-16 18:23               ` Jeff King [this message]
2020-01-16 19:25                 ` Eyal Soha

Reply instructions:

You may reply publically 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

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

  git send-email \
    --in-reply-to=20200116182331.GA2946050@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=shawarmakarma@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

Git Mailing List Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/git/0 git/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 git git/ https://lore.kernel.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.git


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git