Re: Wrong assumption about unidiff syntax in libsvn_diff?

On Tue, Nov 20, 2012 at 4:09 PM, Stefan Sperling <stsp_at_elego.de> wrote:
> On Tue, Nov 20, 2012 at 03:40:20PM +0100, Branko Čibej wrote:
>> On 20.11.2012 15:05, Stefan Sperling wrote:
>> > I just checked the UNIX patch code shipped with OpenBSD and it seems
>> > you're right that it only looks for the backslash and ignores the comment.
>> >
>> > However, it seems in practice patches usually contain this string in
>> > non-localised form. At least nobody has yet complained about svn patch
>> > misparsing such patches.
>>
>> I'm distinctly remember a report on this very list, with a unidiff with
>> a localized message attached. However, I can't find it in the archives.
>> In any case it would be nice if our diff parser only looked at the \,
>> not the whole message. I'm less worried about our not localizing that
>> particular string.
>
> Currently, lines such as:
> \\ this is a comment
> anywhere in the patch are silently ignored.
>
> If we follow your suggestion we must treat any such lines as hunk
> terminators and assume the hunk lacks a trailing newline.
> I suppose such a change is correct but it's a behaviour change so
> I wouldn't want to backport it to 1.7.x.
>
> Below is a patch. Diff parser tests and patch tests are still passing.
>
> [[[
> * subversion/libsvn_diff/parse-diff.c
> (parse_next_hunk): Treat any line that starts with a backslash as a
> hunk terminator, indicating that the hunk does not end with EOL.
> Comments following the backslash might be localised or missing
> in which case parsing the patch would fail.
>
> Suggested by: brane
> ]]]
>
> Index: subversion/libsvn_diff/parse-diff.c
> ===================================================================
> --- subversion/libsvn_diff/parse-diff.c (revision 1411078)
> +++ subversion/libsvn_diff/parse-diff.c (working copy)
> @@ -555,15 +555,11 @@ parse_next_hunk(svn_diff_hunk_t **hunk,
> pos = 0;
> SVN_ERR(svn_io_file_seek(apr_file, APR_CUR, &pos, iterpool));
>
> - /* Lines starting with a backslash are comments, such as
> + /* Lines starting with a backslash indicate a missing EOL:
> * "\ No newline at end of file". */
> if (line->data[0] == '\\')
> {
> - if (in_hunk &&
> - ((!*is_property &&
> - strcmp(line->data, "\\ No newline at end of file") == 0) ||
> - (*is_property &&
> - strcmp(line->data, "\\ No newline at end of property") == 0)))
> + if (in_hunk)
> {
> char eolbuf[2];
> apr_size_t len;

I thought Branko said "leading backslash followed by a space":

[[[
Only the leading backslash and space are important for
signaling the no-trailing-eoln state.
]]]

(your argument about "\\ this is a comment" still holds though)

-- 
Johan