Re: [PATCH v5 01/16] Git.pm: add subroutines for commenting lines

[Junio C Hamano]

Jakub Narębski  writes:

>> I prefer to have like this instead
>> 
>> sub prefix_lines {
>> my $prefix = shift;
>> my $string = join("\n", @_);
>> $string =~ s/^/$prefix/mg;
>> return $string;
>> }
>> 
>> So both subroutines can take several strings as arguments.
>
> I like the interface, but the implementation looks a bit inefficient.
> Why not simply:
> ...
> If those strings can contain embedded newlines (so that they can be
> called as in Junio example), then your solution is a must-be
>
>   sub prefix_lines {
>   my $prefix = shift;
>   my $string = join("\n", @_);
>   $string =~ s/^/$prefix/mg;
>   return $string;
>   }
>
> Well, nevermind then

OK, so in short, is that a "Reviewed-by:" from you ;-)?

I agree with the conclusion.  Thanks for a review.

Re: [PATCH v5 01/16] Git.pm: add subroutines for commenting lines

[Jakub Narębski]

W dniu 09.11.2016 o 18:02, Vasco Almeida pisze:
> A Ter, 08-11-2016 às 17:06 -0800, Junio C Hamano escreveu:
>> Vasco Almeida  writes:

>>> +sub comment_lines {
>>> +   my $comment_line_char = config("core.commentchar") || '#';
>>> +   return prefix_lines("$comment_line_char ", @_);
>>> +}
>>> +
>>
>> This makes it appear as if comment_lines can take arbitrary number
>> of strings as its arguments (because the outer caller just passes @_
>> thru), but in fact because prefix_lines ignores anything other than
>> $_[0] and $_[1], only the first parameter given to comment_lineS sub
>> is inspected for lines in it and the prefix-char prefixed at the
>> beginning of each of them.
>>
>> Which is not a great interface, as it is quite misleading.
>>
>> Perhaps
>>
>>  prefix_lines("#", join("\n", @_));
>>
>> or something like that may make it less confusing.
> 
> I prefer to have like this instead
> 
> sub prefix_lines {
> my $prefix = shift;
> my $string = join("\n", @_);
> $string =~ s/^/$prefix/mg;
> return $string;
> }
> 
> So both subroutines can take several strings as arguments.

I like the interface, but the implementation looks a bit inefficient.
Why not simply:

  sub prefix_lines {
  my $prefix = shift;
  return "$prefix" . join("\n$prefix", @_) . "\n";
  }

That is, if we can assume that those lines are not terminated by
newlines themselves.

If they can be (but cannot have embedded newlines), then

  sub prefix_lines {
  my $prefix = shift;
  return "$prefix" . join("\n$prefix", map(chomp, @_)) . "\n";
  }

If those strings can contain embedded newlines (so that they can be
called as in Junio example), then your solution is a must-be

  sub prefix_lines {
  my $prefix = shift;
  my $string = join("\n", @_);
  $string =~ s/^/$prefix/mg;
  return $string;
  }

Well, nevermind then
-- 
Jakub Narębski

Re: [PATCH v5 01/16] Git.pm: add subroutines for commenting lines

[Vasco Almeida]

A Ter, 08-11-2016 às 17:06 -0800, Junio C Hamano escreveu:
> Vasco Almeida  writes:
> 
> > 
> > Add subroutines prefix_lines and comment_lines.
> > 
> > Signed-off-by: Vasco Almeida 
> > ---
> >  perl/Git.pm | 23 +++
> >  1 file changed, 23 insertions(+)
> > 
> > diff --git a/perl/Git.pm b/perl/Git.pm
> > index b2732822a..17be59fb7 100644
> > --- a/perl/Git.pm
> > +++ b/perl/Git.pm
> > @@ -1438,6 +1438,29 @@ sub END {
> >  
> >  } # %TEMP_* Lexical Context
> >  
> > +=item prefix_lines ( PREFIX, STRING )
> > +
> > +Prefixes lines in C with C.
> > +
> > +=cut
> > +
> > +sub prefix_lines {
> > +   my ($prefix, $string) = @_;
> > +   $string =~ s/^/$prefix/mg;
> > +   return $string;
> > +}
> > +
> > +=item comment_lines ( STRING )
> > +
> > +Comments lines following core.commentchar configuration.
> > +
> > +=cut
> > +
> > +sub comment_lines {
> > +   my $comment_line_char = config("core.commentchar") || '#';
> > +   return prefix_lines("$comment_line_char ", @_);
> > +}
> > +
> 
> This makes it appear as if comment_lines can take arbitrary number
> of strings as its arguments (because the outer caller just passes @_
> thru), but in fact because prefix_lines ignores anything other than
> $_[0] and $_[1], only the first parameter given to comment_lineS sub
> is inspected for lines in it and the prefix-char prefixed at the
> beginning of each of them.
> 
> Which is not a great interface, as it is quite misleading.
> 
> Perhaps
> 
>   prefix_lines("#", join("\n", @_));
> 
> or something like that may make it less confusing.

I prefer to have like this instead

sub prefix_lines {
my $prefix = shift;
my $string = join("\n", @_);
$string =~ s/^/$prefix/mg;
return $string;
}

So both subroutines can take several strings as arguments.

Re: [PATCH v5 01/16] Git.pm: add subroutines for commenting lines

[Junio C Hamano]

Vasco Almeida  writes:

> Add subroutines prefix_lines and comment_lines.
>
> Signed-off-by: Vasco Almeida 
> ---
>  perl/Git.pm | 23 +++
>  1 file changed, 23 insertions(+)
>
> diff --git a/perl/Git.pm b/perl/Git.pm
> index b2732822a..17be59fb7 100644
> --- a/perl/Git.pm
> +++ b/perl/Git.pm
> @@ -1438,6 +1438,29 @@ sub END {
>  
>  } # %TEMP_* Lexical Context
>  
> +=item prefix_lines ( PREFIX, STRING )
> +
> +Prefixes lines in C with C.
> +
> +=cut
> +
> +sub prefix_lines {
> + my ($prefix, $string) = @_;
> + $string =~ s/^/$prefix/mg;
> + return $string;
> +}
> +
> +=item comment_lines ( STRING )
> +
> +Comments lines following core.commentchar configuration.
> +
> +=cut
> +
> +sub comment_lines {
> + my $comment_line_char = config("core.commentchar") || '#';
> + return prefix_lines("$comment_line_char ", @_);
> +}
> +

This makes it appear as if comment_lines can take arbitrary number
of strings as its arguments (because the outer caller just passes @_
thru), but in fact because prefix_lines ignores anything other than
$_[0] and $_[1], only the first parameter given to comment_lineS sub
is inspected for lines in it and the prefix-char prefixed at the
beginning of each of them.

Which is not a great interface, as it is quite misleading.

Perhaps

prefix_lines("#", join("\n", @_));

or something like that may make it less confusing.

[PATCH v5 01/16] Git.pm: add subroutines for commenting lines

[Vasco Almeida]

Add subroutines prefix_lines and comment_lines.

Signed-off-by: Vasco Almeida 
---
 perl/Git.pm | 23 +++
 1 file changed, 23 insertions(+)

diff --git a/perl/Git.pm b/perl/Git.pm
index b2732822a..17be59fb7 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -1438,6 +1438,29 @@ sub END {
 
 } # %TEMP_* Lexical Context
 
+=item prefix_lines ( PREFIX, STRING )
+
+Prefixes lines in C with C.
+
+=cut
+
+sub prefix_lines {
+   my ($prefix, $string) = @_;
+   $string =~ s/^/$prefix/mg;
+   return $string;
+}
+
+=item comment_lines ( STRING )
+
+Comments lines following core.commentchar configuration.
+
+=cut
+
+sub comment_lines {
+   my $comment_line_char = config("core.commentchar") || '#';
+   return prefix_lines("$comment_line_char ", @_);
+}
+
 =back
 
 =head1 ERROR HANDLING
-- 
2.11.0.rc0.23.g8236252