[PATCH v2] add: check color.ui for interactive add

["Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com>]

From: Derrick Stolee <derrickstolee@github.com>

When 'git add -i' and 'git add -p' were converted to a builtin, they
introduced a color bug: the 'color.ui' config setting is ignored.

The included test demonstrates an example that is similar to the
previous test, which focuses on customizing colors. Here, we are
demonstrating that colors are not being used at all by comparing the raw
output and the color-decoded version of that output.

The fix is simple, to use git_color_default_config() as the fallback for
git_add_config(). A more robust change would instead encapsulate the
git_use_color_default global in methods that would check the config
setting if it has not been initialized yet. Some ideas are being
discussed on this front [1], but nothing has been finalized.

[1] https://lore.kernel.org/git/pull.1539.git.1685716420.gitgitgadget@gmail.com/

This test case naturally bisects down to 0527ccb1b55 (add -i: default to
the built-in implementation, 2021-11-30), but the fix makes it clear
that this would be broken even if we added the config to use the builtin
earlier than this.

Reported-by: Greg Alexander <gitgreg@galexander.org>
Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
    add: check color.ui for interactive add
    
    This was reported by Greg Alexander gitgreg@galexander.org during Git
    IRC Standup [2].
    
    [2]
    https://colabti.org/irclogger/irclogger_log/git-devel?date=2023-06-05
    
    This is also a reoccurrence of the "config not loaded" bug from [3].
    
    [3]
    https://lore.kernel.org/git/pull.1530.git.1683745654800.gitgitgadget@gmail.com/
    
    I linked above to my RFC on lazy-loading global Git config, and these
    are the same "root cause" (not loading something early enough in the
    process) and my RFC proposes to fix this by changing our access
    patterns. By encapsulating these globals, we can make sure they are
    initialized from config before they are accessed.
    
    But that's a discussion for another thread. For now, fix the bug and
    we'll worry about the "better" (and bigger) thing to do another time.
    
    
    Update in v2
    ============
    
     * The test is simplified to compare the raw output to the color-decoded
       output. This makes the test more robust to possible future changes to
       interactive add.
    
    Thanks, -Stolee
    
    P.S. This fails the whitespace check due to the necessary left-padding
    spaces in the expected output in the test file.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1541%2Fderrickstolee%2Fadd-interactive-color-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1541/derrickstolee/add-interactive-color-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1541

Range-diff vs v1:

 1:  a76893de8a7 ! 1:  6d3e34d51c5 add: check color.ui for interactive add
     @@ Commit message
      
          The included test demonstrates an example that is similar to the
          previous test, which focuses on customizing colors. Here, we are
     -    demonstrating that colors are not being used at all.
     +    demonstrating that colors are not being used at all by comparing the raw
     +    output and the color-decoded version of that output.
      
          The fix is simple, to use git_color_default_config() as the fallback for
          git_add_config(). A more robust change would instead encapsulate the
     @@ t/t3701-add-interactive.sh: test_expect_success 'colors can be overridden' '
      +		-c color.ui=false \
      +		add -i >actual.raw <input &&
      +	test_decode_color <actual.raw >actual &&
     -+	cat >expect <<-\EOF &&
     -+	           staged     unstaged path
     -+	  1:        +3/-0        +2/-1 color-test
     -+
     -+	*** Commands ***
     -+	  1: [s]tatus	  2: [u]pdate	  3: [r]evert	  4: [a]dd untracked
     -+	  5: [p]atch	  6: [d]iff	  7: [q]uit	  8: [h]elp
     -+	What now> status        - show paths with changes
     -+	update        - add working tree state to the staged set of changes
     -+	revert        - revert staged set of changes back to the HEAD version
     -+	patch         - pick hunks and update selectively
     -+	diff          - view diff between HEAD and index
     -+	add untracked - add contents of untracked files to the staged set of changes
     -+	*** Commands ***
     -+	  1: [s]tatus	  2: [u]pdate	  3: [r]evert	  4: [a]dd untracked
     -+	  5: [p]atch	  6: [d]iff	  7: [q]uit	  8: [h]elp
     -+	What now> Bye.
     -+	EOF
     -+	test_cmp expect actual
     ++	test_cmp actual.raw actual
      +'
      +
       test_expect_success 'colorized diffs respect diff.wsErrorHighlight' '


 builtin/add.c              |  2 +-
 t/t3701-add-interactive.sh | 15 +++++++++++++++
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/builtin/add.c b/builtin/add.c
index 76cc026a68a..6137e7b4ad7 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -365,7 +365,7 @@ static int add_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
-	return git_default_config(var, value, cb);
+	return git_color_default_config(var, value, cb);
 }
 
 static const char embedded_advice[] = N_(
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 3982b6b49dc..a93fe54e2ad 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -734,6 +734,21 @@ test_expect_success 'colors can be overridden' '
 	test_cmp expect actual
 '
 
+test_expect_success 'colors can be skipped with color.ui=false' '
+	git reset --hard &&
+	test_when_finished "git rm -f color-test" &&
+	test_write_lines context old more-context >color-test &&
+	git add color-test &&
+	test_write_lines context new more-context another-one >color-test &&
+
+	test_write_lines help quit >input &&
+	force_color git \
+		-c color.ui=false \
+		add -i >actual.raw <input &&
+	test_decode_color <actual.raw >actual &&
+	test_cmp actual.raw actual
+'
+
 test_expect_success 'colorized diffs respect diff.wsErrorHighlight' '
 	git reset --hard &&
 

base-commit: fe86abd7511a9a6862d5706c6fa1d9b57a63ba09
-- 
gitgitgadget