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
Received on 2009-08-14 07:27:47 CEST