Re: [PATCH v2 12/12] gitweb: make use of s///r

From: brian m. carlson <sandals@crustytoothpaste.net>
Date: Oct 24, 2024, 21:52 UTC
Message-ID: <ZxrBidBicBcipo5R@tapette.crustytoothpaste.net>
In-Reply-To: <ZxjtRXf8IUrvn1tK@ugly>

On 2024-10-23 at 12:34:13, Oswald Buddenhagen wrote:

Show 9 quoted lines

> On Wed, Oct 23, 2024 at 12:46:00AM +0000, brian m. carlson wrote:
> > In Perl 5.14, released in May 2011, the r modifier was added to the s///
> > operator to allow it to return the modified string instead of modifying
> > the string in place.
> 
> > This allows to write nicer, more succinct code in
> > several cases, so let's do that here.
> > 
> "several" is a bit of an overstatement.

I can rephrase if a v3 is necessary.

Show 11 quoted lines

> > +++ b/gitweb/gitweb.perl
> > @@ -1188,7 +1188,7 @@ sub evaluate_and_validate_params {
> > -				(my $error = $@) =~ s/ at \S+ line \d+.*\n?//;
> > +				my $error = $@ =~ s/ at \S+ line \d+.*\n?//r;
> > 
> i'm a fan of "excess" parentheses where the syntax relies heavily on
> the binding and priority of operators:
> 
>   my $error = ($@ =~ s/ at \S+ line \d+.*\n?//r);
> 
> which is arguably semantically clearer than the old idiom, but not shorter.

I don't think those are necessary. It's obvious to people who use the s///r idiom what's meant here, and in my experience most Perl code using that idiom doesn't use them.

Show 9 quoted lines

> > @@ -2700,7 +2700,7 @@ sub git_cmd {
> > -		map { my $a = $_; $a =~ s/(['!])/'\\$1'/g; "'$a'" } @_ );
> > +		map { my $a = $_ =~ s/(['!])/'\\$1'/gr; "'$a'" } @_ );
> > 
> i think
> 
>   map { "'".(s/(['!])/'\\$1'/gr)."'" } @_ );
> 
> should work, and is an actually significant improvement.

I'm sorry, I don't necessarily like that much better than what we have now. It's not that I think it's awful, just that I don't think it's a significant improvement. If I do a v3, I can omit the `$_ =~`, though.

-- 
brian m. carlson (they/them or he/him)
Toronto, Ontario, CA