{"thread":{"id":"30535","subject":"[PATCH] diff --no-index: reset temporary buffer lengths on directory iteration","startedAt":"2012-05-16T04:14:53Z","lastAt":"2012-05-16T14:28:31Z","messageCount":4,"participants":["Bobby Powers","René Scharfe"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"191573","messageId":"1337141693-3515-1-git-send-email-bobbypowers@gmail.com","threadId":"30535","inReplyTo":null,"subject":"[PATCH] diff --no-index: reset temporary buffer lengths on directory iteration","fromName":"Bobby Powers","fromEmail":"bobbypowers@gmail.com","sentAt":"2012-05-16T04:14:53Z","receivedAt":"2012-05-16T04:14:53Z","isPatch":true,"sender":{"key":"bobbypowers@gmail.com","avatar":"https://avatars.githubusercontent.com/u/854918?v=4"},"body":"Commit 875b91b3 introduced a regression when using diff --no-index\nwith directories.  When iterating through a directory, the switch to\nstrbuf from heap-allocated char arrays caused paths to form like\n'dir/file1', 'dir/file1file2', rather than 'dir/file1', 'dir/file2' as\nexpected.  By resetting the length on each iteration (but not\nbuf.alloc), we avoid this.\n\nSigned-off-by: Bobby Powers <bobbypowers@gmail.com>\n---\n diff-no-index.c |    7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex b44473e..bec3ea4 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -67,7 +67,7 @@ static int queue_diff(struct diff_options *o,\n \t\tstruct strbuf buffer2 = STRBUF_INIT;\n \t\tstruct string_list p1 = STRING_LIST_INIT_DUP;\n \t\tstruct string_list p2 = STRING_LIST_INIT_DUP;\n-\t\tint i1, i2, ret = 0;\n+\t\tint len1 = 0, len2 = 0, i1, i2, ret = 0;\n \n \t\tif (name1 && read_directory(name1, &p1))\n \t\t\treturn -1;\n@@ -80,18 +80,23 @@ static int queue_diff(struct diff_options *o,\n \t\t\tstrbuf_addstr(&buffer1, name1);\n \t\t\tif (buffer1.len && buffer1.buf[buffer1.len - 1] != '/')\n \t\t\t\tstrbuf_addch(&buffer1, '/');\n+\t\t\tlen1 = buffer1.len;\n \t\t}\n \n \t\tif (name2) {\n \t\t\tstrbuf_addstr(&buffer2, name2);\n \t\t\tif (buffer2.len && buffer2.buf[buffer2.len - 1] != '/')\n \t\t\t\tstrbuf_addch(&buffer2, '/');\n+\t\t\tlen2 = buffer2.len;\n \t\t}\n \n \t\tfor (i1 = i2 = 0; !ret && (i1 < p1.nr || i2 < p2.nr); ) {\n \t\t\tconst char *n1, *n2;\n \t\t\tint comp;\n \n+\t\t\tbuffer1.len = len1;\n+\t\t\tbuffer2.len = len2;\n+\n \t\t\tif (i1 == p1.nr)\n \t\t\t\tcomp = 1;\n \t\t\telse if (i2 == p2.nr)\n-- \n1.7.10.2\n"},{"id":"191576","messageId":"4FB34A42.60300@lsrfire.ath.cx","threadId":"30535","inReplyTo":"1337141693-3515-1-git-send-email-bobbypowers@gmail.com","subject":"Re: [PATCH] diff --no-index: reset temporary buffer lengths on directory iteration","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2012-05-16T06:33:38Z","receivedAt":"2012-05-16T06:33:38Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 16.05.2012 06:14, schrieb Bobby Powers:\n> Commit 875b91b3 introduced a regression when using diff --no-index\n> with directories.  When iterating through a directory, the switch to\n> strbuf from heap-allocated char arrays caused paths to form like\n> 'dir/file1', 'dir/file1file2', rather than 'dir/file1', 'dir/file2' as\n> expected.  By resetting the length on each iteration (but not\n> buf.alloc), we avoid this.\n>\n> Signed-off-by: Bobby Powers <bobbypowers@gmail.com>\n> ---\n>  diff-no-index.c |    7 ++++++-\n>  1 file changed, 6 insertions(+), 1 deletion(-)\n\nNice catch!  Could you please also add a test case so that we can be \nsure a similar bug is not reintroduced later?\n\n> diff --git a/diff-no-index.c b/diff-no-index.c\n> index b44473e..bec3ea4 100644\n> --- a/diff-no-index.c\n> +++ b/diff-no-index.c\n> @@ -67,7 +67,7 @@ static int queue_diff(struct diff_options *o,\n>  \t\tstruct strbuf buffer2 = STRBUF_INIT;\n>  \t\tstruct string_list p1 = STRING_LIST_INIT_DUP;\n>  \t\tstruct string_list p2 = STRING_LIST_INIT_DUP;\n> -\t\tint i1, i2, ret = 0;\n> +\t\tint len1 = 0, len2 = 0, i1, i2, ret = 0;\n>\n>  \t\tif (name1 && read_directory(name1, &p1))\n>  \t\t\treturn -1;\n> @@ -80,18 +80,23 @@ static int queue_diff(struct diff_options *o,\n>  \t\t\tstrbuf_addstr(&buffer1, name1);\n>  \t\t\tif (buffer1.len && buffer1.buf[buffer1.len - 1] != '/')\n>  \t\t\t\tstrbuf_addch(&buffer1, '/');\n> +\t\t\tlen1 = buffer1.len;\n>  \t\t}\n>\n>  \t\tif (name2) {\n>  \t\t\tstrbuf_addstr(&buffer2, name2);\n>  \t\t\tif (buffer2.len && buffer2.buf[buffer2.len - 1] != '/')\n>  \t\t\t\tstrbuf_addch(&buffer2, '/');\n> +\t\t\tlen2 = buffer2.len;\n>  \t\t}\n>\n>  \t\tfor (i1 = i2 = 0; !ret && (i1 < p1.nr || i2 < p2.nr); ) {\n>  \t\t\tconst char *n1, *n2;\n>  \t\t\tint comp;\n\nIf you declare len1 and len2 right here at the start of the loop and \nreset the strbufs at its end, you wouldn't have to initialize them to \nzero and they'd have the right scope for their task.\n\nUsing type size_t, the type used in struct strbuf, is more correct.\n\n>\n> +\t\t\tbuffer1.len = len1;\n> +\t\t\tbuffer2.len = len2;\n\nIt's cleaner to use strbuf_setlen() instead of setting the len member \ndirectly.\n\nLooking at the code, I think the strbufs are never freed and the \nstrbuf_reset() calls after the loop should be replaced by ones to \nstrbuf_release() in order to avoid leaking.  This is a different issue, \nbut would be nice to squash as well.\n\n> +\n>  \t\t\tif (i1 == p1.nr)\n>  \t\t\t\tcomp = 1;\n>  \t\t\telse if (i2 == p2.nr)\n> -- 1.7.10.2\n>\n"},{"id":"191590","messageId":"CAArOQ2UNJyBHNa5_+G+J0bihCEOxPebey8-EkcoXDyeR1wM+yw@mail.gmail.com","threadId":"30535","inReplyTo":"4FB34A42.60300@lsrfire.ath.cx","subject":"Re: [PATCH] diff --no-index: reset temporary buffer lengths on directory iteration","fromName":"Bobby Powers","fromEmail":"bobbypowers@gmail.com","sentAt":"2012-05-16T14:20:32Z","receivedAt":"2012-05-16T14:20:32Z","isPatch":true,"sender":{"key":"bobbypowers@gmail.com","avatar":"https://avatars.githubusercontent.com/u/854918?v=4"},"body":"On Wed, May 16, 2012 at 2:33 AM, René Scharfe\n<rene.scharfe@lsrfire.ath.cx> wrote:\n> Am 16.05.2012 06:14, schrieb Bobby Powers:\n>\n>> Commit 875b91b3 introduced a regression when using diff --no-index\n>> with directories.  When iterating through a directory, the switch to\n>> strbuf from heap-allocated char arrays caused paths to form like\n>> 'dir/file1', 'dir/file1file2', rather than 'dir/file1', 'dir/file2' as\n>> expected.  By resetting the length on each iteration (but not\n>> buf.alloc), we avoid this.\n>>\n>> Signed-off-by: Bobby Powers <bobbypowers@gmail.com>\n>> ---\n>>  diff-no-index.c |    7 ++++++-\n>>  1 file changed, 6 insertions(+), 1 deletion(-)\n>\n>\n> Nice catch!  Could you please also add a test case so that we can be sure a\n> similar bug is not reintroduced later?\n\nYup, will do.\n\n>\n>\n>> diff --git a/diff-no-index.c b/diff-no-index.c\n>> index b44473e..bec3ea4 100644\n>> --- a/diff-no-index.c\n>> +++ b/diff-no-index.c\n>> @@ -67,7 +67,7 @@ static int queue_diff(struct diff_options *o,\n>>                struct strbuf buffer2 = STRBUF_INIT;\n>>                struct string_list p1 = STRING_LIST_INIT_DUP;\n>>                struct string_list p2 = STRING_LIST_INIT_DUP;\n>> -               int i1, i2, ret = 0;\n>> +               int len1 = 0, len2 = 0, i1, i2, ret = 0;\n>>\n>>                if (name1 && read_directory(name1, &p1))\n>>                        return -1;\n>> @@ -80,18 +80,23 @@ static int queue_diff(struct diff_options *o,\n>>                        strbuf_addstr(&buffer1, name1);\n>>                        if (buffer1.len && buffer1.buf[buffer1.len - 1] !=\n>> '/')\n>>                                strbuf_addch(&buffer1, '/');\n>> +                       len1 = buffer1.len;\n>>                }\n>>\n>>                if (name2) {\n>>                        strbuf_addstr(&buffer2, name2);\n>>                        if (buffer2.len && buffer2.buf[buffer2.len - 1] !=\n>> '/')\n>>                                strbuf_addch(&buffer2, '/');\n>> +                       len2 = buffer2.len;\n>>                }\n>>\n>>                for (i1 = i2 = 0; !ret && (i1 < p1.nr || i2 < p2.nr); ) {\n>>                        const char *n1, *n2;\n>>                        int comp;\n>\n>\n> If you declare len1 and len2 right here at the start of the loop and reset\n> the strbufs at its end, you wouldn't have to initialize them to zero and\n> they'd have the right scope for their task.\n\nNope, thats not what we want actually.  At the start of the first\niteration, the buffers have directory names in them, like 'dir_1/' and\n'dir_2/'.  The loop appends the directory contents to the directory\npath, upon each iteration we only want to clear the last filename from\nthe buffer, not the entire buffer.\n\n>\n> Using type size_t, the type used in struct strbuf, is more correct.\n\nDone.\n\n>\n>\n>>\n>> +                       buffer1.len = len1;\n>> +                       buffer2.len = len2;\n>\n>\n> It's cleaner to use strbuf_setlen() instead of setting the len member\n> directly.\n\nDone.\n\n>\n> Looking at the code, I think the strbufs are never freed and the\n> strbuf_reset() calls after the loop should be replaced by ones to\n> strbuf_release() in order to avoid leaking.  This is a different issue, but\n> would be nice to squash as well.\n\nYup, I noticed that as well.  I'll send a separate patch out for it.\n\nThanks for the review!  I'll send a revised patch out momentarily.\n\nyours,\nBobby\n\n>\n>\n>> +\n>>                        if (i1 == p1.nr)\n>>                                comp = 1;\n>>                        else if (i2 == p2.nr)\n>> -- 1.7.10.2\n>>\n>\n"},{"id":"191591","messageId":"1337178511-22980-1-git-send-email-bobbypowers@gmail.com","threadId":"30535","inReplyTo":"CAArOQ2UNJyBHNa5_+G+J0bihCEOxPebey8-EkcoXDyeR1wM+yw@mail.gmail.com","subject":"[PATCH v2] diff --no-index: reset temporary buffer lengths on directory iteration","fromName":"Bobby Powers","fromEmail":"bobbypowers@gmail.com","sentAt":"2012-05-16T14:28:31Z","receivedAt":"2012-05-16T14:28:31Z","isPatch":true,"sender":{"key":"bobbypowers@gmail.com","avatar":"https://avatars.githubusercontent.com/u/854918?v=4"},"body":"Commit 875b91b3 introduced a regression when using diff --no-index\nwith directories.  When iterating through a directory, the switch to\nstrbuf from heap-allocated char arrays caused paths to form like\n'dir/file1', 'dir/file1file2', rather than 'dir/file1', 'dir/file2' as\nexpected.  By resetting the length on each iteration (but not\nbuf.alloc), we avoid this.\n\nSigned-off-by: Bobby Powers <bobbypowers@gmail.com>\n---\n diff-no-index.c          |  6 ++++++\n t/t4053-diff-no-index.sh | 19 +++++++++++++++++++\n 2 files changed, 25 insertions(+)\n create mode 100755 t/t4053-diff-no-index.sh\n\ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex b44473e..3080b66 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -68,6 +68,7 @@ static int queue_diff(struct diff_options *o,\n \t\tstruct string_list p1 = STRING_LIST_INIT_DUP;\n \t\tstruct string_list p2 = STRING_LIST_INIT_DUP;\n \t\tint i1, i2, ret = 0;\n+\t\tsize_t len1 = 0, len2 = 0;\n \n \t\tif (name1 && read_directory(name1, &p1))\n \t\t\treturn -1;\n@@ -80,18 +81,23 @@ static int queue_diff(struct diff_options *o,\n \t\t\tstrbuf_addstr(&buffer1, name1);\n \t\t\tif (buffer1.len && buffer1.buf[buffer1.len - 1] != '/')\n \t\t\t\tstrbuf_addch(&buffer1, '/');\n+\t\t\tlen1 = buffer1.len;\n \t\t}\n \n \t\tif (name2) {\n \t\t\tstrbuf_addstr(&buffer2, name2);\n \t\t\tif (buffer2.len && buffer2.buf[buffer2.len - 1] != '/')\n \t\t\t\tstrbuf_addch(&buffer2, '/');\n+\t\t\tlen2 = buffer2.len;\n \t\t}\n \n \t\tfor (i1 = i2 = 0; !ret && (i1 < p1.nr || i2 < p2.nr); ) {\n \t\t\tconst char *n1, *n2;\n \t\t\tint comp;\n \n+\t\t\tstrbuf_setlen(&buffer1, len1);\n+\t\t\tstrbuf_setlen(&buffer2, len2);\n+\n \t\t\tif (i1 == p1.nr)\n \t\t\t\tcomp = 1;\n \t\t\telse if (i2 == p2.nr)\ndiff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh\nnew file mode 100755\nindex 0000000..4dc8c67\n--- /dev/null\n+++ b/t/t4053-diff-no-index.sh\n@@ -0,0 +1,19 @@\n+#!/bin/sh\n+\n+test_description='diff --no-index'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\tmkdir a &&\n+\tmkdir b &&\n+\techo 1 >a/1 &&\n+\techo 2 >a/2\n+'\n+\n+test_expect_success 'git diff --no-index directories' '\n+\tgit diff --no-index a b >cnt\n+\ttest $? = 1 && test_line_count = 14 cnt\n+'\n+\n+test_done\n-- \n1.7.10.2.521.g8ddb639\n"}]}