From: Bobby Powers Date: Wed, 16 May 2012 14:20:32 GMT Subject: Re: [PATCH] diff --no-index: reset temporary buffer lengths on directory iteration Message-ID: In-Reply-To: <4FB34A42.60300@lsrfire.ath.cx> On Wed, May 16, 2012 at 2:33 AM, René Scharfe wrote: > 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 >> --- >>  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. > > >> 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. > > >> >> +                       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 > > >> + >>                        if (i1 == p1.nr) >>                                comp = 1; >>                        else if (i2 == p2.nr) >> -- 1.7.10.2 >> >