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

["Jakub Narębski" <jnareb@gmail.com>]

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 <vascomalmeida@sapo.pt> 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