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

[Matthieu Moy]

Célestin Matte celestin.ma...@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)

 @@ -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

[Célestin Matte]

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]

Le 10/06/2013 10:37, Matthieu Moy a écrit :
 Célestin Matte celestin.ma...@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)

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

[Junio C Hamano]

Matthieu Moy matthieu@grenoble-inp.fr writes:

 Célestin Matte celestin.ma...@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 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

[Benoît Person]

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 gits...@pobox.com wrote:
 Matthieu Moy matthieu@grenoble-inp.fr writes:

 Célestin Matte celestin.ma...@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 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]

Please, don't top-post. Quote the part of the message you're replying
to, and reply below.

Benoît Person benoit.per...@ensimag.fr 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

[Junio C Hamano]

Matthieu Moy matthieu@grenoble-inp.fr writes:

 Please, don't top-post. Quote the part of the message you're replying
 to, and reply below.

 Benoît Person benoit.per...@ensimag.fr 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

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

[Célestin Matte]

- 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 celestin.ma...@ensimag.fr
Signed-off-by: Matthieu Moy matthieu@grenoble-inp.fr
---
 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 = !-- empty page --\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 corresponding to a 

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

[Eric Sunshine]

On Sun, Jun 9, 2013 at 6:22 PM, Célestin Matte
celestin.ma...@ensimag.fr 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})) {
 -   print STDERR $n/, scalar(@$revision_ids),
 -   : Skipping revision #$rev-{revid} of 
 $page_title\n;
 +