Re: [PATCH v3 22/28] git-remote-mediawiki: Modify strings for a better coding-style
Matthieu Moy writes: > Please, don't top-post. Quote the part of the message you're replying > to, and reply below. > > Benoît Person writes: > >> 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 :) . > > Agreed. run_git was written to avoid having to depend on Git.pm, but now > that we do, we should do it the Git.pm way (although this is not a > high priority for now). > >> Don't know though if it's better to mix that with this serie which is >> mainly based on what perlcritic returns. > > If you go this way, I'd rather have it on top (i.e. a separate patch > series). Or not worry too much about it in the 3-week long school project. Finish one that you started and then build on top. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 22/28] git-remote-mediawiki: Modify strings for a better coding-style
Please, don't top-post. Quote the part of the message you're replying to, and reply below. Benoît Person writes: > 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 :) . Agreed. run_git was written to avoid having to depend on Git.pm, but now that we do, we should do it the Git.pm way (although this is not a high priority for now). > Don't know though if it's better to mix that with this serie which is > mainly based on what perlcritic returns. If you go this way, I'd rather have it on top (i.e. a separate patch series). -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 22/28] git-remote-mediawiki: Modify strings for a better coding-style
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 wrote:
> Matthieu Moy writes:
>
>> Célestin Matte 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 majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 22/28] git-remote-mediawiki: Modify strings for a better coding-style
Matthieu Moy writes:
> Célestin Matte 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 22/28] git-remote-mediawiki: Modify strings for a better coding-style
Le 10/06/2013 10:37, Matthieu Moy a écrit :
> Célestin Matte 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)
The 80-columns rule is already broken in *many* places in this file.
I know this is not a valid reason to do it more often, but in my opinion
the second version is easier to read (it's easy to miss the dots in the
first version), so we would gain from the change.
--
Célestin Matte
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 22/28] git-remote-mediawiki: Modify strings for a better coding-style
Le 10/06/2013 02:50, Eric Sunshine a écrit :
> Given this patch's intention to use ${} within strings, should this be
> ${credential{username}}?
>
> (I don't have a preference, but it's a genuine question since it's not
> clear if this was an oversight or intentional.)
The answer is simple: I didn't know the exact syntax, so I didn't bother
doing it.
> The whitespace-only change to line "my $res = do {" is effectively
> noise. The reviewer has to stop and puzzle out what changed on the
> line before continuing with review of the remaining _real_ changes. It
> is a good idea to avoid noise changes if possible.
>
> In this particular case, it's easy to avoid the noise since the
> trailing space on that line could/should have been removed in patch
> 18/28 when the statement was split over multiple lines.
Actually, I noticed this but didn't find what the difference between the
two lines was. I assumed git was making some kind of mistake - but eh,
it seems git is never wrong :)
>> local $/ = undef;
>> <$git>
>> };
>> @@ -475,26 +475,26 @@ sub download_mw_mediafile {
>> return $response->decoded_content;
>> } else {
>> print STDERR "Error downloading mediafile from :\n";
>> - print STDERR "URL: $download_url\n";
>> - print STDERR "Server response: " . $response->code . " " .
>> $response->message . "\n";
>> + print STDERR "URL: ${download_url}\n";
>> + print STDERR 'Server response: ' . $response->code . q{ } .
>> $response->message . "\n";
>
> To meet the goals of this patch, would you want to do this instead?
>
> "Server response: @{[$response->code]} @{[$response->message]}\n";
>
> Whether this is easier or more difficult to read is a matter of
> opinion. (Again, this is a genuine question rather than a show of
> preference on my part.)
Same as above, I tried to change it but didn't know the exact syntax, so
I gave up.
--
Célestin Matte
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 22/28] git-remote-mediawiki: Modify strings for a better coding-style
Célestin Matte 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)
> @@ -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.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 22/28] git-remote-mediawiki: Modify strings for a better coding-style
On Sun, Jun 9, 2013 at 6:22 PM, Célestin Matte
wrote:
> - strings which don't need interpolation are single-quoted for more clarity
> and
> slight gain of performance
> - interpolation is preferred over concatenation in many cases, for more
> clarity
> - variables are always used with the ${} operator inside strings
> - strings including double-quotes are written with qq() so that the quotes do
> not have to be escaped
> ---
> diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl
> b/contrib/mw-to-git/git-remote-mediawiki.perl
> index a66cef4..efc376a 100755
> --- a/contrib/mw-to-git/git-remote-mediawiki.perl
> +++ b/contrib/mw-to-git/git-remote-mediawiki.perl
> @@ -200,10 +200,10 @@ sub mw_connect_maybe {
>lgdomain => $wiki_domain};
> if ($mediawiki->login($request)) {
> Git::credential \%credential, 'approve';
> - print STDERR "Logged in mediawiki user
> \"$credential{username}\".\n";
> + print STDERR qq(Logged in mediawiki user
> "$credential{username}".\n);
Given this patch's intention to use ${} within strings, should this be
${credential{username}}?
(I don't have a preference, but it's a genuine question since it's not
clear if this was an oversight or intentional.)
> } else {
> - print STDERR "Failed to log in mediawiki user
> \"$credential{username}\" on $url\n";
> - print STDERR " (error " .
> + print STDERR qq(Failed to log in mediawiki user
> "$credential{username}" on ${url}\n);
Ditto: ${credential{username}}
> + print STDERR ' (error ' .
> $mediawiki->{error}->{code} . ': ' .
> $mediawiki->{error}->{details} . ")\n";
> Git::credential \%credential, 'reject';
> @@ -347,10 +347,10 @@ sub get_mw_pages {
> #$out = run_git("command args", "raw"); # don't interpret output as
> UTF-8.
> sub run_git {
> my $args = shift;
> - my $encoding = (shift || "encoding(UTF-8)");
> - open(my $git, "-|:$encoding", "git " . $args)
> - or die "Unable to open: $!\n";
> - my $res = do {
> + my $encoding = (shift || 'encoding(UTF-8)');
> + open(my $git, "-|:${encoding}", "git ${args}")
> + or die "Unable to fork: $!\n";
> + my $res = do {
The whitespace-only change to line "my $res = do {" is effectively
noise. The reviewer has to stop and puzzle out what changed on the
line before continuing with review of the remaining _real_ changes. It
is a good idea to avoid noise changes if possible.
In this particular case, it's easy to avoid the noise since the
trailing space on that line could/should have been removed in patch
18/28 when the statement was split over multiple lines.
> local $/ = undef;
> <$git>
> };
> @@ -475,26 +475,26 @@ sub download_mw_mediafile {
> return $response->decoded_content;
> } else {
> print STDERR "Error downloading mediafile from :\n";
> - print STDERR "URL: $download_url\n";
> - print STDERR "Server response: " . $response->code . " " .
> $response->message . "\n";
> + print STDERR "URL: ${download_url}\n";
> + print STDERR 'Server response: ' . $response->code . q{ } .
> $response->message . "\n";
To meet the goals of this patch, would you want to do this instead?
"Server response: @{[$response->code]} @{[$response->message]}\n";
Whether this is easier or more difficult to read is a matter of
opinion. (Again, this is a genuine question rather than a show of
preference on my part.)
> exit 1;
> }
> }
> @@ -691,8 +691,7 @@ sub fetch_mw_revisions {
> my $n = 1;
> foreach my $page (@pages) {
> my $id = $page->{pageid};
> -
> - print STDERR "page $n/", scalar(@pages), ": ". $page->{title}
> ."\n";
> + print STDERR "page ${n}/", scalar(@pages), ': ',
> $page->{title}, "\n";
Similarly:
"page ${n}/@{[scalar(@pages)]}: @{[$page->{title}]}\n"
> $n++;
> my @page_revs = fetch_mw_revisions_for_page($page, $id,
> $fetch_from);
> @revisions = (@page_revs, @revisions);
> @@ -706,7 +705,7 @@ sub fe_escape_path {
> }
> - print STDOUT "N inline :$n\n";
> - literal_data("mediawiki_revision: " . $commit{mw_revision});
> + print STDOUT "N inline :${n}\n";
> + literal_data("mediawiki_revision: $commit{mw_revision}");
As questioned earlier, do you want ${commit{mw_revision}}?
> print STDOUT "\n\n";
> return;
> }
> @@ -911,8 +910,8 @@ sub mw_import_revids {
> my $page_title = $result_page->{title};
>
> if (!exists($pages->{$page_title})) {
> -
[PATCH v3 22/28] git-remote-mediawiki: Modify strings for a better coding-style
- strings which don't need interpolation are single-quoted for more clarity and
slight gain of performance
- interpolation is preferred over concatenation in many cases, for more clarity
- variables are always used with the ${} operator inside strings
- strings including double-quotes are written with qq() so that the quotes do
not have to be escaped
Signed-off-by: Célestin Matte
Signed-off-by: Matthieu Moy
---
contrib/mw-to-git/git-remote-mediawiki.perl | 242 +--
1 file changed, 120 insertions(+), 122 deletions(-)
diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl
b/contrib/mw-to-git/git-remote-mediawiki.perl
index a66cef4..efc376a 100755
--- a/contrib/mw-to-git/git-remote-mediawiki.perl
+++ b/contrib/mw-to-git/git-remote-mediawiki.perl
@@ -18,13 +18,13 @@ use DateTime::Format::ISO8601;
use warnings;
# By default, use UTF-8 to communicate with Git and the user
-binmode STDERR, ":encoding(UTF-8)";
-binmode STDOUT, ":encoding(UTF-8)";
+binmode STDERR, ':encoding(UTF-8)';
+binmode STDOUT, ':encoding(UTF-8)';
use URI::Escape;
# Mediawiki filenames can contain forward slashes. This variable decides by
which pattern they should be replaced
-use constant SLASH_REPLACEMENT => "%2F";
+use constant SLASH_REPLACEMENT => '%2F';
# It's not always possible to delete pages (may require some
# privileges). Deleted pages are replaced with this content.
@@ -35,7 +35,7 @@ use constant DELETED_CONTENT => "[[Category:Deleted]]\n";
use constant EMPTY_CONTENT => "\n";
# used to reflect file creation or deletion in diff.
-use constant NULL_SHA1 => "";
+use constant NULL_SHA1 => '';
# Used on Git's side to reflect empty edit messages on the wiki
use constant EMPTY_MESSAGE => '*Empty MediaWiki Message*';
@@ -49,35 +49,35 @@ my $url = $ARGV[1];
# Accept both space-separated and multiple keys in config file.
# Spaces should be written as _ anyway because we'll use chomp.
-my @tracked_pages = split(/[ \n]/, run_git("config --get-all remote.".
$remotename .".pages"));
+my @tracked_pages = split(/[ \n]/, run_git("config --get-all
remote.${remotename}.pages"));
chomp(@tracked_pages);
# Just like @tracked_pages, but for MediaWiki categories.
-my @tracked_categories = split(/[ \n]/, run_git("config --get-all remote.".
$remotename .".categories"));
+my @tracked_categories = split(/[ \n]/, run_git("config --get-all
remote.${remotename}.categories"));
chomp(@tracked_categories);
# Import media files on pull
-my $import_media = run_git("config --get --bool remote.". $remotename
.".mediaimport");
+my $import_media = run_git("config --get --bool
remote.${remotename}.mediaimport");
chomp($import_media);
-$import_media = ($import_media eq "true");
+$import_media = ($import_media eq 'true');
# Export media files on push
-my $export_media = run_git("config --get --bool remote.". $remotename
.".mediaexport");
+my $export_media = run_git("config --get --bool
remote.${remotename}.mediaexport");
chomp($export_media);
-$export_media = !($export_media eq "false");
+$export_media = !($export_media eq 'false');
-my $wiki_login = run_git("config --get remote.". $remotename .".mwLogin");
+my $wiki_login = run_git("config --get remote.${remotename}.mwLogin");
# Note: mwPassword is discourraged. Use the credential system instead.
-my $wiki_passwd = run_git("config --get remote.". $remotename .".mwPassword");
-my $wiki_domain = run_git("config --get remote.". $remotename .".mwDomain");
+my $wiki_passwd = run_git("config --get remote.${remotename}.mwPassword");
+my $wiki_domain = run_git("config --get remote.${remotename}.mwDomain");
chomp($wiki_login);
chomp($wiki_passwd);
chomp($wiki_domain);
# Import only last revisions (both for clone and fetch)
-my $shallow_import = run_git("config --get --bool remote.". $remotename
.".shallow");
+my $shallow_import = run_git("config --get --bool
remote.${remotename}.shallow");
chomp($shallow_import);
-$shallow_import = ($shallow_import eq "true");
+$shallow_import = ($shallow_import eq 'true');
# Fetch (clone and pull) by revisions instead of by pages. This behavior
# is more efficient when we have a wiki with lots of pages and we fetch
@@ -85,13 +85,13 @@ $shallow_import = ($shallow_import eq "true");
# Possible values:
# - by_rev: perform one query per new revision on the remote wiki
# - by_page: query each tracked page for new revision
-my $fetch_strategy = run_git("config --get remote.$remotename.fetchStrategy");
+my $fetch_strategy = run_git("config --get
remote.${remotename}.fetchStrategy");
unless ($fetch_strategy) {
- $fetch_strategy = run_git("config --get mediawiki.fetchStrategy");
+ $fetch_strategy = run_git('config --get mediawiki.fetchStrategy');
}
chomp($fetch_strategy);
unless ($fetch_strategy) {
- $fetch_strategy = "by_page";
+ $fetch_strategy = 'by_page';
}
# Remember the timestamp correspond
