git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jakub Narebski <jnareb@gmail.com>
To: Junio C Hamano <junkio@cox.net>
Cc: git@vger.kernel.org
Subject: Re: [PATCH 3/3] gitweb: Few doublequoted strings cleanups
Date: Wed, 4 Apr 2007 11:10:33 +0200	[thread overview]
Message-ID: <200704041110.33921.jnareb@gmail.com> (raw)
In-Reply-To: <7vlkh9w6pa.fsf@assigned-by-dhcp.cox.net>

Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
>> Replace a few doublequoted strings by theirs singlequoted equivalent,
>> lose doublequotes around variable in string containing only
>> of a variable name, use '' consistently as an empty string (and not
>> sometimes as "").
> 
> Why?  I do not in particular like a micro-cleanup like this (it
> seems more of personal taste than cleanup).

IMVHO consistency (always use '' to denote empty string... or always
use "") is a cleanup.

But I'm not that attached to this patch. It could stay or it could
go... One of the reasons for this patch was (most probably wrong)
idea from dealing with PHP that using single quotes makes program
a tiny bit faster.

>> -	} elsif ($char eq "\\") {
>> +	} elsif ($char eq '\\') {
>>  		$diff_class = " incomplete";
>>  	}
> 
> Especially this makes a shell scripter think twice before
> realizing that this is Perl and a backslash expands inside
> single quotes.  In other words, I find that the former is easier
> to read.

O.K. Although you can always escape singlequote inside single
quotes, so escape character also has to be escaped.

>> @@ -1052,7 +1052,7 @@ sub git_get_projects_list {
>>  		my $dir = $projects_list . ($filter ? "/$filter" : '');
>>  		# remove the trailing "/"
>>  		$dir =~ s!/+$!!;
>> -		my $pfxlen = length("$dir");
>> +		my $pfxlen = length($dir);
> 
> On the other hand, I think this makes the code easier to read.

There are only two such chunks. Feel free to pick them...
 
>> @@ -3616,7 +3616,7 @@ sub git_snapshot {
>>  
>>  	print $cgi->header(
>>  		-type => "application/$ctype",
>> -		-content_disposition => 'inline; filename="' . "$filename" . '"',
>> +		-content_disposition => 'inline; filename="' . $filename . '"',
>>  		-status => '200 OK');
>>  
> 
> Wouldn't it be easier to read if we did
> 
> 	"inline; filename=\"$filename\""
> 
> instead?

It would. Right.

-- 
Jakub Narebski
Poland

      reply	other threads:[~2007-04-04 12:12 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-04-01 20:21 [PATCH 0/3] gitweb: Few cleanup patches Jakub Narebski
2007-04-01 20:21 ` [PATCH 1/3] gitweb: Whitespace cleanup - tabs are for indent, spaces are for align (3) Jakub Narebski
2007-04-01 20:22 ` [PATCH 2/3] gitweb: Quote hash keys, and do not use barewords keys Jakub Narebski
2007-04-01 20:22 ` [PATCH 3/3] gitweb: Few doublequoted strings cleanups Jakub Narebski
2007-04-03 20:20   ` Junio C Hamano
2007-04-04  9:10     ` Jakub Narebski [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=200704041110.33921.jnareb@gmail.com \
    --to=jnareb@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=junkio@cox.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).