{"thread":{"id":"63994","subject":"[BUG] Some subcommands ignore color.diff and color.ui in --patch mode","startedAt":"2025-08-20T11:05:57Z","lastAt":"2025-09-09T06:09:08Z","messageCount":23,"participants":["Isaac Oscar Gariano","Jeff King","Junio C Hamano","Patrick Steinhardt"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"524517","messageId":"SYBP282MB296329544B33E3C16DD99FD28C33A@SYBP282MB2963.AUSP282.PROD.OUTLOOK.COM","threadId":"63994","inReplyTo":null,"subject":"[BUG] Some subcommands ignore color.diff and color.ui in --patch mode","fromName":"Isaac Oscar Gariano","fromEmail":"isaacoscar@live.com.au","sentAt":"2025-08-20T11:05:53Z","receivedAt":"2025-08-20T11:05:57Z","isPatch":false,"sender":{"key":"isaacoscar@live.com.au","avatar":null},"body":"Bassically the colouring behaviour of the interactive --patch option to the various commands differ.\nI'll call \"commit, add, and stash the \"good commands\" (as they behave as I expect), and stash push, stash save, checkout, reset, and restore the \"bad commands\" (which are bugged).\n\nI assume you have git v2.50.1, a dirty working tree, and no colour related settings in any of the config files, and where $CMD is the name of any \"bad command\".\n\nThe following all print in colour (I expect no colour):\n    git -c color.diff=never        $CMD --patch .\n    git -c color.ui=never          $CMD --patch .\n\nThe folowing do not print anything in colour (I expect it to work the same as without the cat):\n    git -c color.diff=always        $CMD --patch . | cat\n    git -c color.ui=always          $CMD --patch . | cat\n\nNow the documenation for color.interactive says:\n    When set to always, always use colors for interactive prompts and displays (such as those used by \"git-add --interactive\" and\n    \"git-clean --interactive\"). When false (or never), never. When set to true or auto, use colors only when the output is to the\n    terminal. If unset, then the value of color.ui is used (auto by default).\n\nNow the bad commands are respecting the setting of color.interactive corroectly (and the same as the good commands).\nFor example, this always prints a coloured prompt (but not a coloured diff)\n    git -c color.interactive=always          $CMD --patch . | cat\n\nBut as mentioned above, \"color.ui=always\" will NOT print the prompt in color.\n\nAs for why I care, I was trying to pipe git restore through diff-highlight (this functionality should really be inbuilt into git diff)\n\nA related issue, that is probably not a 'bug': all the --patch options ignore the diff config options (e.g. diff.wordRegex).\n\n— Isaac Oscar Gariano​\n"},{"id":"524582","messageId":"20250820220439.GA1668511@coredump.intra.peff.net","threadId":"63994","inReplyTo":"SYBP282MB296329544B33E3C16DD99FD28C33A@SYBP282MB2963.AUSP282.PROD.OUTLOOK.COM","subject":"Re: [BUG] Some subcommands ignore color.diff and color.ui in --patch mode","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-08-20T22:04:39Z","receivedAt":"2025-08-20T22:04:42Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 20, 2025 at 11:05:53AM +0000, Isaac Oscar Gariano wrote:\n\n> Bassically the colouring behaviour of the interactive --patch option\n> to the various commands differ.\n> I'll call \"commit, add, and stash the \"good commands\" (as they behave\n> as I expect), and stash push, stash save, checkout, reset, and restore\n> the \"bad commands\" (which are bugged).\n\nI think this is a regression in the conversion of the interactive-patch\ncode from a perl script to a C builtin. Bisecting points to 0527ccb1b5\n(add -i: default to the built-in implementation, 2021-11-30).\n\nWithout digging too deeply, I'd guess the issue is that the original\nperl script loaded all config itself. But now that the code runs\nin-process, it is depending on the outer command to have loaded the\ncolor.ui setting. And indeed, the code here:\n\n $ git grep -A3 color.interactive add-interactive.c\n add-interactive.c: if (repo_config_get_value(r, \"color.interactive\", &value))\n add-interactive.c- s->use_color = -1;\n add-interactive.c- else\n add-interactive.c- s->use_color =\n add-interactive.c: git_config_colorbool(\"color.interactive\", value);\n add-interactive.c- s->use_color = want_color(s->use_color);\n\nshows that we consult color.interactive correctly, but then depend on\nwant_color() to do any fallback to color.ui. And that is just looking at\na pre-set variable:\n\n \n int want_color_fd(int fd, int var)\n {\n [...]\n if (var < 0)\n var = git_use_color_default;\n [...]\n }\n\nwhich is expected to be set by the outer command loading the color\nconfig via git_color_config(). And that's why it works for \"git add\",\nbut not \"git checkout\". Either \"checkout\" (and other commands) should\nlearn to call git_color_config(), or the interactive code should itself\nlearn to handle the fallback.\n\nI'd expect something like this:\n\ndiff --git a/add-interactive.c b/add-interactive.c\nindex 3e692b47ec..ad8b4907e1 100644\n--- a/add-interactive.c\n+++ b/add-interactive.c\n@@ -50,6 +50,8 @@ void init_add_i_state(struct add_i_state *s, struct repository *r,\n \telse\n \t\ts->use_color =\n \t\t\tgit_config_colorbool(\"color.interactive\", value);\n+\tif (s->use_color < 0 && !repo_config_get_value(r, \"color.ui\", &value))\n+\t\ts->use_color = git_config_colorbool(\"color.ui\", value);\n \ts->use_color = want_color(s->use_color);\n \n \tinit_color(r, s, \"interactive.header\", s->header_color, GIT_COLOR_BOLD);\n\nto work, but it doesn't seem to. Maybe the diff code is independently\nlooking at git_use_color_default, and we really do need to set the\nvariable?\n\nAt any rate, I think there may be a simpler workaround for you...\n\n> As for why I care, I was trying to pipe git restore through\n> diff-highlight (this functionality should really be inbuilt into git\n> diff)\n\nHave you tried setting interactive.diffFilter to \"diff-highlight\"?\nThat's what it was designed for.\n\n> A related issue, that is probably not a 'bug': all the --patch options\n> ignore the diff config options (e.g. diff.wordRegex).\n\nThe interactive patch options use the diff plumbing under the hood,\nbecause they have certain requirements from the output. For example, you\ncan't apply a word-diff (or a colorized one for that matter; the color\nis handled specially by generating the diff twice, once with color and\nonce without, and assuming that the lines correspond between them).\n\nSo the interactive code has to manually interpret any diff options that\nit thinks are OK and pass them along to the underlying diff command.\nThere are undoubtedly some that would make sense for it to handle, but\nnobody has cared enough yet to teach it (I think it just learned about\ndiff.context in the latest release).\n\nI'm not sure if diff.wordRegex is such a case, though. You can't apply a\nword diff (and it does not have line-to-line correspondence with a\nnon-word diff, so you can't show one and apply the other). But there may\nbe spots where we'd use it for generating a normal unified diff.\n\n-Peff\n"},{"id":"524590","messageId":"SY4P282MB2965003F2D5DF18C6252978A8C33A@SY4P282MB2965.AUSP282.PROD.OUTLOOK.COM","threadId":"63994","inReplyTo":"20250820220439.GA1668511@coredump.intra.peff.net","subject":"Re: [BUG] Some subcommands ignore color.diff and color.ui in --patch mode","fromName":"Isaac Oscar Gariano","fromEmail":"isaacoscar@live.com.au","sentAt":"2025-08-20T23:48:36Z","receivedAt":"2025-08-20T23:48:39Z","isPatch":false,"sender":{"key":"isaacoscar@live.com.au","avatar":null},"body":"> Have you tried setting interactive.diffFilter to \"diff-highlight\"?\n> That's what it was designed for.\nWow thanks! that worked perfectly. You really should put that in the Readme (it only tells you to set pager.)."},{"id":"524600","messageId":"20250821070050.GA3905042@coredump.intra.peff.net","threadId":"63994","inReplyTo":"SY4P282MB2965003F2D5DF18C6252978A8C33A@SY4P282MB2965.AUSP282.PROD.OUTLOOK.COM","subject":"Re: [BUG] Some subcommands ignore color.diff and color.ui in --patch mode","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-08-21T07:00:50Z","receivedAt":"2025-08-21T07:00:54Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 20, 2025 at 11:48:36PM +0000, Isaac Oscar Gariano wrote:\n\n> > Have you tried setting interactive.diffFilter to \"diff-highlight\"?\n> > That's what it was designed for.\n> Wow thanks! that worked perfectly. You really should put that in the\n> Readme (it only tells you to set pager.).\n\nYes, I suspect that README hasn't been touched since well before the\nconfig option was introduced. ;)\n\nI'm preparing a few patches and will include that.\n\n-Peff\n"},{"id":"524602","messageId":"20250821070740.GA3356411@coredump.intra.peff.net","threadId":"63994","inReplyTo":"20250820220439.GA1668511@coredump.intra.peff.net","subject":"[PATCH 0/4] oddities around add-interactive and color","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-08-21T07:07:40Z","receivedAt":"2025-08-21T07:07:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 20, 2025 at 06:04:40PM -0400, Jeff King wrote:\n\n> I'd expect something like this:\n> \n> diff --git a/add-interactive.c b/add-interactive.c\n> index 3e692b47ec..ad8b4907e1 100644\n> --- a/add-interactive.c\n> +++ b/add-interactive.c\n> @@ -50,6 +50,8 @@ void init_add_i_state(struct add_i_state *s, struct repository *r,\n> \telse\n> \t\ts->use_color =\n> \t\t\tgit_config_colorbool(\"color.interactive\", value);\n> +\tif (s->use_color < 0 && !repo_config_get_value(r, \"color.ui\", &value))\n> +\t\ts->use_color = git_config_colorbool(\"color.ui\", value);\n> \ts->use_color = want_color(s->use_color);\n> \n> \tinit_color(r, s, \"interactive.header\", s->header_color, GIT_COLOR_BOLD);\n> \n> to work, but it doesn't seem to. Maybe the diff code is independently\n> looking at git_use_color_default, and we really do need to set the\n> variable?\n\nAh, indeed. There's yet another bug here. And while adding a test for\nthat, I found a third bug. Yikes.\n\nSo here's a series which I think addresses everything I found. These\nbugs have been lurking for a while, but I guess not many people tend to\nset color variables to anything exotic.\n\n [1/4]: stash: pass --no-color to diff-tree child processes\n [2/4]: add-interactive: respect color.diff for diff coloring\n [3/4]: add-interactive: manually fall back color config to color.ui\n [4/4]: contrib/diff-highlight: mention interactive.diffFilter\n\n add-interactive.c | 88 ++++++++++++++++++++++-------------\n add-interactive.h | 7 ++-\n add-patch.c | 12 ++---\n builtin/stash.c | 4 +-\n contrib/diff-highlight/README | 8 ++++\n t/t3701-add-interactive.sh | 51 ++++++++++++++++++++\n t/t3904-stash-patch.sh | 10 ++++\n 7 files changed, 138 insertions(+), 42 deletions(-)\n\n-Peff\n"},{"id":"524603","messageId":"20250821071517.GA1839835@coredump.intra.peff.net","threadId":"63994","inReplyTo":"20250821070740.GA3356411@coredump.intra.peff.net","subject":"[PATCH 1/4] stash: pass --no-color to diff-tree child processes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-08-21T07:15:17Z","receivedAt":"2025-08-21T07:15:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"After a partial stash, we may clear out the working tree by capturing\nthe output of diff-tree and piping it into git-apply. So we most\ndefinitely do not want color diff output from that diff-tree process.\nAnd it normally would not produce any, since its stdout is not going to\na tty, and the default value of color.ui is \"auto\".\n\nHowever, if GIT_PAGER_IN_USE is set in the environment, that overrides\nthe tty check, and we'll produce a colorized diff that chokes git-apply:\n\n $ echo y | GIT_PAGER_IN_USE=1 git stash -p\n [...]\n Saved working directory and index state WIP on main: 4f2e2bb foo\n error: No valid patches in input (allow with \"--allow-empty\")\n Cannot remove worktree changes\n\nSetting this variable is a relatively silly thing to do, and not\nsomething most users would run into. But we sometimes do it in our tests\nto stimulate color. And it is a user-visible bug, so let's fix it rather\nthan work around it in the tests.\n\nThe root issue here is that diff-tree (and other diff plumbing) should\nprobably not ever produce color by default. It does so not by parsing\ncolor.ui, but because of the baked-in \"auto\" default from 4c7f1819b3\n(make color.ui default to 'auto', 2013-06-10). But changing that is\nrisky; we've had discussions back and forth on the topic over the years.\nE.g.:\n\n https://lore.kernel.org/git/86D0A377-8AFD-460D-A90E-6327C6934DFC@gmail.com/.\n\nSo let's accept that as the status quo for now and protect ourselves by\npassing --no-color to the child processes. This is the same thing we did\nfor add-interactive itself in 1c6ffb546b (add--interactive.perl: specify\n--no-color explicitly, 2020-09-07).\n\nSigned-off-by: Jeff King \n---\nI ran into this while writing tests for the subsequent patches.\n\nReading that referenced thread again, Junio was in favor of reverting\n4c7f1819b3 and replacing it with something that didn't kick in for\nplumbing (thus fixing the root issue). I argued against it somewhat\nthere, but now I think I was foolish and agree with 2017-Junio. ;) I do\nthink that fixing it now carries some risk of people complaining,\nthough. So I'd rather do this immediate fix and worry about the larger\nproblem separately.\n\nI also had another patch long ago that would have helped here:\n\n https://lore.kernel.org/git/20150810052353.GB15441@sigill.intra.peff.net/\n\nThe general idea is for GIT_PAGER_IN_USE to actually identify the pipe\nto the pager, so that sub-processes that are not going directly to the\npager know to ignore it. I think I didn't pursue it because I never\nworked out the portability issues for Windows.\n\n builtin/stash.c | 4 +++-\n t/t3904-stash-patch.sh | 10 ++++++++++\n 2 files changed, 13 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex 1977e50df2..c55628aafc 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -377,7 +377,7 @@ static int diff_tree_binary(struct strbuf *out, struct object_id *w_commit)\n \t * however it should be done together with apply_cached.\n \t */\n \tcp.git_cmd = 1;\n-\tstrvec_pushl(&cp.args, \"diff-tree\", \"--binary\", NULL);\n+\tstrvec_pushl(&cp.args, \"diff-tree\", \"--binary\", \"--no-color\", NULL);\n \tstrvec_pushf(&cp.args, \"%s^2^..%s^2\", w_commit_hex, w_commit_hex);\n \n \treturn pipe_command(&cp, NULL, 0, out, 0, NULL, 0);\n@@ -1283,6 +1283,7 @@ static int stash_staged(struct stash_info *info, struct strbuf *out_patch,\n \n \tcp_diff_tree.git_cmd = 1;\n \tstrvec_pushl(&cp_diff_tree.args, \"diff-tree\", \"-p\", \"--binary\",\n+\t\t \"--no-color\",\n \t\t \"-U1\", \"HEAD\", oid_to_hex(&info->w_tree), \"--\", NULL);\n \tif (pipe_command(&cp_diff_tree, NULL, 0, out_patch, 0, NULL, 0)) {\n \t\tret = -1;\n@@ -1345,6 +1346,7 @@ static int stash_patch(struct stash_info *info, const struct pathspec *ps,\n \n \tcp_diff_tree.git_cmd = 1;\n \tstrvec_pushl(&cp_diff_tree.args, \"diff-tree\", \"-p\", \"-U1\", \"HEAD\",\n+\t\t \"--no-color\",\n \t\t oid_to_hex(&info->w_tree), \"--\", NULL);\n \tif (pipe_command(&cp_diff_tree, NULL, 0, out_patch, 0, NULL, 0)) {\n \t\tret = -1;\ndiff --git a/t/t3904-stash-patch.sh b/t/t3904-stash-patch.sh\nindex ae313e3c70..0bddbce504 100755\n--- a/t/t3904-stash-patch.sh\n+++ b/t/t3904-stash-patch.sh\n@@ -107,4 +107,14 @@ test_expect_success 'stash -p with split hunk' '\n \t! grep \"added line 2\" test\n '\n \n+test_expect_success 'stash -p not confused by GIT_PAGER_IN_USE' '\n+\techo to-stash >test &&\n+\t# Set both GIT_PAGER_IN_USE and TERM. Our goal is entice any\n+\t# diff subprocesses into thinking that they could output\n+\t# color, even though their stdout is not going into a tty.\n+\techo y |\n+\tGIT_PAGER_IN_USE=1 TERM=vt100 git stash -p &&\n+\tgit diff --exit-code\n+'\n+\n test_done\n-- \n2.51.0.356.g99d8374de0\n\n"},{"id":"524606","messageId":"20250821071918.GB1839835@coredump.intra.peff.net","threadId":"63994","inReplyTo":"20250821070740.GA3356411@coredump.intra.peff.net","subject":"[PATCH 2/4] add-interactive: respect color.diff for diff coloring","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-08-21T07:19:18Z","receivedAt":"2025-08-21T07:19:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The old perl git-add--interactive.perl script used the color.diff config\noption to decide whether to color diffs (and if not set, it fell back to\nthe value of color.ui via git-config's --get-colorbool option). When we\nswitched to the builtin version, this was lost: we respect only\ncolor.ui. So for example:\n\n git -c color.diff=false add -p\n\nwould color the diff, even when it should not.\n\nThe culprit is this line in add-interactive.c's parse_diff():\n\n if (want_color_fd(1, -1))\n\nThat \"-1\" means \"no config has been set\", which causes it to fall back\nto the color.ui setting. We should instead be passing the value of\ncolor.diff. But the problem is that we never even parse that config\noption!\n\nInstead the builtin interactive code parses only the value of\ncolor.interactive, which is used for prompts and other messages. One\ncould perhaps argue that this should cover interactive diff coloring,\ntoo, but historically it did not. The perl script treated\ncolor.interactive and color.diff separately. So we should grab the\nvalues for both, keeping separate fields in our add_i_state variable,\nrather than a single use_color field.\n\nWe also load individual color slots (e.g., color.interactive.prompt),\nleaving them as the empty string when color is disabled. This happens\nvia the init_color() helper in add-interactive, which checks that\nuse_color field. Now that there are two such fields, we need to pass the\nappropriate one for each color.\n\nThe colors are mostly easy to divide up; color.interactive.* follows\ncolor.interactive, and color.diff.* follows color.diff. But the \"reset\"\ncolor is tricky. It is used for both types of coloring, but the two can\nbe configured independently. So we introduce two separate reset colors,\nand use each in the appropriate spot.\n\nThere are two new tests. The first enables interactive prompt colors but\ndisables color.diff. We should see a colored prompt but not a colored\ndiff, showing that we are now respecting color.diff (and not\ncolor.interactive or color.ui).\n\nThe second does the opposite. We disable color.interactive but turn on\ncolor.diff with a custom fragment color. When we split a hunk, the\ninteractive code has to re-color the hunk header, which lets us check\nthat we correctly loaded the color.diff.frag config based on color.diff,\nnot color.interactive.\n\nSigned-off-by: Jeff King \n---\n add-interactive.c | 79 ++++++++++++++++++++++----------------\n add-interactive.h | 7 +++-\n add-patch.c | 12 +++---\n t/t3701-add-interactive.sh | 36 +++++++++++++++++\n 4 files changed, 93 insertions(+), 41 deletions(-)\n\ndiff --git a/add-interactive.c b/add-interactive.c\nindex 3e692b47ec..95ab251963 100644\n--- a/add-interactive.c\n+++ b/add-interactive.c\n@@ -20,14 +20,14 @@\n #include \"prompt.h\"\n #include \"tree.h\"\n \n-static void init_color(struct repository *r, struct add_i_state *s,\n+static void init_color(struct repository *r, int use_color,\n \t\t const char *section_and_slot, char *dst,\n \t\t const char *default_color)\n {\n \tchar *key = xstrfmt(\"color.%s\", section_and_slot);\n \tconst char *value;\n \n-\tif (!s->use_color)\n+\tif (!use_color)\n \t\tdst[0] = '\\0';\n \telse if (repo_config_get_value(r, key, &value) ||\n \t\t color_parse(value, dst))\n@@ -36,42 +36,54 @@ static void init_color(struct repository *r, struct add_i_state *s,\n \tfree(key);\n }\n \n-void init_add_i_state(struct add_i_state *s, struct repository *r,\n-\t\t struct add_p_opt *add_p_opt)\n+static int check_color_config(struct repository *r, const char *var)\n {\n \tconst char *value;\n+\tint ret;\n+\n+\tif (repo_config_get_value(r, var, &value))\n+\t\tret = -1;\n+\telse\n+\t\tret = git_config_colorbool(var, value);\n+\treturn want_color(ret);\n+}\n \n+void init_add_i_state(struct add_i_state *s, struct repository *r,\n+\t\t struct add_p_opt *add_p_opt)\n+{\n \ts->r = r;\n \ts->context = -1;\n \ts->interhunkcontext = -1;\n \n-\tif (repo_config_get_value(r, \"color.interactive\", &value))\n-\t\ts->use_color = -1;\n-\telse\n-\t\ts->use_color =\n-\t\t\tgit_config_colorbool(\"color.interactive\", value);\n-\ts->use_color = want_color(s->use_color);\n-\n-\tinit_color(r, s, \"interactive.header\", s->header_color, GIT_COLOR_BOLD);\n-\tinit_color(r, s, \"interactive.help\", s->help_color, GIT_COLOR_BOLD_RED);\n-\tinit_color(r, s, \"interactive.prompt\", s->prompt_color,\n-\t\t GIT_COLOR_BOLD_BLUE);\n-\tinit_color(r, s, \"interactive.error\", s->error_color,\n-\t\t GIT_COLOR_BOLD_RED);\n-\n-\tinit_color(r, s, \"diff.frag\", s->fraginfo_color,\n-\t\t diff_get_color(s->use_color, DIFF_FRAGINFO));\n-\tinit_color(r, s, \"diff.context\", s->context_color, \"fall back\");\n+\ts->use_color_interactive = check_color_config(r, \"color.interactive\");\n+\n+\tinit_color(r, s->use_color_interactive, \"interactive.header\",\n+\t\t s->header_color, GIT_COLOR_BOLD);\n+\tinit_color(r, s->use_color_interactive, \"interactive.help\",\n+\t\t s->help_color, GIT_COLOR_BOLD_RED);\n+\tinit_color(r, s->use_color_interactive, \"interactive.prompt\",\n+\t\t s->prompt_color, GIT_COLOR_BOLD_BLUE);\n+\tinit_color(r, s->use_color_interactive, \"interactive.error\",\n+\t\t s->error_color, GIT_COLOR_BOLD_RED);\n+\tstrlcpy(s->reset_color_interactive,\n+\t\ts->use_color_interactive ? GIT_COLOR_RESET : \"\", COLOR_MAXLEN);\n+\n+\ts->use_color_diff = check_color_config(r, \"color.diff\");\n+\n+\tinit_color(r, s->use_color_diff, \"diff.frag\", s->fraginfo_color,\n+\t\t diff_get_color(s->use_color_diff, DIFF_FRAGINFO));\n+\tinit_color(r, s->use_color_diff, \"diff.context\", s->context_color,\n+\t\t \"fall back\");\n \tif (!strcmp(s->context_color, \"fall back\"))\n-\t\tinit_color(r, s, \"diff.plain\", s->context_color,\n-\t\t\t diff_get_color(s->use_color, DIFF_CONTEXT));\n-\tinit_color(r, s, \"diff.old\", s->file_old_color,\n-\t\tdiff_get_color(s->use_color, DIFF_FILE_OLD));\n-\tinit_color(r, s, \"diff.new\", s->file_new_color,\n-\t\tdiff_get_color(s->use_color, DIFF_FILE_NEW));\n-\n-\tstrlcpy(s->reset_color,\n-\t\ts->use_color ? GIT_COLOR_RESET : \"\", COLOR_MAXLEN);\n+\t\tinit_color(r, s->use_color_diff, \"diff.plain\",\n+\t\t\t s->context_color,\n+\t\t\t diff_get_color(s->use_color_diff, DIFF_CONTEXT));\n+\tinit_color(r, s->use_color_diff, \"diff.old\", s->file_old_color,\n+\t\tdiff_get_color(s->use_color_diff, DIFF_FILE_OLD));\n+\tinit_color(r, s->use_color_diff, \"diff.new\", s->file_new_color,\n+\t\tdiff_get_color(s->use_color_diff, DIFF_FILE_NEW));\n+\tstrlcpy(s->reset_color_diff,\n+\t\ts->use_color_diff ? GIT_COLOR_RESET : \"\", COLOR_MAXLEN);\n \n \tFREE_AND_NULL(s->interactive_diff_filter);\n \trepo_config_get_string(r, \"interactive.difffilter\",\n@@ -109,7 +121,8 @@ void clear_add_i_state(struct add_i_state *s)\n \tFREE_AND_NULL(s->interactive_diff_filter);\n \tFREE_AND_NULL(s->interactive_diff_algorithm);\n \tmemset(s, 0, sizeof(*s));\n-\ts->use_color = -1;\n+\ts->use_color_interactive = -1;\n+\ts->use_color_diff = -1;\n }\n \n /*\n@@ -1188,9 +1201,9 @@ int run_add_i(struct repository *r, const struct pathspec *ps,\n \t * When color was asked for, use the prompt color for\n \t * highlighting, otherwise use square brackets.\n \t */\n-\tif (s.use_color) {\n+\tif (s.use_color_interactive) {\n \t\tdata.color = s.prompt_color;\n-\t\tdata.reset = s.reset_color;\n+\t\tdata.reset = s.reset_color_interactive;\n \t}\n \tprint_file_item_data.color = data.color;\n \tprint_file_item_data.reset = data.reset;\ndiff --git a/add-interactive.h b/add-interactive.h\nindex 4213dcd67b..ceadfa6bb6 100644\n--- a/add-interactive.h\n+++ b/add-interactive.h\n@@ -12,16 +12,19 @@ struct add_p_opt {\n \n struct add_i_state {\n \tstruct repository *r;\n-\tint use_color;\n+\tint use_color_interactive;\n+\tint use_color_diff;\n \tchar header_color[COLOR_MAXLEN];\n \tchar help_color[COLOR_MAXLEN];\n \tchar prompt_color[COLOR_MAXLEN];\n \tchar error_color[COLOR_MAXLEN];\n-\tchar reset_color[COLOR_MAXLEN];\n+\tchar reset_color_interactive[COLOR_MAXLEN];\n+\n \tchar fraginfo_color[COLOR_MAXLEN];\n \tchar context_color[COLOR_MAXLEN];\n \tchar file_old_color[COLOR_MAXLEN];\n \tchar file_new_color[COLOR_MAXLEN];\n+\tchar reset_color_diff[COLOR_MAXLEN];\n \n \tint use_single_key;\n \tchar *interactive_diff_filter, *interactive_diff_algorithm;\ndiff --git a/add-patch.c b/add-patch.c\nindex 302e6ba7d9..b0389c5d5b 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -300,7 +300,7 @@ static void err(struct add_p_state *s, const char *fmt, ...)\n \tva_start(args, fmt);\n \tfputs(s->s.error_color, stdout);\n \tvprintf(fmt, args);\n-\tputs(s->s.reset_color);\n+\tputs(s->s.reset_color_interactive);\n \tva_end(args);\n }\n \n@@ -457,7 +457,7 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)\n \t}\n \tstrbuf_complete_line(plain);\n \n-\tif (want_color_fd(1, -1)) {\n+\tif (want_color_fd(1, s->s.use_color_diff)) {\n \t\tstruct child_process colored_cp = CHILD_PROCESS_INIT;\n \t\tconst char *diff_filter = s->s.interactive_diff_filter;\n \n@@ -714,7 +714,7 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk,\n \t\tif (len)\n \t\t\tstrbuf_add(out, p, len);\n \t\telse if (colored)\n-\t\t\tstrbuf_addf(out, \"%s\\n\", s->s.reset_color);\n+\t\t\tstrbuf_addf(out, \"%s\\n\", s->s.reset_color_diff);\n \t\telse\n \t\t\tstrbuf_addch(out, '\\n');\n \t}\n@@ -1107,7 +1107,7 @@ static void recolor_hunk(struct add_p_state *s, struct hunk *hunk)\n \t\t\t s->s.file_new_color :\n \t\t\t s->s.context_color);\n \t\tstrbuf_add(&s->colored, plain + current, eol - current);\n-\t\tstrbuf_addstr(&s->colored, s->s.reset_color);\n+\t\tstrbuf_addstr(&s->colored, s->s.reset_color_diff);\n \t\tif (next > eol)\n \t\t\tstrbuf_add(&s->colored, plain + eol, next - eol);\n \t\tcurrent = next;\n@@ -1528,8 +1528,8 @@ static int patch_update_file(struct add_p_state *s,\n \t\t\t\t\t\t: 1));\n \t\tprintf(_(s->mode->prompt_mode[prompt_mode_type]),\n \t\t s->buf.buf);\n-\t\tif (*s->s.reset_color)\n-\t\t\tfputs(s->s.reset_color, stdout);\n+\t\tif (*s->s.reset_color_interactive)\n+\t\t\tfputs(s->s.reset_color_interactive, stdout);\n \t\tfflush(stdout);\n \t\tif (read_single_character(s) == EOF)\n \t\t\tbreak;\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 04d2a19835..3f9cb9453f 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -866,6 +866,42 @@ test_expect_success 'colorized diffs respect diff.wsErrorHighlight' '\n \ttest_grep \"old<\" output\n '\n \n+test_expect_success 'diff color respects color.diff' '\n+\tgit reset --hard &&\n+\n+\techo old >test &&\n+\tgit add test &&\n+\techo new >test &&\n+\n+\tprintf n >n &&\n+\tforce_color git \\\n+\t\t-c color.interactive=auto \\\n+\t\t-c color.interactive.prompt=blue \\\n+\t\t-c color.diff=false \\\n+\t\t-c color.diff.old=red \\\n+\t\tadd -p >output.raw 2>&1 output &&\n+\ttest_grep \"BLUE.*Stage this hunk\" output &&\n+\ttest_grep ! \"RED\" output\n+'\n+\n+test_expect_success 're-coloring diff without color.interactive' '\n+\tgit reset --hard &&\n+\n+\ttest_write_lines 1 2 3 >test &&\n+\tgit add test &&\n+\ttest_write_lines one 2 three >test &&\n+\n+\ttest_write_lines s n n |\n+\tforce_color git \\\n+\t\t-c color.interactive=false \\\n+\t\t-c color.diff=true \\\n+\t\t-c color.diff.frag=\"bold magenta\" \\\n+\t\tadd -p >output.raw 2>&1 &&\n+\ttest_decode_color output &&\n+\ttest_grep \"@@\" output\n+'\n+\n test_expect_success 'diffFilter filters diff' '\n \tgit reset --hard &&\n \n-- \n2.51.0.356.g99d8374de0\n\n"},{"id":"524607","messageId":"20250821072224.GC1839835@coredump.intra.peff.net","threadId":"63994","inReplyTo":"20250821070740.GA3356411@coredump.intra.peff.net","subject":"[PATCH 3/4] add-interactive: manually fall back color config to color.ui","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-08-21T07:22:24Z","receivedAt":"2025-08-21T07:22:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Color options like color.interactive and color.diff should fall back to\nthe value of color.ui if they aren't set. In add-interactive, we check\nthe specific options (e.g., color.diff) via repo_config_get_value(),\nwhich does not depend on the main command having loaded any color config\nvia the git_config() callback mechanism.\n\nBut then we call want_color() on the result; if our specific config is\nunset then that function uses the value of git_use_color_default. That\nvariable is typically set from color.ui by the git_color_config()\ncallback, which is called by the main command in its own git_config()\ncallback function.\n\nThis works fine for \"add -p\", whose add_config() callback calls into\ngit_color_config(). But it doesn't work for other commands like\n\"checkout -p\", which is otherwise unaware of color at all. People tend\nnot to notice because the default is \"auto\", and that's what they'd set\ncolor.ui to as well. But something like:\n\n git -c color.ui=false checkout -p\n\nshould disable color, and it doesn't.\n\nThis regression goes back to 0527ccb1b5 (add -i: default to the built-in\nimplementation, 2021-11-30). In the perl version we got the color config\nfrom \"git config --get-colorbool\", which did the full lookup for us.\n\nThe obvious fix is for git-checkout to add a call to git_color_config()\nto its own config callback. But we'd have to do so for every command\nwith this problem, which is error-prone. Let's see if we can fix it more\ncentrally.\n\nIt is tempting to teach want_color() to look up the value of\nrepo_config_get_value(\"color.ui\") itself. But I think that would have\ndisastrous consequences. Plumbing commands, especially older ones, avoid\nporcelain config like color. by simply not parsing it in their config\ncallbacks. Looking up the value of color.ui under the hood would\nundermine that.\n\nInstead, let's do that lookup in the add-interactive setup code. We're\nalready demand-loading other color config there, which is probably fine\n(even in a plumbing command like \"git reset\", the interactive mode is\ninherently porcelain-ish). That catches all commands that use the\ninteractive code, whether they were calling git_color_config()\nthemselves or not.\n\nReported-by: Isaac Oscar Gariano \nSigned-off-by: Jeff King \n---\n add-interactive.c | 9 +++++++++\n t/t3701-add-interactive.sh | 15 +++++++++++++++\n 2 files changed, 24 insertions(+)\n\ndiff --git a/add-interactive.c b/add-interactive.c\nindex 95ab251963..db7e6a81a8 100644\n--- a/add-interactive.c\n+++ b/add-interactive.c\n@@ -45,6 +45,15 @@ static int check_color_config(struct repository *r, const char *var)\n \t\tret = -1;\n \telse\n \t\tret = git_config_colorbool(var, value);\n+\n+\t/*\n+\t * Do not rely on want_color() to fall back to color.ui for us. It uses\n+\t * the value parsed by git_color_config(), which may not have been\n+\t * called by the main command.\n+\t */\n+\tif (ret < 0 && !repo_config_get_value(r, \"color.ui\", &value))\n+\t\tret = git_config_colorbool(\"color.ui\", value);\n+\n \treturn want_color(ret);\n }\n \ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 3f9cb9453f..0024991257 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -1319,6 +1319,12 @@ test_expect_success 'stash accepts -U and --inter-hunk-context' '\n \ttest_grep \"@@ -2,20 +2,20 @@\" actual\n '\n \n+test_expect_success 'set up base for -p color tests' '\n+\techo commit >file &&\n+\tgit commit -am \"commit state\" &&\n+\tgit tag patch-base\n+'\n+\n for cmd in add checkout commit reset restore \"stash save\" \"stash push\"\n do\n \ttest_expect_success \"$cmd rejects invalid context options\" '\n@@ -1335,6 +1341,15 @@ do\n \t\ttest_must_fail git $cmd --inter-hunk-context 2 2>actual &&\n \t\ttest_grep -E \".--inter-hunk-context. requires .(--interactive/)?--patch.\" actual\n \t'\n+\n+\ttest_expect_success \"$cmd falls back to color.ui\" '\n+\t\tgit reset --hard patch-base &&\n+\t\techo working-tree >file &&\n+\t\ttest_write_lines y |\n+\t\tforce_color git -c color.ui=false $cmd -p >output.raw 2>&1 &&\n+\t\ttest_decode_color output &&\n+\t\ttest_cmp output.raw output\n+\t'\n done\n \n test_done\n-- \n2.51.0.356.g99d8374de0\n\n"},{"id":"524608","messageId":"20250821072249.GD1839835@coredump.intra.peff.net","threadId":"63994","inReplyTo":"20250821070740.GA3356411@coredump.intra.peff.net","subject":"[PATCH 4/4] contrib/diff-highlight: mention interactive.diffFilter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-08-21T07:22:49Z","receivedAt":"2025-08-21T07:22:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When the README for diff-highlight was written, there was no way to\ntrigger it for the `add -p` interactive patch mode. We've since grown a\nfeature to support that, but it was documented only on the Git side.\nLet's also let people coming the other direction, from diff-highlight,\nknow that it's an option.\n\nSuggested-by: Isaac Oscar Gariano \nSigned-off-by: Jeff King \n---\n contrib/diff-highlight/README | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/contrib/diff-highlight/README b/contrib/diff-highlight/README\nindex d4c2343175..1db4440e68 100644\n--- a/contrib/diff-highlight/README\n+++ b/contrib/diff-highlight/README\n@@ -58,6 +58,14 @@ following in your git configuration:\n \tdiff = diff-highlight | less\n ---------------------------------------------\n \n+If you use the interactive patch mode of `git add -p`, `git checkout\n+-p`, etc, you may also want to configure it to be used there:\n+\n+---------------------------------------------\n+[interactive]\n+ diffFilter = diff-highlight\n+---------------------------------------------\n+\n \n Color Config\n ------------\n-- \n2.51.0.356.g99d8374de0\n"},{"id":"524661","messageId":"xmqqh5y04r6u.fsf@gitster.g","threadId":"63994","inReplyTo":"20250821072224.GC1839835@coredump.intra.peff.net","subject":"Re: [PATCH 3/4] add-interactive: manually fall back color config to color.ui","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-21T15:42:17Z","receivedAt":"2025-08-21T15:42:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Instead, let's do that lookup in the add-interactive setup code. We're\n> already demand-loading other color config there, which is probably fine\n> (even in a plumbing command like \"git reset\", the interactive mode is\n> inherently porcelain-ish). That catches all commands that use the\n> interactive code, whether they were calling git_color_config()\n> themselves or not.\n\nA very good design decision I can agree with.\nNice.\n"},{"id":"525402","messageId":"aLfs5EDk-krJHnmQ@pks.im","threadId":"63994","inReplyTo":"20250821071517.GA1839835@coredump.intra.peff.net","subject":"Re: [PATCH 1/4] stash: pass --no-color to diff-tree child processes","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-09-03T07:23:16Z","receivedAt":"2025-09-03T07:23:28Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Aug 21, 2025 at 03:15:17AM -0400, Jeff King wrote:\n[snip]\n> Reading that referenced thread again, Junio was in favor of reverting\n> 4c7f1819b3 and replacing it with something that didn't kick in for\n> plumbing (thus fixing the root issue). I argued against it somewhat\n> there, but now I think I was foolish and agree with 2017-Junio. ;) I do\n> think that fixing it now carries some risk of people complaining,\n> though. So I'd rather do this immediate fix and worry about the larger\n> problem separately.\n\nFair. I'm also in the camp of that git-diff-tree(1) shouldn't ever\nproduce color unless explicitly asked. After all it's part of our\nplumbing layer, so it's basically expected to be used mostly for scripts\nand not for users.\n\n> diff --git a/builtin/stash.c b/builtin/stash.c\n> index 1977e50df2..c55628aafc 100644\n> --- a/builtin/stash.c\n> +++ b/builtin/stash.c\n> @@ -377,7 +377,7 @@ static int diff_tree_binary(struct strbuf *out, struct object_id *w_commit)\n> \t * however it should be done together with apply_cached.\n> \t */\n> \tcp.git_cmd = 1;\n> -\tstrvec_pushl(&cp.args, \"diff-tree\", \"--binary\", NULL);\n> +\tstrvec_pushl(&cp.args, \"diff-tree\", \"--binary\", \"--no-color\", NULL);\n> \tstrvec_pushf(&cp.args, \"%s^2^..%s^2\", w_commit_hex, w_commit_hex);\n> \n> \treturn pipe_command(&cp, NULL, 0, out, 0, NULL, 0);\n> @@ -1283,6 +1283,7 @@ static int stash_staged(struct stash_info *info, struct strbuf *out_patch,\n> \n> \tcp_diff_tree.git_cmd = 1;\n> \tstrvec_pushl(&cp_diff_tree.args, \"diff-tree\", \"-p\", \"--binary\",\n> +\t\t \"--no-color\",\n> \t\t \"-U1\", \"HEAD\", oid_to_hex(&info->w_tree), \"--\", NULL);\n> \tif (pipe_command(&cp_diff_tree, NULL, 0, out_patch, 0, NULL, 0)) {\n> \t\tret = -1;\n\nThe line-wrapping is a bit funny, but I don't mind that too much.\n\n> @@ -1345,6 +1346,7 @@ static int stash_patch(struct stash_info *info, const struct pathspec *ps,\n> \n> \tcp_diff_tree.git_cmd = 1;\n> \tstrvec_pushl(&cp_diff_tree.args, \"diff-tree\", \"-p\", \"-U1\", \"HEAD\",\n> +\t\t \"--no-color\",\n> \t\t oid_to_hex(&info->w_tree), \"--\", NULL);\n> \tif (pipe_command(&cp_diff_tree, NULL, 0, out_patch, 0, NULL, 0)) {\n> \t\tret = -1;\n\nAll of these make sense. It feels a bit like whack-a-mole, and fixing\nthe root cause would address that. But I also understand that you shy\naway from addressing it due to the high chance for regressions.\n\nThere's also a call to \"diff-index\" in the same file. Do we also need to\nadjust that instance?\n\n> diff --git a/t/t3904-stash-patch.sh b/t/t3904-stash-patch.sh\n> index ae313e3c70..0bddbce504 100755\n> --- a/t/t3904-stash-patch.sh\n> +++ b/t/t3904-stash-patch.sh\n> @@ -107,4 +107,14 @@ test_expect_success 'stash -p with split hunk' '\n> \t! grep \"added line 2\" test\n> '\n> \n> +test_expect_success 'stash -p not confused by GIT_PAGER_IN_USE' '\n> +\techo to-stash >test &&\n> +\t# Set both GIT_PAGER_IN_USE and TERM. Our goal is entice any\n\ns/is/& to/\n\n> +\t# diff subprocesses into thinking that they could output\n> +\t# color, even though their stdout is not going into a tty.\n> +\techo y |\n> +\tGIT_PAGER_IN_USE=1 TERM=vt100 git stash -p &&\n> +\tgit diff --exit-code\n> +'\n> +\n> test_done\n\nPatrick\n"},{"id":"525403","messageId":"aLfs7wuFpMhg8fK_@pks.im","threadId":"63994","inReplyTo":"20250821071918.GB1839835@coredump.intra.peff.net","subject":"Re: [PATCH 2/4] add-interactive: respect color.diff for diff coloring","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-09-03T07:23:27Z","receivedAt":"2025-09-03T07:23:33Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Aug 21, 2025 at 03:19:18AM -0400, Jeff King wrote:\n> diff --git a/add-interactive.c b/add-interactive.c\n> index 3e692b47ec..95ab251963 100644\n> --- a/add-interactive.c\n> +++ b/add-interactive.c\n> @@ -20,14 +20,14 @@\n> #include \"prompt.h\"\n> #include \"tree.h\"\n> \n> -static void init_color(struct repository *r, struct add_i_state *s,\n> +static void init_color(struct repository *r, int use_color,\n> \t\t const char *section_and_slot, char *dst,\n> \t\t const char *default_color)\n> {\n> \tchar *key = xstrfmt(\"color.%s\", section_and_slot);\n> \tconst char *value;\n> \n> -\tif (!s->use_color)\n> +\tif (!use_color)\n> \t\tdst[0] = '\\0';\n> \telse if (repo_config_get_value(r, key, &value) ||\n> \t\t color_parse(value, dst))\n\nOkay. This needs to change so that we can pass in either\n`use_color_diff` or `use_color_interactive`.\n\n> @@ -36,42 +36,54 @@ static void init_color(struct repository *r, struct add_i_state *s,\n> \tfree(key);\n> }\n> \n> -void init_add_i_state(struct add_i_state *s, struct repository *r,\n> -\t\t struct add_p_opt *add_p_opt)\n> +static int check_color_config(struct repository *r, const char *var)\n> {\n> \tconst char *value;\n> +\tint ret;\n> +\n> +\tif (repo_config_get_value(r, var, &value))\n> +\t\tret = -1;\n\nNot an old issue, but should we use `GIT_COLOR_UNKNOWN` here?\n\n> @@ -109,7 +121,8 @@ void clear_add_i_state(struct add_i_state *s)\n> \tFREE_AND_NULL(s->interactive_diff_filter);\n> \tFREE_AND_NULL(s->interactive_diff_algorithm);\n> \tmemset(s, 0, sizeof(*s));\n> -\ts->use_color = -1;\n> +\ts->use_color_interactive = -1;\n> +\ts->use_color_diff = -1;\n> }\n> \n> /*\n\nSame here, should we use `GIT_COLOR_UNKNOWN` to initialize these fields?\nIt would be even better if the `GIT_COLOR` values were a proper enum so\nthat we can use the type in both `want_color_fd()` and for these struct\nmembers.\n\n> @@ -1188,9 +1201,9 @@ int run_add_i(struct repository *r, const struct pathspec *ps,\n> \t * When color was asked for, use the prompt color for\n> \t * highlighting, otherwise use square brackets.\n> \t */\n> -\tif (s.use_color) {\n> +\tif (s.use_color_interactive) {\n> \t\tdata.color = s.prompt_color;\n> -\t\tdata.reset = s.reset_color;\n> +\t\tdata.reset = s.reset_color_interactive;\n> \t}\n> \tprint_file_item_data.color = data.color;\n> \tprint_file_item_data.reset = data.reset;\n\nMakes sense. We don't want to show the diff here, but render the prompt,\nwhich should of course honor the interactive colors.\n\n> diff --git a/add-patch.c b/add-patch.c\n> index 302e6ba7d9..b0389c5d5b 100644\n> --- a/add-patch.c\n> +++ b/add-patch.c\n> @@ -300,7 +300,7 @@ static void err(struct add_p_state *s, const char *fmt, ...)\n> \tva_start(args, fmt);\n> \tfputs(s->s.error_color, stdout);\n> \tvprintf(fmt, args);\n> -\tputs(s->s.reset_color);\n> +\tputs(s->s.reset_color_interactive);\n> \tva_end(args);\n> }\n\nYup, printing an error message should respect interactive colors.\n\n> @@ -457,7 +457,7 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)\n> \t}\n> \tstrbuf_complete_line(plain);\n> \n> -\tif (want_color_fd(1, -1)) {\n> +\tif (want_color_fd(1, s->s.use_color_diff)) {\n> \t\tstruct child_process colored_cp = CHILD_PROCESS_INIT;\n> \t\tconst char *diff_filter = s->s.interactive_diff_filter;\n> \n\nWe're printing the diff here, and this change is the whole point of this\ncommit as far as I understand as we now properly respect configured diff\ncolors.\n\n> @@ -714,7 +714,7 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk,\n> \t\tif (len)\n> \t\t\tstrbuf_add(out, p, len);\n> \t\telse if (colored)\n> -\t\t\tstrbuf_addf(out, \"%s\\n\", s->s.reset_color);\n> +\t\t\tstrbuf_addf(out, \"%s\\n\", s->s.reset_color_diff);\n> \t\telse\n> \t\t\tstrbuf_addch(out, '\\n');\n> \t}\n> @@ -1107,7 +1107,7 @@ static void recolor_hunk(struct add_p_state *s, struct hunk *hunk)\n> \t\t\t s->s.file_new_color :\n> \t\t\t s->s.context_color);\n> \t\tstrbuf_add(&s->colored, plain + current, eol - current);\n> -\t\tstrbuf_addstr(&s->colored, s->s.reset_color);\n> +\t\tstrbuf_addstr(&s->colored, s->s.reset_color_diff);\n> \t\tif (next > eol)\n> \t\t\tstrbuf_add(&s->colored, plain + eol, next - eol);\n> \t\tcurrent = next;\n\nBoth of these print diff hunks, which should use diff colors.\n\n> @@ -1528,8 +1528,8 @@ static int patch_update_file(struct add_p_state *s,\n> \t\t\t\t\t\t: 1));\n> \t\tprintf(_(s->mode->prompt_mode[prompt_mode_type]),\n> \t\t s->buf.buf);\n> -\t\tif (*s->s.reset_color)\n> -\t\t\tfputs(s->s.reset_color, stdout);\n> +\t\tif (*s->s.reset_color_interactive)\n> +\t\t\tfputs(s->s.reset_color_interactive, stdout);\n> \t\tfflush(stdout);\n> \t\tif (read_single_character(s) == EOF)\n> \t\t\tbreak;\n\nAnd here we reset colors after the prompt. So all of these conversions\nlook good.\n\n> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\n> index 04d2a19835..3f9cb9453f 100755\n> --- a/t/t3701-add-interactive.sh\n> +++ b/t/t3701-add-interactive.sh\n> @@ -866,6 +866,42 @@ test_expect_success 'colorized diffs respect diff.wsErrorHighlight' '\n> \ttest_grep \"old<\" output\n> '\n> \n> +test_expect_success 'diff color respects color.diff' '\n> +\tgit reset --hard &&\n> +\n> +\techo old >test &&\n> +\tgit add test &&\n> +\techo new >test &&\n> +\n> +\tprintf n >n &&\n> +\tforce_color git \\\n> +\t\t-c color.interactive=auto \\\n> +\t\t-c color.interactive.prompt=blue \\\n> +\t\t-c color.diff=false \\\n> +\t\t-c color.diff.old=red \\\n> +\t\tadd -p >output.raw 2>&1 +\ttest_decode_color output &&\n> +\ttest_grep \"BLUE.*Stage this hunk\" output &&\n> +\ttest_grep ! \"RED\" output\n> +'\n> +\n> +test_expect_success 're-coloring diff without color.interactive' '\n> +\tgit reset --hard &&\n> +\n> +\ttest_write_lines 1 2 3 >test &&\n> +\tgit add test &&\n> +\ttest_write_lines one 2 three >test &&\n> +\n> +\ttest_write_lines s n n |\n> +\tforce_color git \\\n> +\t\t-c color.interactive=false \\\n> +\t\t-c color.diff=true \\\n> +\t\t-c color.diff.frag=\"bold magenta\" \\\n> +\t\tadd -p >output.raw 2>&1 &&\n> +\ttest_decode_color output &&\n> +\ttest_grep \"@@\" output\n> +'\n> +\n\nShould we also verify that the interactive prompts aren't colored here?\n\nPatrick\n"},{"id":"525404","messageId":"aLfs9ZbAxHnsqluw@pks.im","threadId":"63994","inReplyTo":"20250821072224.GC1839835@coredump.intra.peff.net","subject":"Re: [PATCH 3/4] add-interactive: manually fall back color config to color.ui","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-09-03T07:23:33Z","receivedAt":"2025-09-03T07:23:39Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Aug 21, 2025 at 03:22:24AM -0400, Jeff King wrote:\n> Color options like color.interactive and color.diff should fall back to\n> the value of color.ui if they aren't set. In add-interactive, we check\n> the specific options (e.g., color.diff) via repo_config_get_value(),\n> which does not depend on the main command having loaded any color config\n> via the git_config() callback mechanism.\n> \n> But then we call want_color() on the result; if our specific config is\n> unset then that function uses the value of git_use_color_default. That\n> variable is typically set from color.ui by the git_color_config()\n> callback, which is called by the main command in its own git_config()\n> callback function.\n> \n> This works fine for \"add -p\", whose add_config() callback calls into\n> git_color_config(). But it doesn't work for other commands like\n> \"checkout -p\", which is otherwise unaware of color at all. People tend\n> not to notice because the default is \"auto\", and that's what they'd set\n> color.ui to as well. But something like:\n> \n> git -c color.ui=false checkout -p\n> \n> should disable color, and it doesn't.\n> \n> This regression goes back to 0527ccb1b5 (add -i: default to the built-in\n> implementation, 2021-11-30). In the perl version we got the color config\n> from \"git config --get-colorbool\", which did the full lookup for us.\n> \n> The obvious fix is for git-checkout to add a call to git_color_config()\n> to its own config callback. But we'd have to do so for every command\n> with this problem, which is error-prone. Let's see if we can fix it more\n> centrally.\n> \n> It is tempting to teach want_color() to look up the value of\n> repo_config_get_value(\"color.ui\") itself. But I think that would have\n> disastrous consequences. Plumbing commands, especially older ones, avoid\n> porcelain config like color. by simply not parsing it in their config\n\nIs the \"color.\" intended to refer to config keys starting with that\nstring? If so it would help to quote it and maybe say \"color.*\".\n\n> diff --git a/add-interactive.c b/add-interactive.c\n> index 95ab251963..db7e6a81a8 100644\n> --- a/add-interactive.c\n> +++ b/add-interactive.c\n> @@ -45,6 +45,15 @@ static int check_color_config(struct repository *r, const char *var)\n> \t\tret = -1;\n> \telse\n> \t\tret = git_config_colorbool(var, value);\n> +\n> +\t/*\n> +\t * Do not rely on want_color() to fall back to color.ui for us. It uses\n> +\t * the value parsed by git_color_config(), which may not have been\n> +\t * called by the main command.\n> +\t */\n> +\tif (ret < 0 && !repo_config_get_value(r, \"color.ui\", &value))\n> +\t\tret = git_config_colorbool(\"color.ui\", value);\n\nWith the previous comments where I say that we should use\n`GIT_COLOR_UNKNOWN` we should probably convert the `ret < 0` to `ret ==\nGIT_COLOR_UNKNOWN`, as well.\n\n> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\n> index 3f9cb9453f..0024991257 100755\n> --- a/t/t3701-add-interactive.sh\n> +++ b/t/t3701-add-interactive.sh\n> @@ -1335,6 +1341,15 @@ do\n> \t\ttest_must_fail git $cmd --inter-hunk-context 2 2>actual &&\n> \t\ttest_grep -E \".--inter-hunk-context. requires .(--interactive/)?--patch.\" actual\n> \t'\n> +\n> +\ttest_expect_success \"$cmd falls back to color.ui\" '\n> +\t\tgit reset --hard patch-base &&\n> +\t\techo working-tree >file &&\n> +\t\ttest_write_lines y |\n> +\t\tforce_color git -c color.ui=false $cmd -p >output.raw 2>&1 &&\n> +\t\ttest_decode_color output &&\n> +\t\ttest_cmp output.raw output\n> +\t'\n\nNice and straight-forward.\n\nPatrick\n"},{"id":"525849","messageId":"20250908160628.GB1308482@coredump.intra.peff.net","threadId":"63994","inReplyTo":"aLfs5EDk-krJHnmQ@pks.im","subject":"Re: [PATCH 1/4] stash: pass --no-color to diff-tree child processes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-09-08T16:06:28Z","receivedAt":"2025-09-08T16:06:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 03, 2025 at 09:23:16AM +0200, Patrick Steinhardt wrote:\n\n> > @@ -1345,6 +1346,7 @@ static int stash_patch(struct stash_info *info, const struct pathspec *ps,\n> > \n> > \tcp_diff_tree.git_cmd = 1;\n> > \tstrvec_pushl(&cp_diff_tree.args, \"diff-tree\", \"-p\", \"-U1\", \"HEAD\",\n> > +\t\t \"--no-color\",\n> > \t\t oid_to_hex(&info->w_tree), \"--\", NULL);\n> > \tif (pipe_command(&cp_diff_tree, NULL, 0, out_patch, 0, NULL, 0)) {\n> > \t\tret = -1;\n> \n> All of these make sense. It feels a bit like whack-a-mole, and fixing\n> the root cause would address that. But I also understand that you shy\n> away from addressing it due to the high chance for regressions.\n> \n> There's also a call to \"diff-index\" in the same file. Do we also need to\n> adjust that instance?\n\nI was focused on the \"-p\" code path, but yes, I think it fails when\nwe stash an index change with GIT_PAGER_IN_USE=1. I've fixed it and\nadded a new test in my re-roll.\n\n-Peff\n"},{"id":"525852","messageId":"20250908161648.GC1308482@coredump.intra.peff.net","threadId":"63994","inReplyTo":"aLfs7wuFpMhg8fK_@pks.im","subject":"Re: [PATCH 2/4] add-interactive: respect color.diff for diff coloring","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-09-08T16:16:48Z","receivedAt":"2025-09-08T16:16:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 03, 2025 at 09:23:27AM +0200, Patrick Steinhardt wrote:\n\n> > +static int check_color_config(struct repository *r, const char *var)\n> > {\n> > \tconst char *value;\n> > +\tint ret;\n> > +\n> > +\tif (repo_config_get_value(r, var, &value))\n> > +\t\tret = -1;\n> \n> Not an old issue, but should we use `GIT_COLOR_UNKNOWN` here?\n\nMy initial reaction was: yeah, we could probably fix this up in a\npreparatory patch. But the problem is much deeper than the\nadd-interactive code. Nobody uses GIT_COLOR_UNKNOWN at all! Even\ngit_config_colorbool() just returns -1.\n\nMoreover, it does not even use the ALWAYS/NEVER defines, but just 1 and\n0. Making things even more complicated, we sometimes want to consider\n\"do we want color\" as this always/never/auto/unknown set, and then\nsometimes we collapse that (using the same variable!) into a single\ntrue/false value.\n\nSo using that consistently and possibly switching to an enum is a much\nbigger topic. It may be worth cleaning up, but I don't think it's worth\nderailing this regression fix. In the meantime, I'd rather keep this\ncode matching the rest of the color code (it's not even really adding\nnew instances of \"-1\", but just shuffling them around).\n\n> > -\tif (want_color_fd(1, -1)) {\n> > +\tif (want_color_fd(1, s->s.use_color_diff)) {\n> > \t\tstruct child_process colored_cp = CHILD_PROCESS_INIT;\n> > \t\tconst char *diff_filter = s->s.interactive_diff_filter;\n> > \n> \n> We're printing the diff here, and this change is the whole point of this\n> commit as far as I understand as we now properly respect configured diff\n> colors.\n\nYes. I would have liked to split it up more to make this hunk stand out,\nbut there's some chicken-and-egg dependencies.\n\n> > +test_expect_success 're-coloring diff without color.interactive' '\n> > +\tgit reset --hard &&\n> > +\n> > +\ttest_write_lines 1 2 3 >test &&\n> > +\tgit add test &&\n> > +\ttest_write_lines one 2 three >test &&\n> > +\n> > +\ttest_write_lines s n n |\n> > +\tforce_color git \\\n> > +\t\t-c color.interactive=false \\\n> > +\t\t-c color.diff=true \\\n> > +\t\t-c color.diff.frag=\"bold magenta\" \\\n> > +\t\tadd -p >output.raw 2>&1 &&\n> > +\ttest_decode_color output &&\n> > +\ttest_grep \"@@\" output\n> > +'\n> > +\n> \n> Should we also verify that the interactive prompts aren't colored here?\n\nSeems reasonable. Knowing that the patch is splitting the diff coloring\noff of the interactive, it would be pretty hard to introduce such a bug.\nBut from a black box perspective, that is probably a good thing to test.\n\nUltimately the best test would be for every item that _could_ be colored\nby each type to be individually checked in each scenario. But I didn't\nwant the test to depend on enumerating those very specific details of\nthe code.\n\n-Peff\n"},{"id":"525853","messageId":"20250908161747.GD1308482@coredump.intra.peff.net","threadId":"63994","inReplyTo":"aLfs9ZbAxHnsqluw@pks.im","subject":"Re: [PATCH 3/4] add-interactive: manually fall back color config to color.ui","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-09-08T16:17:47Z","receivedAt":"2025-09-08T16:17:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 03, 2025 at 09:23:33AM +0200, Patrick Steinhardt wrote:\n\n> > It is tempting to teach want_color() to look up the value of\n> > repo_config_get_value(\"color.ui\") itself. But I think that would have\n> > disastrous consequences. Plumbing commands, especially older ones, avoid\n> > porcelain config like color. by simply not parsing it in their config\n> \n> Is the \"color.\" intended to refer to config keys starting with that\n> string? If so it would help to quote it and maybe say \"color.*\".\n\nI'm not sure if I meant \"config like color\" in the general sense, or\ntypo'd \"color.*\". Either way, what is there is indeed confusing. ;) I'll\nfix it.\n\n-Peff\n"},{"id":"525855","messageId":"20250908164157.GA1323487@coredump.intra.peff.net","threadId":"63994","inReplyTo":"20250821070740.GA3356411@coredump.intra.peff.net","subject":"[PATCH v2 0/4] oddities around add-interactive and color","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-09-08T16:41:57Z","receivedAt":"2025-09-08T16:41:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 21, 2025 at 03:07:40AM -0400, Jeff King wrote:\n\n> So here's a series which I think addresses everything I found. These\n> bugs have been lurking for a while, but I guess not many people tend to\n> set color variables to anything exotic.\n\nAnd here's a v2 based on Patrick's review. I also touched up a few lines\nwhose indentation did not pass clang-format (not new, but ones I was\ntouching or moving around). The only thing I punted on was refactoring\nthe GIT_COLOR_* defines, as I think it extends well beyond the code I'm\ntouching here (see the reply I left in the thread).\n\n-Peff\n\n [1/4]: stash: pass --no-color to diff plumbing child processes\n [2/4]: add-interactive: respect color.diff for diff coloring\n [3/4]: add-interactive: manually fall back color config to color.ui\n [4/4]: contrib/diff-highlight: mention interactive.diffFilter\n\n add-interactive.c | 88 ++++++++++++++++++++++-------------\n add-interactive.h | 7 ++-\n add-patch.c | 12 ++---\n builtin/stash.c | 5 +-\n contrib/diff-highlight/README | 8 ++++\n t/t3701-add-interactive.sh | 53 +++++++++++++++++++++\n t/t3904-stash-patch.sh | 19 ++++++++\n 7 files changed, 150 insertions(+), 42 deletions(-)\n\n1: d1d3c0e7f4 ! 1: d02117a0d6 stash: pass --no-color to diff-tree child processes\n @@ Metadata\n Author: Jeff King \n \n ## Commit message ##\n - stash: pass --no-color to diff-tree child processes\n + stash: pass --no-color to diff plumbing child processes\n \n After a partial stash, we may clear out the working tree by capturing\n - the output of diff-tree and piping it into git-apply. So we most\n - definitely do not want color diff output from that diff-tree process.\n - And it normally would not produce any, since its stdout is not going to\n - a tty, and the default value of color.ui is \"auto\".\n + the output of diff-tree and piping it into git-apply (and likewise we\n + may use diff-index to restore the index). So we most definitely do not\n + want color diff output from that diff-tree process. And it normally\n + would not produce any, since its stdout is not going to a tty, and the\n + default value of color.ui is \"auto\".\n \n However, if GIT_PAGER_IN_USE is set in the environment, that overrides\n the tty check, and we'll produce a colorized diff that chokes git-apply:\n @@ builtin/stash.c: static int stash_patch(struct stash_info *info, const struct pa\n \t\t oid_to_hex(&info->w_tree), \"--\", NULL);\n \tif (pipe_command(&cp_diff_tree, NULL, 0, out_patch, 0, NULL, 0)) {\n \t\tret = -1;\n +@@ builtin/stash.c: static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q\n + \n + \t\t\tcp_diff.git_cmd = 1;\n + \t\t\tstrvec_pushl(&cp_diff.args, \"diff-index\", \"-p\",\n ++\t\t\t\t \"--no-color\",\n + \t\t\t\t \"--cached\", \"--binary\", \"HEAD\", \"--\",\n + \t\t\t\t NULL);\n + \t\t\tadd_pathspecs(&cp_diff.args, ps);\n \n ## t/t3904-stash-patch.sh ##\n @@ t/t3904-stash-patch.sh: test_expect_success 'stash -p with split hunk' '\n @@ t/t3904-stash-patch.sh: test_expect_success 'stash -p with split hunk' '\n \n +test_expect_success 'stash -p not confused by GIT_PAGER_IN_USE' '\n +\techo to-stash >test &&\n -+\t# Set both GIT_PAGER_IN_USE and TERM. Our goal is entice any\n ++\t# Set both GIT_PAGER_IN_USE and TERM. Our goal is to entice any\n +\t# diff subprocesses into thinking that they could output\n +\t# color, even though their stdout is not going into a tty.\n +\techo y |\n +\tGIT_PAGER_IN_USE=1 TERM=vt100 git stash -p &&\n +\tgit diff --exit-code\n +'\n ++\n ++test_expect_success 'index push not confused by GIT_PAGER_IN_USE' '\n ++\techo index >test &&\n ++\tgit add test &&\n ++\techo working-tree >test &&\n ++\t# As above, we try to entice the child diff into using color.\n ++\tGIT_PAGER_IN_USE=1 TERM=vt100 git stash push test &&\n ++\tgit diff --exit-code\n ++'\n +\n test_done\n2: 5d40a0ed74 ! 2: f2600751b9 add-interactive: respect color.diff for diff coloring\n @@ add-interactive.c: static void init_color(struct repository *r, struct add_i_sta\n +\t\t\t s->context_color,\n +\t\t\t diff_get_color(s->use_color_diff, DIFF_CONTEXT));\n +\tinit_color(r, s->use_color_diff, \"diff.old\", s->file_old_color,\n -+\t\tdiff_get_color(s->use_color_diff, DIFF_FILE_OLD));\n ++\t\t diff_get_color(s->use_color_diff, DIFF_FILE_OLD));\n +\tinit_color(r, s->use_color_diff, \"diff.new\", s->file_new_color,\n -+\t\tdiff_get_color(s->use_color_diff, DIFF_FILE_NEW));\n ++\t\t diff_get_color(s->use_color_diff, DIFF_FILE_NEW));\n +\tstrlcpy(s->reset_color_diff,\n +\t\ts->use_color_diff ? GIT_COLOR_RESET : \"\", COLOR_MAXLEN);\n \n @@ t/t3701-add-interactive.sh: test_expect_success 'colorized diffs respect diff.ws\n +\ttest_write_lines s n n |\n +\tforce_color git \\\n +\t\t-c color.interactive=false \\\n ++\t\t-c color.interactive.prompt=blue \\\n +\t\t-c color.diff=true \\\n +\t\t-c color.diff.frag=\"bold magenta\" \\\n +\t\tadd -p >output.raw 2>&1 &&\n +\ttest_decode_color output &&\n -+\ttest_grep \"@@\" output\n ++\ttest_grep \"@@\" output &&\n ++\ttest_grep ! \"BLUE\" output\n +'\n +\n test_expect_success 'diffFilter filters diff' '\n3: 44cb772e07 ! 3: 8979bff0c5 add-interactive: manually fall back color config to color.ui\n @@ Commit message\n It is tempting to teach want_color() to look up the value of\n repo_config_get_value(\"color.ui\") itself. But I think that would have\n disastrous consequences. Plumbing commands, especially older ones, avoid\n - porcelain config like color. by simply not parsing it in their config\n + porcelain config like \"color.*\" by simply not parsing it in their config\n callbacks. Looking up the value of color.ui under the hood would\n undermine that.\n \n4: 31c0a6f81e = 4: a2b328389a contrib/diff-highlight: mention interactive.diffFilter\n"},{"id":"525856","messageId":"20250908164232.GA1323964@coredump.intra.peff.net","threadId":"63994","inReplyTo":"20250908164157.GA1323487@coredump.intra.peff.net","subject":"[PATCH v2 1/4] stash: pass --no-color to diff plumbing child processes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-09-08T16:42:32Z","receivedAt":"2025-09-08T16:42:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"After a partial stash, we may clear out the working tree by capturing\nthe output of diff-tree and piping it into git-apply (and likewise we\nmay use diff-index to restore the index). So we most definitely do not\nwant color diff output from that diff-tree process. And it normally\nwould not produce any, since its stdout is not going to a tty, and the\ndefault value of color.ui is \"auto\".\n\nHowever, if GIT_PAGER_IN_USE is set in the environment, that overrides\nthe tty check, and we'll produce a colorized diff that chokes git-apply:\n\n $ echo y | GIT_PAGER_IN_USE=1 git stash -p\n [...]\n Saved working directory and index state WIP on main: 4f2e2bb foo\n error: No valid patches in input (allow with \"--allow-empty\")\n Cannot remove worktree changes\n\nSetting this variable is a relatively silly thing to do, and not\nsomething most users would run into. But we sometimes do it in our tests\nto stimulate color. And it is a user-visible bug, so let's fix it rather\nthan work around it in the tests.\n\nThe root issue here is that diff-tree (and other diff plumbing) should\nprobably not ever produce color by default. It does so not by parsing\ncolor.ui, but because of the baked-in \"auto\" default from 4c7f1819b3\n(make color.ui default to 'auto', 2013-06-10). But changing that is\nrisky; we've had discussions back and forth on the topic over the years.\nE.g.:\n\n https://lore.kernel.org/git/86D0A377-8AFD-460D-A90E-6327C6934DFC@gmail.com/.\n\nSo let's accept that as the status quo for now and protect ourselves by\npassing --no-color to the child processes. This is the same thing we did\nfor add-interactive itself in 1c6ffb546b (add--interactive.perl: specify\n--no-color explicitly, 2020-09-07).\n\nSigned-off-by: Jeff King \n---\n builtin/stash.c | 5 ++++-\n t/t3904-stash-patch.sh | 19 +++++++++++++++++++\n 2 files changed, 23 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex f5ddee5c7f..67b291f3fd 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -377,7 +377,7 @@ static int diff_tree_binary(struct strbuf *out, struct object_id *w_commit)\n \t * however it should be done together with apply_cached.\n \t */\n \tcp.git_cmd = 1;\n-\tstrvec_pushl(&cp.args, \"diff-tree\", \"--binary\", NULL);\n+\tstrvec_pushl(&cp.args, \"diff-tree\", \"--binary\", \"--no-color\", NULL);\n \tstrvec_pushf(&cp.args, \"%s^2^..%s^2\", w_commit_hex, w_commit_hex);\n \n \treturn pipe_command(&cp, NULL, 0, out, 0, NULL, 0);\n@@ -1284,6 +1284,7 @@ static int stash_staged(struct stash_info *info, struct strbuf *out_patch,\n \n \tcp_diff_tree.git_cmd = 1;\n \tstrvec_pushl(&cp_diff_tree.args, \"diff-tree\", \"-p\", \"--binary\",\n+\t\t \"--no-color\",\n \t\t \"-U1\", \"HEAD\", oid_to_hex(&info->w_tree), \"--\", NULL);\n \tif (pipe_command(&cp_diff_tree, NULL, 0, out_patch, 0, NULL, 0)) {\n \t\tret = -1;\n@@ -1346,6 +1347,7 @@ static int stash_patch(struct stash_info *info, const struct pathspec *ps,\n \n \tcp_diff_tree.git_cmd = 1;\n \tstrvec_pushl(&cp_diff_tree.args, \"diff-tree\", \"-p\", \"-U1\", \"HEAD\",\n+\t\t \"--no-color\",\n \t\t oid_to_hex(&info->w_tree), \"--\", NULL);\n \tif (pipe_command(&cp_diff_tree, NULL, 0, out_patch, 0, NULL, 0)) {\n \t\tret = -1;\n@@ -1720,6 +1722,7 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q\n \n \t\t\tcp_diff.git_cmd = 1;\n \t\t\tstrvec_pushl(&cp_diff.args, \"diff-index\", \"-p\",\n+\t\t\t\t \"--no-color\",\n \t\t\t\t \"--cached\", \"--binary\", \"HEAD\", \"--\",\n \t\t\t\t NULL);\n \t\t\tadd_pathspecs(&cp_diff.args, ps);\ndiff --git a/t/t3904-stash-patch.sh b/t/t3904-stash-patch.sh\nindex ae313e3c70..90a4ff2c10 100755\n--- a/t/t3904-stash-patch.sh\n+++ b/t/t3904-stash-patch.sh\n@@ -107,4 +107,23 @@ test_expect_success 'stash -p with split hunk' '\n \t! grep \"added line 2\" test\n '\n \n+test_expect_success 'stash -p not confused by GIT_PAGER_IN_USE' '\n+\techo to-stash >test &&\n+\t# Set both GIT_PAGER_IN_USE and TERM. Our goal is to entice any\n+\t# diff subprocesses into thinking that they could output\n+\t# color, even though their stdout is not going into a tty.\n+\techo y |\n+\tGIT_PAGER_IN_USE=1 TERM=vt100 git stash -p &&\n+\tgit diff --exit-code\n+'\n+\n+test_expect_success 'index push not confused by GIT_PAGER_IN_USE' '\n+\techo index >test &&\n+\tgit add test &&\n+\techo working-tree >test &&\n+\t# As above, we try to entice the child diff into using color.\n+\tGIT_PAGER_IN_USE=1 TERM=vt100 git stash push test &&\n+\tgit diff --exit-code\n+'\n+\n test_done\n-- \n2.51.0.462.g0a0e5b9b75\n\n"},{"id":"525857","messageId":"20250908164236.GB1323964@coredump.intra.peff.net","threadId":"63994","inReplyTo":"20250908164157.GA1323487@coredump.intra.peff.net","subject":"[PATCH v2 2/4] add-interactive: respect color.diff for diff coloring","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-09-08T16:42:36Z","receivedAt":"2025-09-08T16:42:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The old perl git-add--interactive.perl script used the color.diff config\noption to decide whether to color diffs (and if not set, it fell back to\nthe value of color.ui via git-config's --get-colorbool option). When we\nswitched to the builtin version, this was lost: we respect only\ncolor.ui. So for example:\n\n git -c color.diff=false add -p\n\nwould color the diff, even when it should not.\n\nThe culprit is this line in add-interactive.c's parse_diff():\n\n if (want_color_fd(1, -1))\n\nThat \"-1\" means \"no config has been set\", which causes it to fall back\nto the color.ui setting. We should instead be passing the value of\ncolor.diff. But the problem is that we never even parse that config\noption!\n\nInstead the builtin interactive code parses only the value of\ncolor.interactive, which is used for prompts and other messages. One\ncould perhaps argue that this should cover interactive diff coloring,\ntoo, but historically it did not. The perl script treated\ncolor.interactive and color.diff separately. So we should grab the\nvalues for both, keeping separate fields in our add_i_state variable,\nrather than a single use_color field.\n\nWe also load individual color slots (e.g., color.interactive.prompt),\nleaving them as the empty string when color is disabled. This happens\nvia the init_color() helper in add-interactive, which checks that\nuse_color field. Now that there are two such fields, we need to pass the\nappropriate one for each color.\n\nThe colors are mostly easy to divide up; color.interactive.* follows\ncolor.interactive, and color.diff.* follows color.diff. But the \"reset\"\ncolor is tricky. It is used for both types of coloring, but the two can\nbe configured independently. So we introduce two separate reset colors,\nand use each in the appropriate spot.\n\nThere are two new tests. The first enables interactive prompt colors but\ndisables color.diff. We should see a colored prompt but not a colored\ndiff, showing that we are now respecting color.diff (and not\ncolor.interactive or color.ui).\n\nThe second does the opposite. We disable color.interactive but turn on\ncolor.diff with a custom fragment color. When we split a hunk, the\ninteractive code has to re-color the hunk header, which lets us check\nthat we correctly loaded the color.diff.frag config based on color.diff,\nnot color.interactive.\n\nSigned-off-by: Jeff King \n---\n add-interactive.c | 79 ++++++++++++++++++++++----------------\n add-interactive.h | 7 +++-\n add-patch.c | 12 +++---\n t/t3701-add-interactive.sh | 38 ++++++++++++++++++\n 4 files changed, 95 insertions(+), 41 deletions(-)\n\ndiff --git a/add-interactive.c b/add-interactive.c\nindex 3e692b47ec..877160d298 100644\n--- a/add-interactive.c\n+++ b/add-interactive.c\n@@ -20,14 +20,14 @@\n #include \"prompt.h\"\n #include \"tree.h\"\n \n-static void init_color(struct repository *r, struct add_i_state *s,\n+static void init_color(struct repository *r, int use_color,\n \t\t const char *section_and_slot, char *dst,\n \t\t const char *default_color)\n {\n \tchar *key = xstrfmt(\"color.%s\", section_and_slot);\n \tconst char *value;\n \n-\tif (!s->use_color)\n+\tif (!use_color)\n \t\tdst[0] = '\\0';\n \telse if (repo_config_get_value(r, key, &value) ||\n \t\t color_parse(value, dst))\n@@ -36,42 +36,54 @@ static void init_color(struct repository *r, struct add_i_state *s,\n \tfree(key);\n }\n \n-void init_add_i_state(struct add_i_state *s, struct repository *r,\n-\t\t struct add_p_opt *add_p_opt)\n+static int check_color_config(struct repository *r, const char *var)\n {\n \tconst char *value;\n+\tint ret;\n+\n+\tif (repo_config_get_value(r, var, &value))\n+\t\tret = -1;\n+\telse\n+\t\tret = git_config_colorbool(var, value);\n+\treturn want_color(ret);\n+}\n \n+void init_add_i_state(struct add_i_state *s, struct repository *r,\n+\t\t struct add_p_opt *add_p_opt)\n+{\n \ts->r = r;\n \ts->context = -1;\n \ts->interhunkcontext = -1;\n \n-\tif (repo_config_get_value(r, \"color.interactive\", &value))\n-\t\ts->use_color = -1;\n-\telse\n-\t\ts->use_color =\n-\t\t\tgit_config_colorbool(\"color.interactive\", value);\n-\ts->use_color = want_color(s->use_color);\n-\n-\tinit_color(r, s, \"interactive.header\", s->header_color, GIT_COLOR_BOLD);\n-\tinit_color(r, s, \"interactive.help\", s->help_color, GIT_COLOR_BOLD_RED);\n-\tinit_color(r, s, \"interactive.prompt\", s->prompt_color,\n-\t\t GIT_COLOR_BOLD_BLUE);\n-\tinit_color(r, s, \"interactive.error\", s->error_color,\n-\t\t GIT_COLOR_BOLD_RED);\n-\n-\tinit_color(r, s, \"diff.frag\", s->fraginfo_color,\n-\t\t diff_get_color(s->use_color, DIFF_FRAGINFO));\n-\tinit_color(r, s, \"diff.context\", s->context_color, \"fall back\");\n+\ts->use_color_interactive = check_color_config(r, \"color.interactive\");\n+\n+\tinit_color(r, s->use_color_interactive, \"interactive.header\",\n+\t\t s->header_color, GIT_COLOR_BOLD);\n+\tinit_color(r, s->use_color_interactive, \"interactive.help\",\n+\t\t s->help_color, GIT_COLOR_BOLD_RED);\n+\tinit_color(r, s->use_color_interactive, \"interactive.prompt\",\n+\t\t s->prompt_color, GIT_COLOR_BOLD_BLUE);\n+\tinit_color(r, s->use_color_interactive, \"interactive.error\",\n+\t\t s->error_color, GIT_COLOR_BOLD_RED);\n+\tstrlcpy(s->reset_color_interactive,\n+\t\ts->use_color_interactive ? GIT_COLOR_RESET : \"\", COLOR_MAXLEN);\n+\n+\ts->use_color_diff = check_color_config(r, \"color.diff\");\n+\n+\tinit_color(r, s->use_color_diff, \"diff.frag\", s->fraginfo_color,\n+\t\t diff_get_color(s->use_color_diff, DIFF_FRAGINFO));\n+\tinit_color(r, s->use_color_diff, \"diff.context\", s->context_color,\n+\t\t \"fall back\");\n \tif (!strcmp(s->context_color, \"fall back\"))\n-\t\tinit_color(r, s, \"diff.plain\", s->context_color,\n-\t\t\t diff_get_color(s->use_color, DIFF_CONTEXT));\n-\tinit_color(r, s, \"diff.old\", s->file_old_color,\n-\t\tdiff_get_color(s->use_color, DIFF_FILE_OLD));\n-\tinit_color(r, s, \"diff.new\", s->file_new_color,\n-\t\tdiff_get_color(s->use_color, DIFF_FILE_NEW));\n-\n-\tstrlcpy(s->reset_color,\n-\t\ts->use_color ? GIT_COLOR_RESET : \"\", COLOR_MAXLEN);\n+\t\tinit_color(r, s->use_color_diff, \"diff.plain\",\n+\t\t\t s->context_color,\n+\t\t\t diff_get_color(s->use_color_diff, DIFF_CONTEXT));\n+\tinit_color(r, s->use_color_diff, \"diff.old\", s->file_old_color,\n+\t\t diff_get_color(s->use_color_diff, DIFF_FILE_OLD));\n+\tinit_color(r, s->use_color_diff, \"diff.new\", s->file_new_color,\n+\t\t diff_get_color(s->use_color_diff, DIFF_FILE_NEW));\n+\tstrlcpy(s->reset_color_diff,\n+\t\ts->use_color_diff ? GIT_COLOR_RESET : \"\", COLOR_MAXLEN);\n \n \tFREE_AND_NULL(s->interactive_diff_filter);\n \trepo_config_get_string(r, \"interactive.difffilter\",\n@@ -109,7 +121,8 @@ void clear_add_i_state(struct add_i_state *s)\n \tFREE_AND_NULL(s->interactive_diff_filter);\n \tFREE_AND_NULL(s->interactive_diff_algorithm);\n \tmemset(s, 0, sizeof(*s));\n-\ts->use_color = -1;\n+\ts->use_color_interactive = -1;\n+\ts->use_color_diff = -1;\n }\n \n /*\n@@ -1188,9 +1201,9 @@ int run_add_i(struct repository *r, const struct pathspec *ps,\n \t * When color was asked for, use the prompt color for\n \t * highlighting, otherwise use square brackets.\n \t */\n-\tif (s.use_color) {\n+\tif (s.use_color_interactive) {\n \t\tdata.color = s.prompt_color;\n-\t\tdata.reset = s.reset_color;\n+\t\tdata.reset = s.reset_color_interactive;\n \t}\n \tprint_file_item_data.color = data.color;\n \tprint_file_item_data.reset = data.reset;\ndiff --git a/add-interactive.h b/add-interactive.h\nindex 4213dcd67b..ceadfa6bb6 100644\n--- a/add-interactive.h\n+++ b/add-interactive.h\n@@ -12,16 +12,19 @@ struct add_p_opt {\n \n struct add_i_state {\n \tstruct repository *r;\n-\tint use_color;\n+\tint use_color_interactive;\n+\tint use_color_diff;\n \tchar header_color[COLOR_MAXLEN];\n \tchar help_color[COLOR_MAXLEN];\n \tchar prompt_color[COLOR_MAXLEN];\n \tchar error_color[COLOR_MAXLEN];\n-\tchar reset_color[COLOR_MAXLEN];\n+\tchar reset_color_interactive[COLOR_MAXLEN];\n+\n \tchar fraginfo_color[COLOR_MAXLEN];\n \tchar context_color[COLOR_MAXLEN];\n \tchar file_old_color[COLOR_MAXLEN];\n \tchar file_new_color[COLOR_MAXLEN];\n+\tchar reset_color_diff[COLOR_MAXLEN];\n \n \tint use_single_key;\n \tchar *interactive_diff_filter, *interactive_diff_algorithm;\ndiff --git a/add-patch.c b/add-patch.c\nindex 302e6ba7d9..b0389c5d5b 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -300,7 +300,7 @@ static void err(struct add_p_state *s, const char *fmt, ...)\n \tva_start(args, fmt);\n \tfputs(s->s.error_color, stdout);\n \tvprintf(fmt, args);\n-\tputs(s->s.reset_color);\n+\tputs(s->s.reset_color_interactive);\n \tva_end(args);\n }\n \n@@ -457,7 +457,7 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)\n \t}\n \tstrbuf_complete_line(plain);\n \n-\tif (want_color_fd(1, -1)) {\n+\tif (want_color_fd(1, s->s.use_color_diff)) {\n \t\tstruct child_process colored_cp = CHILD_PROCESS_INIT;\n \t\tconst char *diff_filter = s->s.interactive_diff_filter;\n \n@@ -714,7 +714,7 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk,\n \t\tif (len)\n \t\t\tstrbuf_add(out, p, len);\n \t\telse if (colored)\n-\t\t\tstrbuf_addf(out, \"%s\\n\", s->s.reset_color);\n+\t\t\tstrbuf_addf(out, \"%s\\n\", s->s.reset_color_diff);\n \t\telse\n \t\t\tstrbuf_addch(out, '\\n');\n \t}\n@@ -1107,7 +1107,7 @@ static void recolor_hunk(struct add_p_state *s, struct hunk *hunk)\n \t\t\t s->s.file_new_color :\n \t\t\t s->s.context_color);\n \t\tstrbuf_add(&s->colored, plain + current, eol - current);\n-\t\tstrbuf_addstr(&s->colored, s->s.reset_color);\n+\t\tstrbuf_addstr(&s->colored, s->s.reset_color_diff);\n \t\tif (next > eol)\n \t\t\tstrbuf_add(&s->colored, plain + eol, next - eol);\n \t\tcurrent = next;\n@@ -1528,8 +1528,8 @@ static int patch_update_file(struct add_p_state *s,\n \t\t\t\t\t\t: 1));\n \t\tprintf(_(s->mode->prompt_mode[prompt_mode_type]),\n \t\t s->buf.buf);\n-\t\tif (*s->s.reset_color)\n-\t\t\tfputs(s->s.reset_color, stdout);\n+\t\tif (*s->s.reset_color_interactive)\n+\t\t\tfputs(s->s.reset_color_interactive, stdout);\n \t\tfflush(stdout);\n \t\tif (read_single_character(s) == EOF)\n \t\t\tbreak;\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 04d2a19835..6b400ad9a3 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -866,6 +866,44 @@ test_expect_success 'colorized diffs respect diff.wsErrorHighlight' '\n \ttest_grep \"old<\" output\n '\n \n+test_expect_success 'diff color respects color.diff' '\n+\tgit reset --hard &&\n+\n+\techo old >test &&\n+\tgit add test &&\n+\techo new >test &&\n+\n+\tprintf n >n &&\n+\tforce_color git \\\n+\t\t-c color.interactive=auto \\\n+\t\t-c color.interactive.prompt=blue \\\n+\t\t-c color.diff=false \\\n+\t\t-c color.diff.old=red \\\n+\t\tadd -p >output.raw 2>&1 output &&\n+\ttest_grep \"BLUE.*Stage this hunk\" output &&\n+\ttest_grep ! \"RED\" output\n+'\n+\n+test_expect_success 're-coloring diff without color.interactive' '\n+\tgit reset --hard &&\n+\n+\ttest_write_lines 1 2 3 >test &&\n+\tgit add test &&\n+\ttest_write_lines one 2 three >test &&\n+\n+\ttest_write_lines s n n |\n+\tforce_color git \\\n+\t\t-c color.interactive=false \\\n+\t\t-c color.interactive.prompt=blue \\\n+\t\t-c color.diff=true \\\n+\t\t-c color.diff.frag=\"bold magenta\" \\\n+\t\tadd -p >output.raw 2>&1 &&\n+\ttest_decode_color output &&\n+\ttest_grep \"@@\" output &&\n+\ttest_grep ! \"BLUE\" output\n+'\n+\n test_expect_success 'diffFilter filters diff' '\n \tgit reset --hard &&\n \n-- \n2.51.0.462.g0a0e5b9b75\n\n"},{"id":"525858","messageId":"20250908164239.GC1323964@coredump.intra.peff.net","threadId":"63994","inReplyTo":"20250908164157.GA1323487@coredump.intra.peff.net","subject":"[PATCH v2 3/4] add-interactive: manually fall back color config to color.ui","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-09-08T16:42:39Z","receivedAt":"2025-09-08T16:42:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Color options like color.interactive and color.diff should fall back to\nthe value of color.ui if they aren't set. In add-interactive, we check\nthe specific options (e.g., color.diff) via repo_config_get_value(),\nwhich does not depend on the main command having loaded any color config\nvia the git_config() callback mechanism.\n\nBut then we call want_color() on the result; if our specific config is\nunset then that function uses the value of git_use_color_default. That\nvariable is typically set from color.ui by the git_color_config()\ncallback, which is called by the main command in its own git_config()\ncallback function.\n\nThis works fine for \"add -p\", whose add_config() callback calls into\ngit_color_config(). But it doesn't work for other commands like\n\"checkout -p\", which is otherwise unaware of color at all. People tend\nnot to notice because the default is \"auto\", and that's what they'd set\ncolor.ui to as well. But something like:\n\n git -c color.ui=false checkout -p\n\nshould disable color, and it doesn't.\n\nThis regression goes back to 0527ccb1b5 (add -i: default to the built-in\nimplementation, 2021-11-30). In the perl version we got the color config\nfrom \"git config --get-colorbool\", which did the full lookup for us.\n\nThe obvious fix is for git-checkout to add a call to git_color_config()\nto its own config callback. But we'd have to do so for every command\nwith this problem, which is error-prone. Let's see if we can fix it more\ncentrally.\n\nIt is tempting to teach want_color() to look up the value of\nrepo_config_get_value(\"color.ui\") itself. But I think that would have\ndisastrous consequences. Plumbing commands, especially older ones, avoid\nporcelain config like \"color.*\" by simply not parsing it in their config\ncallbacks. Looking up the value of color.ui under the hood would\nundermine that.\n\nInstead, let's do that lookup in the add-interactive setup code. We're\nalready demand-loading other color config there, which is probably fine\n(even in a plumbing command like \"git reset\", the interactive mode is\ninherently porcelain-ish). That catches all commands that use the\ninteractive code, whether they were calling git_color_config()\nthemselves or not.\n\nReported-by: Isaac Oscar Gariano \nSigned-off-by: Jeff King \n---\n add-interactive.c | 9 +++++++++\n t/t3701-add-interactive.sh | 15 +++++++++++++++\n 2 files changed, 24 insertions(+)\n\ndiff --git a/add-interactive.c b/add-interactive.c\nindex 877160d298..4604c69140 100644\n--- a/add-interactive.c\n+++ b/add-interactive.c\n@@ -45,6 +45,15 @@ static int check_color_config(struct repository *r, const char *var)\n \t\tret = -1;\n \telse\n \t\tret = git_config_colorbool(var, value);\n+\n+\t/*\n+\t * Do not rely on want_color() to fall back to color.ui for us. It uses\n+\t * the value parsed by git_color_config(), which may not have been\n+\t * called by the main command.\n+\t */\n+\tif (ret < 0 && !repo_config_get_value(r, \"color.ui\", &value))\n+\t\tret = git_config_colorbool(\"color.ui\", value);\n+\n \treturn want_color(ret);\n }\n \ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 6b400ad9a3..d9fe289a7a 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -1321,6 +1321,12 @@ test_expect_success 'stash accepts -U and --inter-hunk-context' '\n \ttest_grep \"@@ -2,20 +2,20 @@\" actual\n '\n \n+test_expect_success 'set up base for -p color tests' '\n+\techo commit >file &&\n+\tgit commit -am \"commit state\" &&\n+\tgit tag patch-base\n+'\n+\n for cmd in add checkout commit reset restore \"stash save\" \"stash push\"\n do\n \ttest_expect_success \"$cmd rejects invalid context options\" '\n@@ -1337,6 +1343,15 @@ do\n \t\ttest_must_fail git $cmd --inter-hunk-context 2 2>actual &&\n \t\ttest_grep -E \".--inter-hunk-context. requires .(--interactive/)?--patch.\" actual\n \t'\n+\n+\ttest_expect_success \"$cmd falls back to color.ui\" '\n+\t\tgit reset --hard patch-base &&\n+\t\techo working-tree >file &&\n+\t\ttest_write_lines y |\n+\t\tforce_color git -c color.ui=false $cmd -p >output.raw 2>&1 &&\n+\t\ttest_decode_color output &&\n+\t\ttest_cmp output.raw output\n+\t'\n done\n \n test_done\n-- \n2.51.0.462.g0a0e5b9b75\n\n"},{"id":"525859","messageId":"20250908164242.GD1323964@coredump.intra.peff.net","threadId":"63994","inReplyTo":"20250908164157.GA1323487@coredump.intra.peff.net","subject":"[PATCH v2 4/4] contrib/diff-highlight: mention interactive.diffFilter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-09-08T16:42:42Z","receivedAt":"2025-09-08T16:42:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When the README for diff-highlight was written, there was no way to\ntrigger it for the `add -p` interactive patch mode. We've since grown a\nfeature to support that, but it was documented only on the Git side.\nLet's also let people coming the other direction, from diff-highlight,\nknow that it's an option.\n\nSuggested-by: Isaac Oscar Gariano \nSigned-off-by: Jeff King \n---\n contrib/diff-highlight/README | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/contrib/diff-highlight/README b/contrib/diff-highlight/README\nindex d4c2343175..1db4440e68 100644\n--- a/contrib/diff-highlight/README\n+++ b/contrib/diff-highlight/README\n@@ -58,6 +58,14 @@ following in your git configuration:\n \tdiff = diff-highlight | less\n ---------------------------------------------\n \n+If you use the interactive patch mode of `git add -p`, `git checkout\n+-p`, etc, you may also want to configure it to be used there:\n+\n+---------------------------------------------\n+[interactive]\n+ diffFilter = diff-highlight\n+---------------------------------------------\n+\n \n Color Config\n ------------\n-- \n2.51.0.462.g0a0e5b9b75\n"},{"id":"525894","messageId":"aL_D-quAoabKxhCN@pks.im","threadId":"63994","inReplyTo":"20250908161648.GC1308482@coredump.intra.peff.net","subject":"Re: [PATCH 2/4] add-interactive: respect color.diff for diff coloring","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-09-09T06:06:50Z","receivedAt":"2025-09-09T06:07:01Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Sep 08, 2025 at 12:16:48PM -0400, Jeff King wrote:\n> On Wed, Sep 03, 2025 at 09:23:27AM +0200, Patrick Steinhardt wrote:\n> \n> > > +static int check_color_config(struct repository *r, const char *var)\n> > > {\n> > > \tconst char *value;\n> > > +\tint ret;\n> > > +\n> > > +\tif (repo_config_get_value(r, var, &value))\n> > > +\t\tret = -1;\n> > \n> > Not an old issue, but should we use `GIT_COLOR_UNKNOWN` here?\n> \n> My initial reaction was: yeah, we could probably fix this up in a\n> preparatory patch. But the problem is much deeper than the\n> add-interactive code. Nobody uses GIT_COLOR_UNKNOWN at all! Even\n> git_config_colorbool() just returns -1.\n> \n> Moreover, it does not even use the ALWAYS/NEVER defines, but just 1 and\n> 0. Making things even more complicated, we sometimes want to consider\n> \"do we want color\" as this always/never/auto/unknown set, and then\n> sometimes we collapse that (using the same variable!) into a single\n> true/false value.\n> \n> So using that consistently and possibly switching to an enum is a much\n> bigger topic. It may be worth cleaning up, but I don't think it's worth\n> derailing this regression fix. In the meantime, I'd rather keep this\n> code matching the rest of the color code (it's not even really adding\n> new instances of \"-1\", but just shuffling them around).\n\nMakes sense.\n\nPatrick\n"},{"id":"525895","messageId":"aL_EfmRj_zDC_8xm@pks.im","threadId":"63994","inReplyTo":"20250908164157.GA1323487@coredump.intra.peff.net","subject":"Re: [PATCH v2 0/4] oddities around add-interactive and color","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-09-09T06:09:02Z","receivedAt":"2025-09-09T06:09:08Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Sep 08, 2025 at 12:41:57PM -0400, Jeff King wrote:\n> On Thu, Aug 21, 2025 at 03:07:40AM -0400, Jeff King wrote:\n> \n> > So here's a series which I think addresses everything I found. These\n> > bugs have been lurking for a while, but I guess not many people tend to\n> > set color variables to anything exotic.\n> \n> And here's a v2 based on Patrick's review. I also touched up a few lines\n> whose indentation did not pass clang-format (not new, but ones I was\n> touching or moving around). The only thing I punted on was refactoring\n> the GIT_COLOR_* defines, as I think it extends well beyond the code I'm\n> touching here (see the reply I left in the thread).\n\nThanks, this addresses all of my feedback from v1. Well, except the\nGIT_COLOR_* defines, but I agree that it doesn't make sense to do that\nas part of this series.\n\nPatrick\n"}]}