Re: [PATCH] diff --no-index: reset temporary buffer lengths on directory iteration
- From
Bobby Powers <bobbypowers@gmail.com>
- Date
- May 16, 2012, 14:20 UTC
- Message-ID
- <CAArOQ2UNJyBHNa5_+G+J0bihCEOxPebey8-EkcoXDyeR1wM+yw@mail.gmail.com>
- In-Reply-To
- <4FB34A42.60300@lsrfire.ath.cx>
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?
Yup, will do.
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.
Done.
Show 9 quoted lines
> > >> >> + buffer1.len = len1; >> + buffer2.len = len2; > > > It's cleaner to use strbuf_setlen() instead of setting the len member > directly.
Done.
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 >> >