[Svnmerge] [PATCH] Indented log patch modified v5

[Raman Gupta]

Giovanni Bajo wrote:
> Raman Gupta <rocketraman at fastmail.fm> wrote:
> 
>>> Note that I also fixed your indentation, and
>>> changed pop(lineslist) to lineslist.pop(). Is pop a recently added
>>> function that we should have a local replacement for?
> 
> list.pop has always been there, it surely doesn't need a local replacement like
> we do for the strip family.

Good.

> Uhm I saw this went through several iteration. I'm satisfied with the semantic
> of the function (as shown in the unittests), but I think it ought to be
> simplified a little. What are the cases in which "lines" does not end with \n?
> Since it comes from get_commit_log, I'm reasonably sure it will always contain
> a trailing newline. This said:
> 
>>>> def prefix_lines(prefix, lines):
> ...     assert lines[-1] == "\n"    # ambiguous otherwise, is "aa" a line?
> ...     return prefix + lines[:-1].replace("\n", "\n"+prefix) + "\n"
> ...
>>>> prefix_lines("zz", "\n")
> 'zz\n'
>>>> prefix_lines("zz", "foo\n")
> 'zzfoo\n'
>>>> prefix_lines("zz", "\nfoo\nbar\n")
> 'zz\nzzfoo\nzzbar\n'
> 
> Then you can call prefix_lines directly with "message". Am I missing something?

If we knew prefix_lines was never going to be called for anything else
in the future, I would agree. But I think for the marginal extra
complexity, we should keep the function handling both the newline
terminated case and the non-terminated case. At some point, someone may
want to call it with non-newline terminated text.

But I agree about simplifying it -- how about this instead:

>>> def prefix_lines(prefix, lines):
...     return re.compile(r"^(.*)$(\n?)", re.M) \
                    .sub(prefix + r"\1\2", lines)
...
>>> prefix_lines("zz", "foo")
'zzfoo'
>>> prefix_lines("zz", "foo\n")
'zzfoo\n'
>>> prefix_lines("zz", "foo\nbar")
'zzfoo\nzzbar'
>>> prefix_lines("zz", "foo\nbar\n")
'zzfoo\nzzbar\n'

>>> +        if len(message) > 0:
> 
> aka "if message".

Ok, I'll change that in the next version of the patch.

>>> +            logs.append(prefix_lines(LOG_LINE_PREFIX, \
>>> +                                     rstrip(message, "\n")) + "\n")
>>> +            for match in LOG_SEPARATOR_RE.findall(message):
> 
> Given this prefix, will LOG_SEPARATOR_RE still work? Current definition is:
> 
> LOG_SEPARATOR_RE = re.compile('^((%s)+)' % re.escape(LOG_SEPARATOR),
> re.MULTILINE)
> 
> So it seems it expects the dots to be at the beginning of the line.

It will still work because the longest separator in the searched log
message will still be at the beginning of the line. We are not indenting
the separator, only the contents.

Cheers,
Raman