threads / patch / 34067

patchgit-remote-mediawiki: Fix a bug in a regexp

Subject: [PATCH] git-remote-mediawiki: Fix a bug in a regexp

## tl;dr

6 messages between Jun 8, 2013 and Jun 9, 2013. Diffs are folded; open one to read it.

replies: 5people: 4as markdown or json

Célestin Matte· Jun 8, 2013, 13:35 UTC · lore

In Perl, '\n' is not a newline, but instead a literal backslash followed by an "n". As the output of "rev-list --first-parent" is line-oriented, what we want here is a newline.

Signed-off-by: Célestin Matte <celestin.matte@ensimag.fr>
Signed-off-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
---
 contrib/mw-to-git/git-remote-mediawiki.perl |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to contrib/mw-to-git/git-remote-mediawiki.perl +1 −1
diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl b/contrib/mw-to-git/git-remote-mediawiki.perl
index 7af202f..a06bc31 100755
--- a/contrib/mw-to-git/git-remote-mediawiki.perl
+++ b/contrib/mw-to-git/git-remote-mediawiki.perl
@@ -1190,7 +1190,7 @@ sub mw_push_revision {
 		# history (linearized with --first-parent)
 		print STDERR "Warning: no common ancestor, pushing complete history\n";
 		my $history = run_git("rev-list --first-parent --children $local");
-		my @history = split('\n', $history);
+		my @history = split(/\n/, $history);
 		@history = @history[1..$#history];
 		foreach my $line (reverse @history) {
 			my @commit_info_split = split(/ |\n/, $line);
-- 
1.7.9.5
Matthieu Moy· Jun 8, 2013, 18:38 UTC · re: Célestin Matte · lore

Re: [PATCH] git-remote-mediawiki: Fix a bug in a regexp

Célestin Matte <celestin.matte@ensimag.fr> writes:
> In Perl, '\n' is not a newline, but instead a literal backslash followed by an
> "n". As the output of "rev-list --first-parent" is line-oriented, what we want
> here is a newline.

This is right, but the code actually worked the way it was. I'm not sure, but my understanding is that '\n' is the string "backslash followed by n", but interpreted as a regexp, it is a newline.

The new code looks better than the old one, but the log message may be improved.

In any case, Acked-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Célestin Matte· Jun 8, 2013, 20:08 UTC · re: Matthieu Moy · lore

Re: [PATCH] git-remote-mediawiki: Fix a bug in a regexp

Le 08/06/2013 20:38, Matthieu Moy a écrit :> This is right, but the code actually worked the way it was. I'm not

Show 5 quoted lines
> sure, but my understanding is that '\n' is the string "backslash
> followed by n", but interpreted as a regexp, it is a newline.
>
> The new code looks better than the old one, but the log message may be
> improved.
Is this better?

" In Perl, '\n' is not a newline, but instead the string composed of a backslash followed by an "n". To match newlines, one has to use the /\n/ regexp. As the output of "rev-list --first-parent" is line-oriented, what we want here is to match newlines, and not the "\n" string. "

-- 
Célestin Matte
Jeff King· Jun 9, 2013, 02:57 UTC · re: Matthieu Moy · lore

Re: [PATCH] git-remote-mediawiki: Fix a bug in a regexp

On Sat, Jun 08, 2013 at 08:38:56PM +0200, Matthieu Moy wrote:
Show 9 quoted lines
> Célestin Matte <celestin.matte@ensimag.fr> writes:
> 
> > In Perl, '\n' is not a newline, but instead a literal backslash followed by an
> > "n". As the output of "rev-list --first-parent" is line-oriented, what we want
> > here is a newline.
> 
> This is right, but the code actually worked the way it was. I'm not
> sure, but my understanding is that '\n' is the string "backslash
> followed by n", but interpreted as a regexp, it is a newline.
Yes, the relevant doc (from "perldoc -f split") is:
  The pattern "/PATTERN/" may be replaced with an expression to specify
  patterns that vary at runtime.  (To do runtime compilation only once,
  use "/$variable/o".)

So it is treating "\n" as an expression and compiling the regex each time through (though I think modern perl may be smart enough to realize it is a constant expression and compile the regex only once). You would get the same behavior with this:

  split $arg, $data;

if $arg contained '\n'. Of course, you _also_ get the same thing if you use a literal newline (either "\n" or if $arg contained a literal newline), because they function the same in a regex. In other words, it does not matter which you use because perl's interpolation of "\n" and the regex expansion of "\n" are identical: t hey both mean a newline.

A more subtle example that shows what is going on is this:
  split '.', $data;

If you feed that "foo.bar.baz", it does not split it into three words; each character is a delimiter, because the dot is compiled to a regex.

> The new code looks better than the old one, but the log message may be
> improved.
Agreed. I think the best explanation is something like:
  Perl's split function takes a regex pattern argument. You can also
  feed it an expression, which is then compiled into a regex at runtime.
  It therefore works to pass your pattern via single quotes, but it is
  much less obvious to a reader that the argument is meant to be a
  regex, not a static string. Using the traditional slash-delimiters
  makes this easier to read.
-Peff
Eric Sunshine· Jun 9, 2013, 05:21 UTC · re: Jeff King · lore

Re: [PATCH] git-remote-mediawiki: Fix a bug in a regexp

On Sat, Jun 8, 2013 at 10:57 PM, Jeff King <peff@peff.net> wrote:
Show 20 quoted lines
> On Sat, Jun 08, 2013 at 08:38:56PM +0200, Matthieu Moy wrote:
>
>> Célestin Matte <celestin.matte@ensimag.fr> writes:
>>
>> > In Perl, '\n' is not a newline, but instead a literal backslash followed by an
>> > "n". As the output of "rev-list --first-parent" is line-oriented, what we want
>> > here is a newline.
>>
>> This is right, but the code actually worked the way it was. I'm not
>> sure, but my understanding is that '\n' is the string "backslash
>> followed by n", but interpreted as a regexp, it is a newline.
>
> Yes, the relevant doc (from "perldoc -f split") is:
>
>   The pattern "/PATTERN/" may be replaced with an expression to specify
>   patterns that vary at runtime.  (To do runtime compilation only once,
>   use "/$variable/o".)
>
> So it is treating "\n" as an expression and compiling the regex each
> time through ...

I read this as saying only that static /PATTERN/ can also be a composed /$PATTERN/. It does not indicate how string 'PATTERN' is treated, nor does any other part of "perldoc -f split" make special mention of string 'PATTERN'. In fact, the only explanation I found regarding string 'PATTERN' is in my Camel book (3rd edition, page 796) in a parenthesized comment:

    (... if you supply a string instead of a regular expression, it'll be
    interpreted as a regular expression anyway.)
Show 11 quoted lines
>> The new code looks better than the old one, but the log message may be
>> improved.
>
> Agreed. I think the best explanation is something like:
>
>   Perl's split function takes a regex pattern argument. You can also
>   feed it an expression, which is then compiled into a regex at runtime.
>   It therefore works to pass your pattern via single quotes, but it is
>   much less obvious to a reader that the argument is meant to be a
>   regex, not a static string. Using the traditional slash-delimiters
>   makes this easier to read.
Sounds good to me.
Eric Sunshine· Jun 9, 2013, 05:11 UTC · re: Célestin Matte · lore

Re: [PATCH] git-remote-mediawiki: Fix a bug in a regexp

On Sat, Jun 8, 2013 at 9:35 AM, Célestin Matte <celestin.matte@ensimag.fr> wrote:

Show 23 quoted lines
> In Perl, '\n' is not a newline, but instead a literal backslash followed by an
> "n". As the output of "rev-list --first-parent" is line-oriented, what we want
> here is a newline.
>
> Signed-off-by: Célestin Matte <celestin.matte@ensimag.fr>
> Signed-off-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
> ---
>  contrib/mw-to-git/git-remote-mediawiki.perl |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl b/contrib/mw-to-git/git-remote-mediawiki.perl
> index 7af202f..a06bc31 100755
> --- a/contrib/mw-to-git/git-remote-mediawiki.perl
> +++ b/contrib/mw-to-git/git-remote-mediawiki.perl
> @@ -1190,7 +1190,7 @@ sub mw_push_revision {
>                 # history (linearized with --first-parent)
>                 print STDERR "Warning: no common ancestor, pushing complete history\n";
>                 my $history = run_git("rev-list --first-parent --children $local");
> -               my @history = split('\n', $history);
> +               my @history = split(/\n/, $history);
>                 @history = @history[1..$#history];
>                 foreach my $line (reverse @history) {
>                         my @commit_info_split = split(/ |\n/, $line);

It would be quite acceptable to include this patch in your existing patch series.

← back to recent threads