Re: [PATCH v3 22/28] git-remote-mediawiki: Modify strings for a better coding-style

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 10, 2013, 16:41 UTC
Message-ID: <7vhah5kje8.fsf@alter.siamese.dyndns.org>
In-Reply-To: <vpqk3m2qs2r.fsf@anie.imag.fr>

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

Show 11 quoted lines

> Célestin Matte <celestin.matte@ensimag.fr> writes:
>
>> @@ -1285,8 +1285,7 @@ sub get_mw_namespace_id {
>>  		# Look at configuration file, if the record for that namespace is
>>  		# already cached. Namespaces are stored in form:
>>  		# "Name_of_namespace:Id_namespace", ex.: "File:6".
>> -		my @temp = split(/\n/, run_git("config --get-all remote."
>> -						. $remotename .".namespaceCache"));
>> +		my @temp = split(/\n/, run_git("config --get-all remote.${remotename}.namespaceCache"));
>
> I tend to prefer the former, as it avoids long lines (> 80 columns)

But you split the name of a single variable across lines by doing so.

You could split lines to make it a bit more readable.

		my @temp = split(/\n/,
			run_git("config --get-all remote.${remotename}.namespaceCache"));

It still overflows the 80-column limit, but the "namespaceCache" is the only thing that overflows; not much harm is done.

This is totally outside of the topic of "coding-style" series, but I would be more concerned about the use of ${remotename}, though. Has it (and in general the parameters to all calls to run_git()) been sanitized for shell metacharacters? If we had a variant of run_git that took an array of command line arguments given to git, you could do this

		my @temp = split(/\n/,
			run_git([qw(config --get-all),
                        	"remote.${remotename}.namespaceCache")]);

which would be safer and easier to read.

Show 9 quoted lines

>> @@ -1339,8 +1338,7 @@ sub get_mw_namespace_id {
>>  
>>  	# Store explicitely requested namespaces on disk
>>  	if (!exists $cached_mw_namespace_id{$name}) {
>> -		run_git("config --add remote.". $remotename
>> -			.".namespaceCache \"". $name .":". $store_id ."\"");
>> +		run_git(qq(config --add remote.${remotename}.namespaceCache "${name}:${store_id}"));
>
> Same.