[PATCH] diff --no-index: reset temporary buffer lengths on directory iteration

Subsystems: the rest

DORMANTno replies

4 messages, 2 authors, 2016-06-15 · open the first message on its own page

[PATCH] diff --no-index: reset temporary buffer lengths on directory iteration

From: Bobby Powers <hidden>
Date: 2016-06-15 22:53:50

Commit 875b91b3 introduced a regression when using diff --no-index
with directories.  When iterating through a directory, the switch to
strbuf from heap-allocated char arrays caused paths to form like
'dir/file1', 'dir/file1file2', rather than 'dir/file1', 'dir/file2' as
expected.  By resetting the length on each iteration (but not
buf.alloc), we avoid this.

Signed-off-by: Bobby Powers <redacted>
---
 diff-no-index.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/diff-no-index.c b/diff-no-index.c
index b44473e..bec3ea4 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c

@@ -67,7 +67,7 @@ static int queue_diff(struct diff_options *o,
 		struct strbuf buffer2 = STRBUF_INIT;
 		struct string_list p1 = STRING_LIST_INIT_DUP;
 		struct string_list p2 = STRING_LIST_INIT_DUP;
-		int i1, i2, ret = 0;
+		int len1 = 0, len2 = 0, i1, i2, ret = 0;
 
 		if (name1 && read_directory(name1, &p1))
 			return -1;

@@ -80,18 +80,23 @@ static int queue_diff(struct diff_options *o,
 			strbuf_addstr(&buffer1, name1);
 			if (buffer1.len && buffer1.buf[buffer1.len - 1] != '/')
 				strbuf_addch(&buffer1, '/');
+			len1 = buffer1.len;
 		}
 
 		if (name2) {
 			strbuf_addstr(&buffer2, name2);
 			if (buffer2.len && buffer2.buf[buffer2.len - 1] != '/')
 				strbuf_addch(&buffer2, '/');
+			len2 = buffer2.len;
 		}
 
 		for (i1 = i2 = 0; !ret && (i1 < p1.nr || i2 < p2.nr); ) {
 			const char *n1, *n2;
 			int comp;
 
+			buffer1.len = len1;
+			buffer2.len = len2;
+
 			if (i1 == p1.nr)
 				comp = 1;
 			else if (i2 == p2.nr)

-- 
1.7.10.2

Re: [PATCH] diff --no-index: reset temporary buffer lengths on directory iteration

From: René Scharfe <hidden>
Date: 2016-06-15 22:53:50

Am 16.05.2012 06:14, schrieb Bobby Powers:

Commit 875b91b3 introduced a regression when using diff --no-index
with directories.  When iterating through a directory, the switch to
strbuf from heap-allocated char arrays caused paths to form like
'dir/file1', 'dir/file1file2', rather than 'dir/file1', 'dir/file2' as
expected.  By resetting the length on each iteration (but not
buf.alloc), we avoid this.

Signed-off-by: Bobby Powers <redacted>
---
 diff-no-index.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Nice catch!  Could you please also add a test case so that we can be 
sure a similar bug is not reintroduced later?

quoted hunk

diff --git a/diff-no-index.c b/diff-no-index.c
index b44473e..bec3ea4 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c

@@ -67,7 +67,7 @@ static int queue_diff(struct diff_options *o,
 		struct strbuf buffer2 = STRBUF_INIT;
 		struct string_list p1 = STRING_LIST_INIT_DUP;
 		struct string_list p2 = STRING_LIST_INIT_DUP;
-		int i1, i2, ret = 0;
+		int len1 = 0, len2 = 0, i1, i2, ret = 0;

 		if (name1 && read_directory(name1, &p1))
 			return -1;

@@ -80,18 +80,23 @@ static int queue_diff(struct diff_options *o,
 			strbuf_addstr(&buffer1, name1);
 			if (buffer1.len && buffer1.buf[buffer1.len - 1] != '/')
 				strbuf_addch(&buffer1, '/');
+			len1 = buffer1.len;
 		}

 		if (name2) {
 			strbuf_addstr(&buffer2, name2);
 			if (buffer2.len && buffer2.buf[buffer2.len - 1] != '/')
 				strbuf_addch(&buffer2, '/');
+			len2 = buffer2.len;
 		}

 		for (i1 = i2 = 0; !ret && (i1 < p1.nr || i2 < p2.nr); ) {
 			const char *n1, *n2;
 			int comp;

If you declare len1 and len2 right here at the start of the loop and 
reset the strbufs at its end, you wouldn't have to initialize them to 
zero and they'd have the right scope for their task.

Using type size_t, the type used in struct strbuf, is more correct.

+			buffer1.len = len1;
+			buffer2.len = len2;

It's cleaner to use strbuf_setlen() instead of setting the len member 
directly.

Looking at the code, I think the strbufs are never freed and the 
strbuf_reset() calls after the loop should be replaced by ones to 
strbuf_release() in order to avoid leaking.  This is a different issue, 
but would be nice to squash as well.

+
 			if (i1 == p1.nr)
 				comp = 1;
 			else if (i2 == p2.nr)
-- 1.7.10.2

Re: [PATCH] diff --no-index: reset temporary buffer lengths on directory iteration

From: Bobby Powers <hidden>
Date: 2016-06-15 22:53:50

On Wed, May 16, 2012 at 2:33 AM, René Scharfe
[off-list ref] wrote:

Am 16.05.2012 06:14, schrieb Bobby Powers:

quoted

Commit 875b91b3 introduced a regression when using diff --no-index
with directories.  When iterating through a directory, the switch to
strbuf from heap-allocated char arrays caused paths to form like
'dir/file1', 'dir/file1file2', rather than 'dir/file1', 'dir/file2' as
expected.  By resetting the length on each iteration (but not
buf.alloc), we avoid this.

Signed-off-by: Bobby Powers <redacted>
---
 diff-no-index.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)


Nice catch!  Could you please also add a test case so that we can be sure a
similar bug is not reintroduced later?

Yup, will do.

quoted

diff --git a/diff-no-index.c b/diff-no-index.c
index b44473e..bec3ea4 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c

@@ -67,7 +67,7 @@ static int queue_diff(struct diff_options *o,

               struct strbuf buffer2 = STRBUF_INIT;
               struct string_list p1 = STRING_LIST_INIT_DUP;
               struct string_list p2 = STRING_LIST_INIT_DUP;
-               int i1, i2, ret = 0;
+               int len1 = 0, len2 = 0, i1, i2, ret = 0;

               if (name1 && read_directory(name1, &p1))
                       return -1;

@@ -80,18 +80,23 @@ static int queue_diff(struct diff_options *o,

                       strbuf_addstr(&buffer1, name1);
                       if (buffer1.len && buffer1.buf[buffer1.len - 1] !=
'/')
                               strbuf_addch(&buffer1, '/');
+                       len1 = buffer1.len;
               }

               if (name2) {
                       strbuf_addstr(&buffer2, name2);
                       if (buffer2.len && buffer2.buf[buffer2.len - 1] !=
'/')
                               strbuf_addch(&buffer2, '/');
+                       len2 = buffer2.len;
               }

               for (i1 = i2 = 0; !ret && (i1 < p1.nr || i2 < p2.nr); ) {
                       const char *n1, *n2;
                       int comp;


If you declare len1 and len2 right here at the start of the loop and reset
the strbufs at its end, you wouldn't have to initialize them to zero and
they'd have the right scope for their task.

Nope, thats not what we want actually.  At the start of the first
iteration, the buffers have directory names in them, like 'dir_1/' and
'dir_2/'.  The loop appends the directory contents to the directory
path, upon each iteration we only want to clear the last filename from
the buffer, not the entire buffer.

Using type size_t, the type used in struct strbuf, is more correct.

Done.

quoted

+                       buffer1.len = len1;
+                       buffer2.len = len2;


It's cleaner to use strbuf_setlen() instead of setting the len member
directly.

Done.

Looking at the code, I think the strbufs are never freed and the
strbuf_reset() calls after the loop should be replaced by ones to
strbuf_release() in order to avoid leaking.  This is a different issue, but
would be nice to squash as well.

Yup, I noticed that as well.  I'll send a separate patch out for it.

Thanks for the review!  I'll send a revised patch out momentarily.

yours,
Bobby

quoted

+
                       if (i1 == p1.nr)
                               comp = 1;
                       else if (i2 == p2.nr)
-- 1.7.10.2

[PATCH v2] diff --no-index: reset temporary buffer lengths on directory iteration

From: Bobby Powers <hidden>
Date: 2016-06-15 22:53:50

Commit 875b91b3 introduced a regression when using diff --no-index
with directories.  When iterating through a directory, the switch to
strbuf from heap-allocated char arrays caused paths to form like
'dir/file1', 'dir/file1file2', rather than 'dir/file1', 'dir/file2' as
expected.  By resetting the length on each iteration (but not
buf.alloc), we avoid this.

Signed-off-by: Bobby Powers <redacted>
---
 diff-no-index.c          |  6 ++++++
 t/t4053-diff-no-index.sh | 19 +++++++++++++++++++
 2 files changed, 25 insertions(+)
 create mode 100755 t/t4053-diff-no-index.sh

diff --git a/diff-no-index.c b/diff-no-index.c
index b44473e..3080b66 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c

@@ -68,6 +68,7 @@ static int queue_diff(struct diff_options *o,
 		struct string_list p1 = STRING_LIST_INIT_DUP;
 		struct string_list p2 = STRING_LIST_INIT_DUP;
 		int i1, i2, ret = 0;
+		size_t len1 = 0, len2 = 0;
 
 		if (name1 && read_directory(name1, &p1))
 			return -1;

@@ -80,18 +81,23 @@ static int queue_diff(struct diff_options *o,
 			strbuf_addstr(&buffer1, name1);
 			if (buffer1.len && buffer1.buf[buffer1.len - 1] != '/')
 				strbuf_addch(&buffer1, '/');
+			len1 = buffer1.len;
 		}
 
 		if (name2) {
 			strbuf_addstr(&buffer2, name2);
 			if (buffer2.len && buffer2.buf[buffer2.len - 1] != '/')
 				strbuf_addch(&buffer2, '/');
+			len2 = buffer2.len;
 		}
 
 		for (i1 = i2 = 0; !ret && (i1 < p1.nr || i2 < p2.nr); ) {
 			const char *n1, *n2;
 			int comp;
 
+			strbuf_setlen(&buffer1, len1);
+			strbuf_setlen(&buffer2, len2);
+
 			if (i1 == p1.nr)
 				comp = 1;
 			else if (i2 == p2.nr)

diff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh
new file mode 100755
index 0000000..4dc8c67
--- /dev/null
+++ b/t/t4053-diff-no-index.sh

@@ -0,0 +1,19 @@
+#!/bin/sh
+
+test_description='diff --no-index'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	mkdir a &&
+	mkdir b &&
+	echo 1 >a/1 &&
+	echo 2 >a/2
+'
+
+test_expect_success 'git diff --no-index directories' '
+	git diff --no-index a b >cnt
+	test $? = 1 && test_line_count = 14 cnt
+'
+
+test_done

-- 
1.7.10.2.521.g8ddb639

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help