Re: Updated to v2.14.2 on macOS; git add --patch broken

From: Jeff King <peff@peff.net>
Date: Oct 3, 2017, 13:37 UTC
Message-ID: <20171003133713.ccxv6clrmuuhhc3u@sigill.intra.peff.net>
In-Reply-To: <xmqqpoa4h5y7.fsf@gitster.mtv.corp.google.com>

On Tue, Oct 03, 2017 at 07:38:24PM +0900, Junio C Hamano wrote:

Show 11 quoted lines

> That's an argument to say color.ui=always is not a useful thing to
> use and Git is wrong to offer such a nonsense option.  I agree with
> both of them.
> 
> But it is a different matter that plumbing commands are now doing
> the "color" thing without being asked.  Reading the configuration
> that is meant for human interaction adds insult to injury.  I think
> the earlier "everybody is colored by default" that forgot to make
> sure the change does not affect plumbing was the main culprit, and
> we were merely lucky that thanks to the auto-detection of "auto" we
> did not break scripts.

Yes, I agree that the "everybody is colored by default" is the root of the problem. And in that sense all of this discussion is band-aid fixes on that.

At the same time, I have a suspicion that "even plumbing is color=auto by default" may actually be _helping_ in many scripts. Because they to use the plumbing to show output to the user, respecting the user's normal color preferences. That's just a hunch, though.

I also think trying to revert that "default" patch (4c7f1819) may be a pretty big pain at this point.

  Side note: Sorry, I noticed while writing this that I got my commit
  references mixed up earlier between 4c7f1819 (which turned the default
  to auto even for plumbing) and 136c8c8b, my recent change to parse
  color.ui in more places. When I said the problem was exacerbated/made
  worse by 4c7f1819, I really meant 136c8c8b. Hopefully that didn't
  confuse the discussion too much.

Show 12 quoted lines

> Having said all that, unless we revert the entire series (and
> possibly other things that happened after the series was merged on
> 'master' that assumes that now default_config would read the
> color.ui thing), we won't be able to get back to the state before
> the "add -p" regression, it seems.  As -rc freeze period for the
> next cycle is approaching fast, I wanted to make sure that we have a
> plan to address the regression (which does not have to solve what
> the reverted commit tried to solve).  If you think we can get a
> workable code in 2.14.x and 'master' that essentially castrates
> "always" and makes it the same as "auto" in several days tops, then
> I'd prefer such a solution, as honoring "color.ui=always" does not
> feel all that important.

Here's what I came up with on the "color.ui=always is nonsense that we should not offer" front. The number of patches may be a little daunting, but most of them are just removing cases of "git -c color.ui=always" from the tests.

  [01/12]: test-terminal: set TERM=vt100
  [02/12]: t4015: prefer --color to -c color.diff=always
  [03/12]: t4015: use --color with --color-moved
  [04/12]: t3701: use test-terminal to collect color output
  [05/12]: t7508: use test_terminal for color output
  [06/12]: t7502: use diff.noprefix for --verbose test
  [07/12]: t6006: drop "always" color config tests
  [08/12]: t3203: drop "always" color test
  [09/12]: t7301: use test_terminal to check color
  [10/12]: t3205: use --color instead of color.branch=always
  [11/12]: provide --color option for all ref-filter users
  [12/12]: color: make "always" the same as "auto" in config

 Documentation/config.txt           | 35 ++++++++++++-------------
 Documentation/git-for-each-ref.txt |  5 ++++
 Documentation/git-tag.txt          |  5 ++++
 builtin/for-each-ref.c             |  1 +
 builtin/tag.c                      |  1 +
 color.c                            |  2 +-
 t/t3203-branch-output.sh           |  8 +-----
 t/t3205-branch-color.sh            |  5 ++--
 t/t3701-add-interactive.sh         | 18 +++++++++----
 t/t4015-diff-whitespace.sh         | 53 +++++++++++++++++++-------------------
 t/t4202-log.sh                     |  2 +-
 t/t6006-rev-list-format.sh         | 23 +++++------------
 t/t6300-for-each-ref.sh            |  7 +++--
 t/t7004-tag.sh                     |  6 ++---
 t/t7006-pager.sh                   |  6 ++---
 t/t7301-clean-interactive.sh       |  5 ++--
 t/t7502-commit.sh                  |  4 +--
 t/t7508-status.sh                  | 41 +++++++++++++++--------------
 t/test-terminal.perl               |  1 +
 19 files changed, 115 insertions(+), 113 deletions(-)

-Peff