git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration
Date: Fri, 13 Oct 2017 09:06:38 -0400	[thread overview]
Message-ID: <20171013130638.dgc6kawy5mvrbasz@sigill.intra.peff.net> (raw)
In-Reply-To: <xmqqzi8vvht6.fsf@gitster.mtv.corp.google.com>

On Fri, Oct 13, 2017 at 12:37:57PM +0900, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > OK. For the record, I'm not against scrapping this whole thing and
> > trying to rollback to your "plumbing never looks at color.ui" proposal.
> > It's quite late in the -rc cycle to do that, but there's nothing that
> > says we can't bump the release date if that's what we need to do to get
> > it right.
> 
> I think that it is too late, regardless of our release cycle.
> 
> "Plumbing never looks at color.ui" implies that "plumbing must not
> get color.ui=auto from 4c7f1819", but given that 4c7f1819 is from
> 2013, I'd be surprised if we stopped coloring output from plumbing
> without getting any complaints from third-party script writers.

I agree that 4c7f1819 is the root of things. But there also weren't a
lot of people complaining about it. I only noticed it as part of other
work I was doing, and (perhaps foolishly) tried to clean it up.

All of the regressions people have actually _noticed_ stem from my
136c8c8b8f in v2.14.2. So I think it is a viable option to try to go
back to the pre-v2.14.2 state. I.e.:

  1. Revert my 20120618c1 (color: downgrade "always" to "auto" only for
     on-disk configuration, 2017-10-10) and the test changes that came
     with it.

  2. Teach for-each-ref and tag to use git_color_default_config(). These
     are the two I _know_ need this treatment as part of the series that
     contained 136c8c8b8f. As you've noted there may be others, but I'd
     be surprised if there are many. There hasn't been a lot of color
     work in the last few months besides what I've done.

  3. Revert 136c8c8b8f.

That takes us back to the pre-regression state. The ancient bug from
4c7f1819 still exists, but that would be OK for v2.15. We'd probably
want to bump the -rc cycle a bit to give more confidence that (2) caught
everything.

Post-release, we would either:

  a. Do nothing. As far as we know, nobody has cared deeply about
     4c7f1819 for the past 4 years.

  b. Teach git_default_config() to respect "never" but not "always", so
     that you can disable the auto-color in the plumbing (but not shoot
     yourself in the foot).

  c. Go all-out and remove the "auto" behavior from plumbing. This is
     much more likely to have fallouts since we've had 4 years of the
     wrong behavior. But we'd have a whole cycle to identify
     regressions.

I'd probably vote for (b), followed by (a). Option (c) seems like a lot
of risk for little benefit.

But we could punt on that part until after the release. The only thing
we'd need to decide on now is that first set of reversions. What I
really _don't_ want to do is ship v2.15 with "always works like auto"
and then flip that back in v2.16.

I know you're probably infuriated with me because I'm essentially
arguing the opposite of what I did earlier in the cycle. And I really am
OK with going either way (shipping what's in -rc1 plus the "let 'always'
work from the command line", or doing something like what I outlined
above). But you've convinced me that the road I was going down really is
piling up the hacks, and I want to make it clear that I'm not married to
following the path I outlined earlier, and that I think there _is_ a
viable alternative.

-Peff

  reply	other threads:[~2017-10-13 13:06 UTC|newest]

Thread overview: 42+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-10-09 23:58 What happened to "git status --color=(always|auto|never)"? Nazri Ramliy
2017-10-10  0:16 ` Jonathan Nieder
2017-10-10  0:43   ` Nazri Ramliy
2017-10-10  0:59     ` Jonathan Nieder
2017-10-10  4:42       ` Nazri Ramliy
2017-10-10 10:25         ` Jeff King
2017-10-10 12:51           ` Junio C Hamano
2017-10-10 13:06             ` Jeff King
2017-10-10 19:03               ` Jonathan Nieder
2017-10-10 19:37                 ` Jeff King
2017-10-11  2:05                   ` Junio C Hamano
2017-10-12  2:10                     ` [PATCH 0/2] Piling more kludge on top of color.ui Junio C Hamano
2017-10-12  2:10                       ` [PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration Junio C Hamano
2017-10-12  4:47                         ` Jonathan Nieder
2017-10-12  5:05                           ` Junio C Hamano
2017-10-12  5:40                             ` Jonathan Nieder
2017-10-12  6:15                               ` Junio C Hamano
2017-10-12  6:58                                 ` Junio C Hamano
2017-10-12 13:06                                   ` Jeff King
2017-10-12 15:12                                     ` Jeff King
2017-10-12 12:31                         ` Jeff King
2017-10-13  0:09                           ` Junio C Hamano
2017-10-13  1:47                             ` Jeff King
2017-10-13  3:37                               ` Junio C Hamano
2017-10-13 13:06                                 ` Jeff King [this message]
2017-10-13 17:20                                   ` [PATCH 0/4] peeling back color.ui=always hacks Jeff King
2017-10-13 17:23                                     ` [PATCH 1/4] Revert "color: make "always" the same as "auto" in config" Jeff King
2017-10-13 17:23                                     ` [PATCH 2/4] Revert "t6006: drop "always" color config tests" Jeff King
2017-10-13 17:24                                     ` [PATCH 3/4] Revert "color: check color.ui in git_default_config()" Jeff King
2017-10-13 17:26                                     ` [PATCH 4/4] tag: respect color.ui config Jeff King
2017-10-14  3:01                                   ` [PATCH 1/2] color: downgrade "always" to "auto" only for on-disk configuration Junio C Hamano
2017-10-16 21:53                                     ` Jeff King
2017-10-17  1:06                                       ` Junio C Hamano
2017-10-17  6:26                                         ` Junio C Hamano
2017-10-18  5:28                                           ` Jeff King
2017-10-18  5:57                                             ` Junio C Hamano
2017-10-17  6:51                                         ` Jonathan Nieder
2017-10-18  5:34                                           ` Jeff King
2017-10-12  2:10                       ` [PATCH 2/2] color: discourage use of ui.color=always Junio C Hamano
2017-10-12  4:48                         ` Jonathan Nieder
2017-10-12 15:08                         ` Jeff King
2017-10-13  0:02                           ` Junio C Hamano

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=20171013130638.dgc6kawy5mvrbasz@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.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).