[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH] truncate EDITOR_EOF_PREFIX with previous EOL

From: <kfogel_at_collab.net>
Date: 2005-07-19 18:11:52 CEST

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 :-).


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

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.