Olleg Samoylov <olleg@mipt.ru> writes:
> In log message wroten by editor always added trailing eol. This patch remove it.
>
> --
> Olleg Samoylov
>
> [[[
> * subversion/clients/cmdline/util.c
> (truncate_buffer_at_prefix): Remove trailing eol from log message.
> ]]]
There are several unrelated problems you could be solving here. It
would be best to describe the problem precisely.
* Problem (1) is that some text editors always add a trailing eol. If
you don't want the trailing eol, the solution is not to change
Subversion, but to use a different editor (or configure your editor
differently, or write a script to fix up the message in a file
afterwards, or whatever.) I'm using "eol" to mean "whatever
sequence of characters represents end-of-line on the client system",
by the way.
* Problem (2) is that the Subversion command-line client
unconditionally adds a trailing eol when it *prints* log messages,
to make sure that multiple logs in sequence look pretty. Changing
this behavior is possible, but would need to be done carefully: we
can't just stop adding the eol, instead we'd have to refrain from
adding it when there is already at least one eol present anyway.
Your patch doesn't do this.
* Problem (3) is that when you write a log message by allowing
Subversion to invoke an interactive editor, Subversion places a list
of changed files at the end of the buffer, and promises to
automatically remove that list. However, when it removes the list,
it still leaves the eol that preceded the list. Since having a
trailing eol on a log message is normally considered desirable, I
think this is a good default behavior for Subversion, especially
since it's easy to work around -- just remove the change list using
your editor, making sure to remove the preceding eol as well (this
assumes your editor doesn't have Problem (1), of course).
Based on where you made the patch, I guess you are trying to solve
problem (3) only. As explained above, I don't think this change is
necessary. However, here is a review of the change:
First, thank you for well-formatted log message!
The actual behavior change is a bit more sophisticated than what you
wrote, however. Saying that you are removing "the trailing eol" is
unclear, since at the time the input goes into the function, that eol
is not trailing (it is somewhere in the middle of the string). Say
instead "Avoid leaving a trailing eol on the result.", or something
like that.
Regarding the patch itself:
> Index: subversion/clients/cmdline/util.c
> ===================================================================
> --- subversion/clients/cmdline/util.c (revision 15361)
> +++ subversion/clients/cmdline/util.c (working copy)
> @@ -417,6 +417,14 @@
> || (*(substring - 1) == '\r')
> || (*(substring - 1) == '\n'))
> {
> + /* Remove previous eol. */
> + while (substring != buffer)
> + if ((*(substring - 1) == '\r')
> + || (*(substring - 1) == '\n'))
> + substring--;
> + else + break;
> +
> *substring = '\0';
> if (new_len)
> *new_len = substring - buffer;
There was a formatting problem, I think: the "+" right after the
"else" should have begun a new line, right?
Also, this algorithm is not sophisticated enough. What if you
encounter '\r' or '\n' as independent characters in a log message, not
as part of an eol sequence (on Windows at least, '\n' can stand alone
outside an eol sequence; on Unix it's always eol, of course).
Of course, the current code has the same bug, which is not your fault
at all.
Finally, you changed the behavior of truncate_buffer_at_prefix(), but
you did not change its doc string. Don't make the doc string lie :-).
Best,
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Jul 19 19:03:02 2005