git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH-v2/RFC 3/6] xutils: fix ignore-all-space on incomplete line

From
Junio C Hamano <gitster@pobox.com>
Date
Aug 23, 2009, 07:57 UTC
Message-ID
<7vvdkfx8rl.fsf@alter.siamese.dyndns.org>
In-Reply-To
<1250999357-10827-3-git-send-email-git@tbfowler.name>
Thell Fowler <git@tbfowler.name> writes:
Show 10 quoted lines
> @@ -191,12 +191,14 @@ int xdl_recmatch(const char *l1, long s1, const char *l2, long s2, long flags)
>  	int i1, i2;
>  
>  	if (flags & XDF_IGNORE_WHITESPACE) {
> -		for (i1 = i2 = 0; i1 < s1 && i2 < s2; ) {
> +		for (i1 = i2 = 0; i1 < s1 || i2 < s2; ) {
>  			if (isspace(l1[i1]))
> -				while (isspace(l1[i1]) && i1 < s1)
> +				while ((isspace(l1[i1]) && i1 < s1)
> +						|| (i1 + 1 == s1 && l1[s1] != '\n'))

This is wrong. If you ran out l1/s1/i1 but you still have remaining characters in l2/s2/i2, you do not want to even look at l1[i1].

You can fudge this by sprinkling more "(i1 < s1) &&" in many places (and reordering how your inner while() loop checks (i1 < s1) and l1[i1]), but I do not think that is the right direction.

The thing is, the loop control in this function is extremely hard to read to begin with, and now it is "if we haven't run out both", the complexity seeps into the inner logic.

How about doing it like this patch instead? This counterproposal replaces your 3 patches starting from [3/6].

-- >8 --
Subject: xutils: Fix xdl_recmatch() on incomplete lines

Thell Fowler noticed that various "ignore whitespace" options to git diff does not work well with whitespace glitches on an incomplete line.

The loop control of this function incorrectly handled incomplete lines, and it was extremely difficult to follow. This restructures the loops for three variants of "ignore whitespace" logic.

The basic idea of the re-written logic is this.
 - An initial loop runs while the characters from both strings we are
   looking at match.  We declare unmatch immediately when we find
   something that does not match and return false from the loop.  And we
   break out of the loop if we ran out of either side of the string.
   The way we skip spaces inside this loop varies depending on the style
   of ignoring whitespaces.
 - After the loop, the lines can match only if the remainder consists of
   nothing but whitespaces.  This part of the logic is shared across all
   three styles.
The new code is more obvious and should be much easier to follow.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 xdiff/xutils.c |  111 +++++++++++++++++++++++++++++++++++++++-----------------
 1 files changed, 77 insertions(+), 34 deletions(-)
diff --git a/xdiff/xutils.c b/xdiff/xutils.c
index 9411fa9..dd8b7e7 100644
--- a/xdiff/xutils.c
+++ b/xdiff/xutils.c
@@ -186,50 +186,93 @@ long xdl_guess_lines(mmfile_t *mf) {
 	return nl + 1;
 }
 
+static int remainder_all_ws(const char *l1, const char *l2,
+			    int i1, int i2, long s1, long s2)
+{
+	if (i1 < s1) {
+		while (i1 < s1 && isspace(l1[i1]))
+			i1++;
+		return (s1 == i1);
+	}
+	if (i2 < s2) {
+		while (i2 < s2 && isspace(l2[i2]))
+			i2++;
+		return (s2 == i2);
+	}
+	return 1;
+}
+
 int xdl_recmatch(const char *l1, long s1, const char *l2, long s2, long flags)
 {
-	int i1, i2;
+	int i1 = 0, i2 = 0;
 
 	if (flags & XDF_IGNORE_WHITESPACE) {
-		for (i1 = i2 = 0; i1 < s1 && i2 < s2; ) {
-			if (isspace(l1[i1]))
-				while (isspace(l1[i1]) && i1 < s1)
-					i1++;
-			if (isspace(l2[i2]))
-				while (isspace(l2[i2]) && i2 < s2)
-					i2++;
-			if (i1 < s1 && i2 < s2 && l1[i1++] != l2[i2++])
-				return 0;
+		while (1) {
+			while (i1 < s1 && isspace(l1[i1]))
+				i1++;
+			while (i2 < s2 && isspace(l2[i2]))
+				i2++;
+			if (i1 < s1 && i2 < s2) {
+				if (l1[i1++] != l2[i2++])
+					return 0;
+				continue;
+			}
+			break;
 		}
-		return (i1 >= s1 && i2 >= s2);
+
+		/*
+		 * we ran out one side; the remaining side must be all
+		 * whitespace to match.
+		 */
+		return remainder_all_ws(l1, l2, i1, i2, s1, s2);
 	} else if (flags & XDF_IGNORE_WHITESPACE_CHANGE) {
-		for (i1 = i2 = 0; i1 < s1 && i2 < s2; ) {
-			if (isspace(l1[i1])) {
-				if (!isspace(l2[i2]))
+		while (1) {
+			if (i1 < s1 && i2 < s2) {
+				/* Skip matching spaces */
+				if (isspace(l1[i1]) && isspace(l2[i2])) {
+					while (i1 < s1 && isspace(l1[i1]))
+						i1++;
+					while (i2 < s2 && isspace(l2[i2]))
+						i2++;
+				}
+			}
+			if (i1 < s1 && i2 < s2) {
+				/*
+				 * We still have both sides; do they match?
+				 */
+				if (l1[i1++] != l2[i2++])
 					return 0;
-				while (isspace(l1[i1]) && i1 < s1)
-					i1++;
-				while (isspace(l2[i2]) && i2 < s2)
-					i2++;
-			} else if (l1[i1++] != l2[i2++])
-				return 0;
+				continue;
+			}
+			break;
 		}
-		return (i1 >= s1 && i2 >= s2);
+
+		/*
+		 * If we do not want -b to imply --ignore-space-at-eol
+		 * then you would need to add this:
+		 *
+		 * if (!(flags & XDF_IGNORE_WHITESPACE_AT_EOL))
+		 *	return (s1 <= i1 && s2 <= i2);
+		 *
+		 */
+
+		/*
+		 * we ran out one side; the remaining side must be all
+		 * whitespace to match.
+		 */
+		return remainder_all_ws(l1, l2, i1, i2, s1, s2);
+
 	} else if (flags & XDF_IGNORE_WHITESPACE_AT_EOL) {
-		for (i1 = i2 = 0; i1 < s1 && i2 < s2; ) {
-			if (l1[i1] != l2[i2]) {
-				while (i1 < s1 && isspace(l1[i1]))
-					i1++;
-				while (i2 < s2 && isspace(l2[i2]))
-					i2++;
-				if (i1 < s1 || i2 < s2)
-					return 0;
-				return 1;
-			}
-			i1++;
-			i2++;
+		while (1) {
+			if (i1 < s1 && i2 < s2 && l1[i1++] == l2[i2++])
+				continue;
+			break;
 		}
-		return i1 >= s1 && i2 >= s2;
+		/*
+		 * we ran out one side; the remaining side must be all
+		 * whitespace to match.
+		 */
+		return remainder_all_ws(l1, l2, i1, i2, s1, s2);
 	} else
 		return s1 == s2 && !memcmp(l1, l2, s1);
 }
Previous: Thell FowlerNext: Nanako Shiraishi
Message 15 of 63 in “Help/Advice needed on diff bug in xutils.c”
  1. Thell FowlerAug 4, 2009
  2. Johannes SchindelinAug 5, 2009
  3. Thell FowlerAug 10, 2009
  4. Add diff tests for trailing-space and now newlineThell Fowler, Aug 12, 2009
  5. 0/6 Series to correct xutils incomplete line handling.Thell Fowler, Aug 19, 2009
  6. Thell FowlerAug 21, 2009
  7. Alex RiesenAug 21, 2009
  8. Thell FowlerAug 22, 2009
  9. 0/6 improvements for trailing-space processing on incomplete linesThell Fowler, Aug 23, 2009
  10. 1/6 Add supplemental test for trailing-whitespace on incomplete linesThell Fowler, Aug 23, 2009
  11. 2/6 xutils: fix hash with whitespace on incomplete lineThell Fowler, Aug 23, 2009
  12. Junio C HamanoAug 23, 2009
  13. Thell FowlerAug 23, 2009
  14. 3/6 xutils: fix ignore-all-space on incomplete lineThell Fowler, Aug 23, 2009
  15. Junio C HamanoAug 23, 2009
  16. Nanako ShiraishiAug 23, 2009
  17. Junio C HamanoAug 23, 2009
  18. Nanako ShiraishiAug 23, 2009
  19. Junio C HamanoAug 23, 2009
  20. Thell FowlerAug 23, 2009
  21. Junio C HamanoAug 23, 2009
  22. Teach mailinfo to ignore everything before -- >8 -- markNicolas Sebrecht, Aug 24, 2009
  23. Junio C HamanoAug 24, 2009
  24. Junio C HamanoAug 24, 2009
  25. Re: Teach mailinfo to ignore everything before -- >8 -- markNicolas Sebrecht, Aug 24, 2009
  26. Junio C HamanoAug 24, 2009
  27. Re: Teach mailinfo to ignore everything before -- >8 -- markNicolas Sebrecht, Aug 24, 2009
  28. Don ZickusAug 24, 2009
  29. Junio C HamanoAug 24, 2009
  30. Nanako ShiraishiAug 24, 2009
  31. Re: Teach mailinfo to ignore everything before -- >8 -- markNicolas Sebrecht, Aug 24, 2009
  32. Re: Teach mailinfo to ignore everything before -- >8 -- markNicolas Sebrecht, Aug 24, 2009
  33. Junio C HamanoAug 24, 2009
  34. Nicolas SebrechtAug 25, 2009
  35. Junio C HamanoAug 26, 2009
  36. Junio C HamanoAug 26, 2009
  37. Re: Teach mailinfo to ignore everything before -- >8 -- markNicolas Sebrecht, Aug 26, 2009
  38. Jakub NarebskiAug 26, 2009
  39. Johannes SchindelinAug 26, 2009
  40. Junio C HamanoAug 27, 2009
  41. Johannes SchindelinAug 27, 2009
  42. Junio C HamanoAug 26, 2009
  43. Re: Teach mailinfo to ignore everything before -- >8 -- markNicolas Sebrecht, Aug 26, 2009
  44. Nanako ShiraishiAug 24, 2009
  45. Thell FowlerAug 23, 2009
  46. Junio C HamanoAug 23, 2009
  47. Thell FowlerAug 23, 2009
  48. Junio C HamanoAug 23, 2009
  49. Thell FowlerAug 24, 2009
  50. Junio C HamanoAug 24, 2009
  51. Thell FowlerAug 24, 2009
  52. Thell FowlerAug 25, 2009
  53. 4/6 xutils: fix ignore-space-change on incomplete lineThell Fowler, Aug 23, 2009
  54. 5/6 xutils: fix ignore-space-at-eol on incomplete lineThell Fowler, Aug 23, 2009
  55. 6/6 t4015: add tests for trailing-space on incomplete lineThell Fowler, Aug 23, 2009
  56. 1/6 Add supplemental test for trailing-whitespace on incomplete lines.Thell Fowler, Aug 19, 2009
  57. 2/6 Make xdl_hash_record_with_whitespace ignore eofThell Fowler, Aug 19, 2009
  58. 3/6 Make diff -w handle trailing-spaces on incomplete lines.Thell Fowler, Aug 19, 2009
  59. Thell FowlerAug 20, 2009
  60. 4/6 Make diff -b handle trailing-spaces on incomplete lines.Thell Fowler, Aug 19, 2009
  61. 5/6 Make diff --ignore-space-at-eol handle incomplete lines.Thell Fowler, Aug 19, 2009
  62. 6/6 Add diff tests for trailing-space on incomplete linesThell Fowler, Aug 19, 2009
  63. Junio C HamanoAug 26, 2009

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.