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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Dec 30, 2017, 15:04 UTC
Message-ID: <87po6w6yul.fsf@evledraar.gmail.com>
In-Reply-To: <20171230144505.GA29252@sigill.intra.peff.net>

On Sat, Dec 30 2017, Jeff King jotted:

Show 14 quoted lines

> On Sat, Dec 30, 2017 at 01:33:06PM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>> >   - 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. :)

Right, the one I tried first broke that, but not this version....

> 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. ;)

...Yeah, for sure.

Show 21 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).

Not knowing the internal APIs for that well, is this basically a matter of copy/pasting (or factoring out into a function), some of this:

    git grep -W cmd_show -- builtin/log.c

I.e. boilerplate + calling cmd_log_walk() to yield a result similar to e22278c0a0 ("bisect: display first bad commit without forking a new process", 2009-05-28).

Or is it preferred to just fake up argc/argv and call cmd_show() directly? I haven't seen many examples of that in the codebase:

    git grep -W '(return|=)\s*cmd.*argc' -- '*.c'

But I don't see why it wouldn't work, the cmd_show() doesn't call exit() itself, and we're right about to call exit anyway when our current diff-tree invocation is called.