[PATCH] Improve function dir.c:trim_trailing_spaces()

Move backwards from the end of the string (more efficient for
lines which do not have trailing spaces or have just a couple).
Slightly more rare occurrences of 'text  \    ' with a backslash
in between spaces are handled correctly.
Namely, the code in 8ba87adad6 does not reset 'last_space' when
a backslash is encountered and the above line stays intact as
a result
---
How about trailing tabs?

 dir.c | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/dir.c b/dir.c
index eb6f581..3315eea 100644
--- a/dir.c
+++ b/dir.c

@@ -508,21 +508,18 @@ void clear_exclude_list(struct exclude_list *el)
 
 static void trim_trailing_spaces(char *buf)
 {
-	int i, last_space = -1, nr_spaces, len = strlen(buf);
-	for (i = 0; i < len; i++)
-		if (buf[i] == '\\')
-			i++;
-		else if (buf[i] == ' ') {
-			if (last_space == -1) {
-				last_space = i;
-				nr_spaces = 1;
-			} else
-				nr_spaces++;
-		} else
-			last_space = -1;
-
-	if (last_space != -1 && last_space + nr_spaces == len)
+	int i, last_space, bslash = 0, len = strlen(buf);
+
+	if (len == 0 || buf[len - 1] != ' ')
+		return;
+	for (i = len - 2; i >= 0 && buf[i] == ' '; i--) ;
+	last_space = i + 1;
+	for ( ; i >=0 && buf[i] == '\\'; i--) bslash ^= 1;
+
+	if (!bslash)
 		buf[last_space] = '\0';
+	else if (bslash && last_space < len - 1)
+		buf[last_space + 1] = '\0';
 }
 
 int add_excludes_from_file_to_list(const char *fname,

-- 
1.9.1

On Wed, May 28, 2014 at 04:45:57PM -0700, Pasha Bolokhov wrote:

Move backwards from the end of the string (more efficient for
lines which do not have trailing spaces or have just a couple).

The original code reads the string from left to right. In theory, that
means we could get away with not calling strlen() at all, over a
right-to-left that must start with a call to strlen().

That being said, I think we already have the length in the calling
function, so you could probably avoid the strlen() altogether, which
makes a right-to-left function more efficient.

However, I doubt it makes that much of a difference in practice, so
unless it's measurable, I would certainly go with the version that is
more readable (and correct, of course).

Slightly more rare occurrences of 'text  \    ' with a backslash
in between spaces are handled correctly.

Can you add a test for this?

Also, if you are refactoring this function, I think it makes sense to
check that:

  "foo\\ "

and

  "foo\\\ "

match "foo\" and "foo\ ", respectively (I think they do with your patch,
but it is a tricky case that is not immediately obvious).

-Peff

On Thu, May 29, 2014 at 1:13 PM, Jeff King [off-list ref] wrote:

On Wed, May 28, 2014 at 04:45:57PM -0700, Pasha Bolokhov wrote:

quoted

Move backwards from the end of the string (more efficient for
lines which do not have trailing spaces or have just a couple).

The original code reads the string from left to right. In theory, that
means we could get away with not calling strlen() at all, over a
right-to-left that must start with a call to strlen().

That being said, I think we already have the length in the calling
function, so you could probably avoid the strlen() altogether, which
makes a right-to-left function more efficient.

However, I doubt it makes that much of a difference in practice, so
unless it's measurable, I would certainly go with the version that is
more readable (and correct, of course).

    Sorry, just to recap, you would go with the existing version
(which needs correction), or with the one that is being suggested? (I
agree I can format the style a tiny bit better in the latter one)

     Tests should not be a big problem, although it's kind of clumsy
to test an internal function which does not really give any output
(you can only measure the outcome). Just again to stress, I have
tested both implementation extensively and the suggested new
implementation gives the correct answers for all your examples below
and all others. If I show this with explicit "t/" tests, will it
suffice then?

    Basically what I suggest is

-- either: improve the existing function such that it does correctly
that "text  \   " case, and also does not use 'strlen' since it anyway
moves left to right

-- or: use the new suggested implementation (and just brush the
formatting a bit), and perhaps borrow 'len' from the calling routine

And add tests in any case. What is the preference?

quoted

Slightly more rare occurrences of 'text  \    ' with a backslash
in between spaces are handled correctly.

Can you add a test for this?

Also, if you are refactoring this function, I think it makes sense to
check that:

  "foo\\ "

and

  "foo\\\ "

match "foo\" and "foo\ ", respectively (I think they do with your patch,
but it is a tricky case that is not immediately obvious).

-Peff

On Thu, May 29, 2014 at 02:34:33PM -0700, Pasha Bolokhov wrote:

quoted

However, I doubt it makes that much of a difference in practice, so
unless it's measurable, I would certainly go with the version that is
more readable (and correct, of course).

    Sorry, just to recap, you would go with the existing version
(which needs correction), or with the one that is being suggested? (I
agree I can format the style a tiny bit better in the latter one)

I actually think the original left-to-right is a little easier to
follow, but I do not feel strongly. I mainly meant "argue based on
readability and correctness, do not argue based on speed".

     Tests should not be a big problem, although it's kind of clumsy
to test an internal function which does not really give any output
(you can only measure the outcome). Just again to stress, I have
tested both implementation extensively and the suggested new
implementation gives the correct answers for all your examples below
and all others. If I show this with explicit "t/" tests, will it
suffice then?

Yes. I think specifically that you can extend the tests at the end of
t0008.

    Basically what I suggest is

-- either: improve the existing function such that it does correctly
that "text  \   " case, and also does not use 'strlen' since it anyway
moves left to right

-- or: use the new suggested implementation (and just brush the
formatting a bit), and perhaps borrow 'len' from the calling routine

And add tests in any case. What is the preference?

I'd be OK with either, though I have a slight preference for the first,
just because I find the "bslash ^= 1" bit of yours, while clever, a bit
hard to follow.

-Peff