From: Jeff King <peff@peff•net>
To: Patrick Steinhardt <ps@pks•im>
Cc: Isaac Oscar Gariano <isaacoscar@live•com.au>, git@vger•kernel.org
Subject: Re: [PATCH 2/4] add-interactive: respect color.diff for diff coloring
Date: Mon, 8 Sep 2025 12:16:48 -0400 [thread overview]
Message-ID: <20250908161648.GC1308482@coredump.intra.peff.net> (raw)
In-Reply-To: <aLfs7wuFpMhg8fK_@pks.im>
On Wed, Sep 03, 2025 at 09:23:27AM +0200, Patrick Steinhardt wrote:
> > +static int check_color_config(struct repository *r, const char *var)
> > {
> > const char *value;
> > + int ret;
> > +
> > + if (repo_config_get_value(r, var, &value))
> > + ret = -1;
>
> Not an old issue, but should we use `GIT_COLOR_UNKNOWN` here?
My initial reaction was: yeah, we could probably fix this up in a
preparatory patch. But the problem is much deeper than the
add-interactive code. Nobody uses GIT_COLOR_UNKNOWN at all! Even
git_config_colorbool() just returns -1.
Moreover, it does not even use the ALWAYS/NEVER defines, but just 1 and
0. Making things even more complicated, we sometimes want to consider
"do we want color" as this always/never/auto/unknown set, and then
sometimes we collapse that (using the same variable!) into a single
true/false value.
So using that consistently and possibly switching to an enum is a much
bigger topic. It may be worth cleaning up, but I don't think it's worth
derailing this regression fix. In the meantime, I'd rather keep this
code matching the rest of the color code (it's not even really adding
new instances of "-1", but just shuffling them around).
> > - if (want_color_fd(1, -1)) {
> > + if (want_color_fd(1, s->s.use_color_diff)) {
> > struct child_process colored_cp = CHILD_PROCESS_INIT;
> > const char *diff_filter = s->s.interactive_diff_filter;
> >
>
> We're printing the diff here, and this change is the whole point of this
> commit as far as I understand as we now properly respect configured diff
> colors.
Yes. I would have liked to split it up more to make this hunk stand out,
but there's some chicken-and-egg dependencies.
> > +test_expect_success 're-coloring diff without color.interactive' '
> > + git reset --hard &&
> > +
> > + test_write_lines 1 2 3 >test &&
> > + git add test &&
> > + test_write_lines one 2 three >test &&
> > +
> > + test_write_lines s n n |
> > + force_color git \
> > + -c color.interactive=false \
> > + -c color.diff=true \
> > + -c color.diff.frag="bold magenta" \
> > + add -p >output.raw 2>&1 &&
> > + test_decode_color <output.raw >output &&
> > + test_grep "<BOLD;MAGENTA>@@" output
> > +'
> > +
>
> Should we also verify that the interactive prompts aren't colored here?
Seems reasonable. Knowing that the patch is splitting the diff coloring
off of the interactive, it would be pretty hard to introduce such a bug.
But from a black box perspective, that is probably a good thing to test.
Ultimately the best test would be for every item that _could_ be colored
by each type to be individually checked in each scenario. But I didn't
want the test to depend on enumerating those very specific details of
the code.
-Peff
next prev parent reply other threads:[~2025-09-08 16:16 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-08-20 11:05 [BUG] Some subcommands ignore color.diff and color.ui in --patch mode Isaac Oscar Gariano
2025-08-20 22:04 ` Jeff King
2025-08-20 23:48 ` Isaac Oscar Gariano
2025-08-21 7:00 ` Jeff King
2025-08-21 7:07 ` [PATCH 0/4] oddities around add-interactive and color Jeff King
2025-08-21 7:15 ` [PATCH 1/4] stash: pass --no-color to diff-tree child processes Jeff King
2025-09-03 7:23 ` Patrick Steinhardt
2025-09-08 16:06 ` Jeff King
2025-08-21 7:19 ` [PATCH 2/4] add-interactive: respect color.diff for diff coloring Jeff King
2025-09-03 7:23 ` Patrick Steinhardt
2025-09-08 16:16 ` Jeff King [this message]
2025-09-09 6:06 ` Patrick Steinhardt
2025-08-21 7:22 ` [PATCH 3/4] add-interactive: manually fall back color config to color.ui Jeff King
2025-08-21 15:42 ` Junio C Hamano
2025-09-03 7:23 ` Patrick Steinhardt
2025-09-08 16:17 ` Jeff King
2025-08-21 7:22 ` [PATCH 4/4] contrib/diff-highlight: mention interactive.diffFilter Jeff King
2025-09-08 16:41 ` [PATCH v2 0/4] oddities around add-interactive and color Jeff King
2025-09-08 16:42 ` [PATCH v2 1/4] stash: pass --no-color to diff plumbing child processes Jeff King
2025-09-08 16:42 ` [PATCH v2 2/4] add-interactive: respect color.diff for diff coloring Jeff King
2025-09-08 16:42 ` [PATCH v2 3/4] add-interactive: manually fall back color config to color.ui Jeff King
2025-09-08 16:42 ` [PATCH v2 4/4] contrib/diff-highlight: mention interactive.diffFilter Jeff King
2025-09-09 6:09 ` [PATCH v2 0/4] oddities around add-interactive and color Patrick Steinhardt
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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20250908161648.GC1308482@coredump.intra.peff.net \
--to=peff@peff$(echo .)net \
--cc=git@vger$(echo .)kernel.org \
--cc=isaacoscar@live$(echo .)com.au \
--cc=ps@pks$(echo .)im \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox