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

From: Jeff King <[email-masked]>
Date: Sun, 22 Mar 2026 16:45:09 -0400

On Mon, Mar 23, 2026 at 01:18:30AM +0800, Tian Yuchen wrote:

> > -	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?

I noticed it, too, but didn't have a better name (in fact they are of
the same type, just two different contexts). Shadowing seemed less bad
to me than using a mis-matched name.

> > +	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.

The length() of undef is also undef (and documented in "perldoc -f
length"). But either way, length($s) will be false, and we will return
$default, not $s.

I don't get any warning on perl 5.40.1. Are you seeing one on a
different version?

-Peff