Re: [PATCH] diff-tree: obey the color.ui configuration

From: Jeff King <peff@peff.net>
Date: Dec 30, 2017, 14:45 UTC
Message-ID: <20171230144505.GA29252@sigill.intra.peff.net>
In-Reply-To: <87tvw875vh.fsf@evledraar.gmail.com>

On Sat, Dec 30, 2017 at 01:33:06PM +0100, Ævar Arnfjörð Bjarmason wrote:

Show 10 quoted lines

> >   - we ended up with 33c643bb08 (Revert "color: check color.ui in
> >     git_default_config()", 2017-10-13), which just reverts the whole
> >     mess back to the pre-v2.14 state. This shipped in v2.15.
> 
> Thanks. What a mess.
> 
> I haven't tried that add-interactive case you mentioned, an earlier
> version of this patch where I tried adding the color detection in
> git_diff_basic_config() did break one of its tests, but not my ptch, but
> it's probably still broken with =always (haven't tested.

It should break a test, since I added one in 33c643bb083. :)

That covers "add -p", though, which only does diff-files under the hood. You can convince it to run "diff-index", too, but I don't think diff-tree. So technically your patch doesn't break add--interactive, but probably does break some other script we don't know about. ;)

Show 12 quoted lines

> > So I don't think we want to go down that road again. If anything, we
> > want to either fix the original sin from 4c7f1819b3, or we want to do
> > the "respect only never" hack.
> 
> Getting back to the bug report that prompted this whole thing, wouldn't
> the easiest solution just to run "git show --stat $commit" instead of
> "git diff-tree --pretty $commit" when bisect wants to report the commit
> it found?
> 
> I've always thought the output was a bit ugly, it's plumbing command, so
> why wouldn't we just show the commit as the user usually prefers to see
> commits?

I like that solution. I've often found the output ugly, too. And in particular, it doesn't show any output at all for merge commits. Doing "diff-tree --cc --stat" would be the minimal output improvement there.

I do like the idea of using "show", though. We know the point is to show the output to the user, so we don't mind at all if the behavior or output of show changes in future versions (unless we consider the final output of bisect to be machine-readable, but I certainly don't).

-Peff