Re: [PATCH] diff --no-index: reset temporary buffer lengths on directory iteration
On Wed, May 16, 2012 at 2:33 AM, René Scharfe <rene.scharfe@lsrfire.ath.cx> wrote:
Show 17 quoted lines
> 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 <bobbypowers@gmail.com>
>> ---
>> 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?
Show 39 quoted lines
>
>
>> 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.
Show 9 quoted lines
>
>
>>
>> + buffer1.len = len1;
>> + buffer2.len = len2;
>
>
> It's cleaner to use strbuf_setlen() instead of setting the len member
> directly.
Show 5 quoted lines
>
> 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
Show 9 quoted lines
>
>
>> +
>> if (i1 == p1.nr)
>> comp = 1;
>> else if (i2 == p2.nr)
>> -- 1.7.10.2
>>
>
Bobby Powers