Re: [PATCH 8/8] diff-highlight: fetch all config with one process

[Tian Yuchen <cat@malon.dev>]

Hi Jeff,

On 3/20/26 08:48, Jeff King wrote:
> When diff-highlight was written, there was no way to fetch multiple
> config keys _and_ have them interpreted as colors. So we were stuck
> with either invoking git-config once for each config key, or fetching
> them all and converting human-readable color names into ANSI codes
> ourselves.
> 
> I chose the former, but it means that diff-highlight kicks off 6
> git-config processes (even if you haven't configured anything, it has to
> check each one).
> 
> But since Git 2.18.0, we can do:
> 
>     git config --type=color --get-regexp=^color\.diff-highlight\.
> 
> to get all of them in one shot.
> 
> Note that any callers which pass in colors directly to the module via
> @OLD_HIGHLIGHT and @NEW_HIGHLIGHT (like diff-so-fancy plans to do) are
> unaffected; those colors suppress any config lookup we'd do ourselves.
> 
> You can see the effect like:
> 
>    # diff-highlight suppresses git-config's stderr, so dump
>    # trace through descriptor 3
>    git show d1f33c753d | GIT_TRACE=3 diff-highlight 3>&2 >/dev/null
> 
> which drops from 6 lines down to 1.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>   contrib/diff-highlight/DiffHighlight.pm | 26 ++++++++++++++++++-------
>   1 file changed, 19 insertions(+), 7 deletions(-)
> 
> diff --git a/contrib/diff-highlight/DiffHighlight.pm b/contrib/diff-highlight/DiffHighlight.pm
> index 96369eadf9..a22ba7a851 100644
> --- a/contrib/diff-highlight/DiffHighlight.pm
> +++ b/contrib/diff-highlight/DiffHighlight.pm
> @@ -131,8 +131,20 @@ sub highlight_stdin {
>   # of it being used in other settings. Let's handle our own
>   # fallback, which means we will work even if git can't be run.
>   sub color_config {
> +	our $cached_config;
>   	my ($key, $default) = @_;

$key...

> -	my $s = `git config --get-color $key 2>$NULL`;
> +
> +	if (!defined $cached_config) {
> +		$cached_config = {};
> +		my $data = `git config --type=color --get-regexp '^color\.diff-highlight\.' 2>$NULL`;
> +		for my $line (split /\n/, $data) {
> +			my ($key, $color) = split ' ', $line, 2;

...another $key. I think it would be better to change the name here. 
What do you think?

> +			$key =~ s/^color\.diff-highlight\.// or next;
> +			$cached_config->{$key} = $color;
> +		}
> +	}
> +

...

> +	my $s = $cached_config->{$key};
>   	return length($s) ? $s : $default;
>   }
>  

Something doesn't feel quite right here.

If the user has not configured color.diff-highlight.*, the expression 
git config --type=color --get-regexp=^color\.diff-highlight\. will not 
find a match and should not output anything. In this case, 
%cached_config->{$key} becomes undef, length() returns 0, and a warning 
is issued.

But we have "use warnings FATAL => 'all'". This situation will result in 
a fatal error, which I don't think is what we want.


> @@ -172,16 +184,16 @@ sub load_color_config {
>   	# always be set if you want highlighting to do anything.
>   	if (!defined $OLD_HIGHLIGHT[1]) {
>   		@OLD_HIGHLIGHT = (
> -			color_config('color.diff-highlight.oldnormal'),
> -			color_config('color.diff-highlight.oldhighlight', "\x1b[7m"),
> -			color_config('color.diff-highlight.oldreset', "\x1b[27m")
> +			color_config('oldnormal'),
> +			color_config('oldhighlight', "\x1b[7m"),
> +			color_config('oldreset', "\x1b[27m")
>   		);
>   	}
>   	if (!defined $NEW_HIGHLIGHT[1]) {
>   		@NEW_HIGHLIGHT = (
> -			color_config('color.diff-highlight.newnormal', $OLD_HIGHLIGHT[0]),
> -			color_config('color.diff-highlight.newhighlight', $OLD_HIGHLIGHT[1]),
> -			color_config('color.diff-highlight.newreset', $OLD_HIGHLIGHT[2])
> +			color_config('newnormal', $OLD_HIGHLIGHT[0]),
> +			color_config('newhighlight', $OLD_HIGHLIGHT[1]),
> +			color_config('newreset', $OLD_HIGHLIGHT[2])
>   		);
>   	};
>   }

Regards,

Yuchen