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

From: Benoît Person <benoit.person@ensimag.fr>
Date: Jun 10, 2013, 17:18 UTC
Message-ID: <CAETqRCh2OLq=gj-fEGrO1vyQQfTJNAFp_z_Bf0sirbBgRKPrdw@mail.gmail.com>
In-Reply-To: <7vhah5kje8.fsf@alter.siamese.dyndns.org>

Well, I think next step would be to replace all those calls with Git.pm `command`, `command_oneline` and `config``which take an array of arguments after the "command". In the preview tool we use those but I don't know if we will find the time to clean that up too in git-remote-mediawiki :) .

Don't know though if it's better to mix that with this serie which is mainly based on what perlcritic returns.

On 10 June 2013 18:41, Junio C Hamano <gitster@pobox.com> wrote:

Show 50 quoted lines

> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
>> 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.
>
>>> @@ -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.
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html