Hi Daniel.
Just as an FYI - there is absolutely nothing wrong with you pinging
your own patch.
You are definitely the best person to be taking control of your
submissions.
But also please realise that - just like you - "nearly" everyone else
is a volunteer of their own time to the Subversion Project.
Sometimes it just takes a little while for other people to get around
to looking at a patch submission.
As the Patch Manager it is my role within the project to ensure that a
patch submission doesn't fall through the cracks. (I.e. if you don't
ping your own patch.)
I normally ping the list for uncommented / unreviewed patches at about
a week after it is submitted or last commented on..
I'm not scolding you or anything like that....
It's more a case of just trying to make sue that your expectations of
the "timeliness" to having your patch submissions reviewed are "in-
line" with the "tradition" of how things normally happen within the
mailing list.
I'm certainly not telling you to stop pinging the list, I openly
encourage all patch submitters to take control of their own work - I
just don't want you to think that your submission(s) are being ignored
or that your work is not seen as being valuable by the project -
because neither of those are true - sometimes it just takes a little
while for a review to occur - especially when people are busy doing
their own contributions at the same time, during their volunteered time.
Gavin.
On 14/08/2009, at 15:27 , Daniel Näslund wrote:
> Ping! This patch hasn't been reviewed.
>
> On Sun, Aug 09, 2009 at 09:25:50PM +0200, Daniel Näslund wrote:
>> On Fri, Aug 07, 2009 at 01:23:51PM +0100, Stefan Sperling wrote:
>>>>> Maybe libsvn_client should be shielded from the unidiff specifics
>>>>> entirely, and just get original and modified hunk lines from
>>>>> libsvn_diff
>>>>> with the extra '+', '-' etc. already removed.
>>>>
>>>> Involves changing svn_hunk_t to use some other classification than
>>>> original_text and modified_text? Beeing lazy I would prefer not
>>>> having
>>>> to do that. :-)
>>>
>>> I'd still prefer a solution where libsvn_client conceptually
>>> ends up in a situation like:
>>>
>>> original_text = "foo"
>>> modified_text = "bar"
>>> latest_text = "foo"
>>>
>>> which would be much more easy to deal with instead of the current:
>>>
>>> original_text = "-foo"
>>> modified_text = "+bar"
>>> latest_text = "foo"
>>
>> I was lucky. There was no need to change anything in svn_hunk_t. Just
>> removing the leading '+', '-' and ' ' was enough.
>>>
>>> Say we'd want to add a parser for context diffs (diff -c) at some
>>> point.
>>> That would be easier if libsvn_client was already shielded from the
>>> specifics of the unidiff format.
>>
>> I agree.
>>
>>>> Now I am able to patch the simple example given in the issue
>>>> description
>>>> but when trying to create a file using this patch I get a conflict
>>>> (There is one empty line at the end of the patch) :
>>>>
>>>> Index: foo2
>>>> ===================================================================
>>>> --- foo2 (revision 0)
>>>> +++ foo2 (revision 0)
>>>> @@ -0,0 +1 @@
>>>> +test
>>>>
>>>> Is this an issue with the fuzz? If svn patch would allow at least
>>>> one
>>>> line of fuzz then this patch would work?
>>>>
>>>> Or do I need a way to find out when an empty line is there for
>>>> readability (as the blank line before Property changes on ...) or
>>>> when
>>>> the leading space has been chopped?
>>>
>>> There are two cases:
>>>
>>> 1) trailing empty line is part of the hunk's context
>>> 2) trailing empty line is part of the noise surrounding patches
>>> in the unidiff
>>>
>>> I guess we can assume that the number of lines of leading context
>>> in a hunk is equal to the number of lines of trailing context in
>>> a hunk. That should provide a way to resolve the ambiguity.
>>
>> I used another method. Original_lines in svn_hunk_t is the number of
>> lines with leading ' ' or '-'. I read the number at the beginning of
>> each hunk and decrease for each ' ' or '+'. If an empty line is
>> found -
>> it is added as long as the number of original_lines is not zero.
>>
>> In subversion/test/libsvn_diff/parse-diff-tests.c I rewrote the hunk
>> header. According to me it was wrong, am I wrong?
>>
>> Should I write a new test in subversion/tests/cmdline/patch-tests.py
>> with empty lines?
>>
>> [[[
>> Fix issue 3459 - svn patch does not tolerate empty lines.
>>
>> * subversion/libsvn_diff/parse-diff.c
>> (set_hunk_text): Removes leading character before setting hunk text.
>> (svn_diff__parse_next_hunk): Allow empty lines in hunk.
>>
>> * subversion/tests/libsvn_diff/parse-diff-test.c
>> (test_parse_unidiff): Fix changed representation of hunks.
>>
>> * subversion/libsvn_client/patch.c
>> (match_hunk, copy_hunk_text): Fix changed representation of hunks.
>> ]]]
>>
>> Mvh
>> Daniel
>>
>
>> Index: subversion/libsvn_diff/parse-diff.c
>> ===================================================================
>> --- subversion/libsvn_diff/parse-diff.c (revision 38646)
>> +++ subversion/libsvn_diff/parse-diff.c (arbetskopia)
>> @@ -30,6 +30,7 @@
>> #include "svn_pools.h"
>> #include "svn_utf.h"
>> #include "svn_dirent_uri.h"
>> +#include "svn_cmdline.h"
>>
>> #include "private/svn_diff_private.h"
>>
>> @@ -273,6 +274,42 @@
>> return TRUE;
>> }
>>
>> +static svn_error_t *
>> +set_hunk_text(svn_stream_t **hunk_text, svn_stream_t **text,
>> + apr_pool_t *pool)
>> +{
>> + int offset;
>> + svn_stringbuf_t *line, *buf;
>> + svn_boolean_t eof;
>> + char *c_line;
>> +
>> + buf = svn_stringbuf_create("", pool);
>> +
>> + do
>> + {
>> + SVN_ERR(svn_stream_readline(*text, &line, APR_EOL_STR, &eof,
>> pool));
>> +
>> + char c = line->data[0];
>> +
>> + if ( c == ' ' || c == '-' || c == '+')
>> + offset = 1;
>> + else
>> + offset = 0;
>> +
>> + if (! eof)
>> + c_line = apr_psprintf(pool, "%s%s", line->data + offset,
>> APR_EOL_STR);
>> + else
>> + c_line = apr_psprintf(pool, "%s", line->data + offset);
>> +
>> + svn_stringbuf_appendcstr(buf, c_line);
>> +
>> + } while (! eof);
>> +
>> + *hunk_text = svn_stream_from_stringbuf(buf, pool);
>> +
>> + return SVN_NO_ERROR;
>> +}
>> +
>> /* A stream line-filter which allows only original text from a
>> hunk. */
>> static svn_error_t *
>> original_line_filter(svn_boolean_t *filtered, const char *line,
>> @@ -305,6 +342,7 @@
>> svn_stream_t *diff_text;
>> svn_stream_t *original_text;
>> svn_stream_t *modified_text;
>> + svn_linenum_t original_lines;
>> svn_stream_t *s;
>> apr_pool_t *iterpool;
>>
>> @@ -358,8 +396,22 @@
>> }
>>
>> c = line->data[0];
>> - if (c == ' ' || c == '-' || c == '+')
>> - hunk_seen = TRUE;
>> +
>> + if (c == ' ' || c == '-')
>> + {
>> + hunk_seen = TRUE;
>> + original_lines--;
>> + }
>> + else if (c == '+')
>> + {
>> + hunk_seen = TRUE;
>> + }
>> + /* Should tolerate chopped leading spaces on empty
>> lines. */
>> + else if (original_lines > 0 && ! eof && line->len == 0)
>> + {
>> + hunk_seen = TRUE;
>> + original_lines--;
>> + }
>> else
>> {
>> in_hunk = FALSE;
>> @@ -376,6 +428,10 @@
>> if (starts_with(line->data, atat))
>> /* Looks like we have a hunk header, let's try to rip
>> it apart. */
>> in_hunk = parse_hunk_header(line->data, *hunk, iterpool);
>> +
>> + if (in_hunk)
>> + original_lines = (*hunk)->original_length;
>> +
>> else if (starts_with(line->data, minus))
>> /* This could be a header of another patch. Bail out. */
>> break;
>> @@ -421,9 +477,13 @@
>> svn_stream_set_line_filter_callback(modified_text,
>> modified_line_filter);
>>
>> /* Set the hunk's texts. */
>> - (*hunk)->diff_text = diff_text;
>> - (*hunk)->original_text = original_text;
>> - (*hunk)->modified_text = modified_text;
>> + SVN_ERR(set_hunk_text(&(*hunk)->diff_text, &diff_text,
>> + result_pool));
>> + SVN_ERR(set_hunk_text(&(*hunk)->original_text, &original_text,
>> + result_pool));
>> + SVN_ERR(set_hunk_text(&(*hunk)->modified_text, &modified_text,
>> + result_pool));
>> +
>> }
>> else
>> /* Something went wrong, just discard the result. */
>> Index: subversion/tests/libsvn_diff/parse-diff-test.c
>> ===================================================================
>> --- subversion/tests/libsvn_diff/parse-diff-test.c (revision 38646)
>> +++ subversion/tests/libsvn_diff/parse-diff-test.c (arbetskopia)
>> @@ -46,7 +46,7 @@
>>
>> "=
>> =================================================================="
>> NL
>> "--- A/D/
>> gamma.orig" NL
>> "+++ A/D/
>> gamma" NL
>> - "@@ -1 +1,2
>> @@" NL
>> + "@@ -1,2 +1
>> @@" NL
>> " This is the file
>> 'gamma'." NL
>> "-some less bytes to
>> 'gamma'" NL
>>
>> "" NL
>> @@ -99,7 +99,7 @@
>> /* Make sure original text was parsed correctly. */
>> SVN_ERR(svn_stream_readline(hunk->original_text, &buf, NL, &eof,
>> pool));
>> SVN_ERR_ASSERT(! eof);
>> - SVN_ERR_ASSERT(! strcmp(buf->data, " This is the file 'gamma'."));
>> + SVN_ERR_ASSERT(! strcmp(buf->data, "This is the file 'gamma'."));
>> /* Now we should get EOF. */
>> SVN_ERR(svn_stream_readline(hunk->original_text, &buf, NL, &eof,
>> pool));
>> SVN_ERR_ASSERT(eof);
>> @@ -108,10 +108,10 @@
>> /* Make sure modified text was parsed correctly. */
>> SVN_ERR(svn_stream_readline(hunk->modified_text, &buf, NL, &eof,
>> pool));
>> SVN_ERR_ASSERT(! eof);
>> - SVN_ERR_ASSERT(! strcmp(buf->data, " This is the file 'gamma'."));
>> + SVN_ERR_ASSERT(! strcmp(buf->data, "This is the file 'gamma'."));
>> SVN_ERR(svn_stream_readline(hunk->modified_text, &buf, NL, &eof,
>> pool));
>> SVN_ERR_ASSERT(! eof);
>> - SVN_ERR_ASSERT(! strcmp(buf->data, "+some more bytes to
>> 'gamma'"));
>> + SVN_ERR_ASSERT(! strcmp(buf->data, "some more bytes to 'gamma'"));
>> /* Now we should get EOF. */
>> SVN_ERR(svn_stream_readline(hunk->modified_text, &buf, NL, &eof,
>> pool));
>> SVN_ERR_ASSERT(eof);
>> @@ -128,10 +128,10 @@
>> /* Make sure original text was parsed correctly. */
>> SVN_ERR(svn_stream_readline(hunk->original_text, &buf, NL, &eof,
>> pool));
>> SVN_ERR_ASSERT(! eof);
>> - SVN_ERR_ASSERT(! strcmp(buf->data, " This is the file 'gamma'."));
>> + SVN_ERR_ASSERT(! strcmp(buf->data, "This is the file 'gamma'."));
>> SVN_ERR(svn_stream_readline(hunk->original_text, &buf, NL, &eof,
>> pool));
>> SVN_ERR_ASSERT(! eof);
>> - SVN_ERR_ASSERT(! strcmp(buf->data, "-some less bytes to
>> 'gamma'"));
>> + SVN_ERR_ASSERT(! strcmp(buf->data, "some less bytes to 'gamma'"));
>> /* Now we should get EOF. */
>> SVN_ERR(svn_stream_readline(hunk->original_text, &buf, NL, &eof,
>> pool));
>> SVN_ERR_ASSERT(eof);
>> @@ -140,7 +140,7 @@
>> /* Make sure modified text was parsed correctly. */
>> SVN_ERR(svn_stream_readline(hunk->modified_text, &buf, NL, &eof,
>> pool));
>> SVN_ERR_ASSERT(! eof);
>> - SVN_ERR_ASSERT(! strcmp(buf->data, " This is the file 'gamma'."));
>> + SVN_ERR_ASSERT(! strcmp(buf->data, "This is the file 'gamma'."));
>> /* Now we should get EOF. */
>> SVN_ERR(svn_stream_readline(hunk->modified_text, &buf, NL, &eof,
>> pool));
>> SVN_ERR_ASSERT(eof);
>> Index: subversion/libsvn_client/patch.c
>> ===================================================================
>> --- subversion/libsvn_client/patch.c (revision 38646)
>> +++ subversion/libsvn_client/patch.c (arbetskopia)
>> @@ -2108,12 +2108,11 @@
>> target->patch->eol_str,
>> &hunk_eof, iterpool));
>> SVN_ERR(svn_stream_readline(target->stream, &target_line,
>> target->eol_str,
>> &target->eof, iterpool));
>> +
>> if (! hunk_eof && hunk_line->len > 0)
>> {
>> - char c = hunk_line->data[0];
>> - SVN_ERR_ASSERT(c == ' ' || c == '-');
>> - lines_matched = (hunk_line->len == target_line->len + 1 &&
>> - ! strcmp(hunk_line->data + 1,
>> target_line->data));
>> + lines_matched = (hunk_line->len == target_line->len &&
>> + ! strcmp(hunk_line->data, target_line-
>> >data));
>> }
>> }
>> while (lines_matched && ! (hunk_eof || target->eof));
>> @@ -2386,13 +2385,9 @@
>> {
>> if (line->len >= 1)
>> {
>> - char c = line->data[0];
>> -
>> - SVN_ERR_ASSERT(c == ' ' || c == '+' || c == '-');
>> - len = line->len - 1;
>> - SVN_ERR(svn_io_file_write_full(file, line->data + 1,
>> len, &len,
>> + len = line->len;
>> + SVN_ERR(svn_io_file_write_full(file, line->data,
>> len, &len,
>> iterpool));
>> - SVN_ERR_ASSERT(len == line->len - 1);
>> }
>>
>> /* Add newline. */
>
> ------------------------------------------------------
> http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2383529
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2383805
Received on 2009-08-15 03:44:16 CEST