Re: svn commit: r40403 - trunk/subversion/libsvn_diff

[Arfrever Frehtes Taifersar Arahesis-2]

2009-11-06 16:37:10 Stefan Sperling napisał(a):
> Author: stsp
> Date: Fri Nov  6 07:37:10 2009
> New Revision: 40403
>
> Log:
> Make 'svn patch' ignore "\ No newline at end of file" marker lines.
> They were interpreted as context and caused text conflicts.

I think that the fix is wrong. `svn patch` should ignore lines which are after
lines specified as belonging to given hunk in @@ lines (e.g. '-1,2 +1,2' in example
below).

> An open question is whether 'svn patch' should do something smart
> in the presence of such lines. Currently, applying a patch which only
> changes the last character of a file to a newline is a no-op, even if
> the target file does not end with a newline. However, this behaviour is
> encoded somewhere within svn_wc_merge4(), rather than the 'svn patch' code.
>
> * subversion/libsvn_diff/parse-diff.c
>   (original_line_filter, modified_line_filter): Filter "\ No newline at
>    end of file" lines.
>   (parse_next_hunk): Disregard "\ No newline at end of file" lines.
>
> Modified:
>    trunk/subversion/libsvn_diff/parse-diff.c
>
> Modified: trunk/subversion/libsvn_diff/parse-diff.c
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_diff/parse-diff.c?pathrev=40403&r1=40402&r2=40403
> ==============================================================================
> --- trunk/subversion/libsvn_diff/parse-diff.c Fri Nov  6 05:23:22 2009 (r40402)
> +++ trunk/subversion/libsvn_diff/parse-diff.c Fri Nov  6 07:37:10 2009 (r40403)
> @@ -167,21 +167,25 @@ parse_hunk_header(const char *header, sv
>    return TRUE;
>  }
>  
> -/* A stream line-filter which allows only original text from a hunk. */
> +/* A stream line-filter which allows only original text from a hunk,
> + * and filters special lines. */
>  static svn_error_t *
>  original_line_filter(svn_boolean_t *filtered, const char *line, void *baton,
>                       apr_pool_t *scratch_pool)
>  {
> -  *filtered = line[0] == '+';
> +  *filtered = (line[0] == '+' ||
> +               strcmp(line, "\\ No newline at end of file") == 0);
>    return SVN_NO_ERROR;
>  }
>  
> -/* A stream line-filter which allows only modified text from a hunk. */
> +/* A stream line-filter which allows only modified text from a hunk,
> + * and filters special lines. */
>  static svn_error_t *
>  modified_line_filter(svn_boolean_t *filtered, const char *line, void *baton,
>                       apr_pool_t *scratch_pool)
>  {
> -  *filtered = line[0] == '-';
> +  *filtered = (line[0] == '-' ||
> +               strcmp(line, "\\ No newline at end of file") == 0);
>    return SVN_NO_ERROR;
>  }
>  
> @@ -248,6 +252,11 @@ parse_next_hunk(svn_hunk_t **hunk,
>        last_line = pos;
>        SVN_ERR(svn_stream_readline(stream, &line, patch->eol_str, &eof,
>                                    iterpool));
> +
> +      /* Skip lines which aren't part of the hunk text. */
> +      if (strcmp(line->data, "\\ No newline at end of file") == 0)
> +        continue;

Such comparisons won't help for non-English users:

$ echo -e "x\ny" > file1
$ echo -en "x\ny" > file2
$ diff -u file1 file2
--- file1       2009-11-06 18:19:48.000000000 +0100
+++ file2       2009-11-06 18:19:54.000000000 +0100
@@ -1,2 +1,2 @@
 x
-y
+y
\ Brak znaku nowej linii na końcu pliku
$

--
Arfrever Frehtes Taifersar Arahesis

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2415158

[Branko Cibej]

Arfrever Frehtes Taifersar Arahesis wrote:

> 2009-11-06 16:37:10 Stefan Sperling napisał(a):
>  
>> Author: stsp
>> Date: Fri Nov  6 07:37:10 2009
>> New Revision: 40403
>>
>> Log:
>> Make 'svn patch' ignore "\ No newline at end of file" marker lines.
>> They were interpreted as context and caused text conflicts.
>>    
>
> I think that the fix is wrong. `svn patch` should ignore lines which are after
> lines specified as belonging to given hunk in @@ lines (e.g. '-1,2 +1,2' in example
> below)

I think both the fix and your proposal are wrong. :) Though your
proposal is less wrong than the fix.

The "no newline at end of file" marker is important information about
context and tells the patch algorithm how to modify its context search.
Unless I'm totally sozzled, i means that the last line of the hunk's
context does not contain an EOLN -- so if you search for the whole line,
you'll most likely never find the context you're looking for.

-- Brane

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2415160

[Daniel Rall]

On Fri, Nov 6, 2009 at 9:31 AM, Arfrever Frehtes Taifersar Arahesis
<Arfrever.FTA@...> wrote:
> 2009-11-06 16:37:10 Stefan Sperling napisał(a):
...

>> @@ -248,6 +252,11 @@ parse_next_hunk(svn_hunk_t **hunk,
>>        last_line = pos;
>>        SVN_ERR(svn_stream_readline(stream, &line, patch->eol_str, &eof,
>>                                    iterpool));
>> +
>> +      /* Skip lines which aren't part of the hunk text. */
>> +      if (strcmp(line->data, "\\ No newline at end of file") == 0)
>> +        continue;
>
> Such comparisons won't help for non-English users:
>
> $ echo -e "x\ny" > file1
> $ echo -en "x\ny" > file2
> $ diff -u file1 file2
> --- file1       2009-11-06 18:19:48.000000000 +0100
> +++ file2       2009-11-06 18:19:54.000000000 +0100
> @@ -1,2 +1,2 @@
>  x
> -y
> +y
> \ Brak znaku nowej linii na końcu pliku
> $

Not commenting on the approach, but you could wrap that English string
in _() to localize it before the strcmp (maybe still also falling back
to the English translation).

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2415161

[Branko Cibej]

Daniel Rall wrote:

> On Fri, Nov 6, 2009 at 9:31 AM, Arfrever Frehtes Taifersar Arahesis
> <Arfrever.FTA@...> wrote:
>  
>> 2009-11-06 16:37:10 Stefan Sperling napisał(a):
>>    
> ...
>  
>>> @@ -248,6 +252,11 @@ parse_next_hunk(svn_hunk_t **hunk,
>>>        last_line = pos;
>>>        SVN_ERR(svn_stream_readline(stream, &line, patch->eol_str, &eof,
>>>                                    iterpool));
>>> +
>>> +      /* Skip lines which aren't part of the hunk text. */
>>> +      if (strcmp(line->data, "\\ No newline at end of file") == 0)
>>> +        continue;
>>>      
>> Such comparisons won't help for non-English users:
>>
>> $ echo -e "x\ny" > file1
>> $ echo -en "x\ny" > file2
>> $ diff -u file1 file2
>> --- file1       2009-11-06 18:19:48.000000000 +0100
>> +++ file2       2009-11-06 18:19:54.000000000 +0100
>> @@ -1,2 +1,2 @@
>>  x
>> -y
>> +y
>> \ Brak znaku nowej linii na końcu pliku
>> $
>>    
>
> Not commenting on the approach, but you could wrap that English string
> in _() to localize it before the strcmp (maybe still also falling back
> to the English translation).
>  

In a patch file, a backslash in the first column means "no newline at
end of file" -- the rest of the line is pure comment and *should* be
igmored, but that backslash tells you important things about the hunk's
context.

-- Brane

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2415163

[Arfrever Frehtes Taifersar Arahesis-2]

2009-11-06 18:35:01 Daniel Rall napisał(a):

> On Fri, Nov 6, 2009 at 9:31 AM, Arfrever Frehtes Taifersar Arahesis
> <Arfrever.FTA@...> wrote:
> > 2009-11-06 16:37:10 Stefan Sperling napisał(a):
> ...
> >> @@ -248,6 +252,11 @@ parse_next_hunk(svn_hunk_t **hunk,
> >>        last_line = pos;
> >>        SVN_ERR(svn_stream_readline(stream, &line, patch->eol_str, &eof,
> >>                                    iterpool));
> >> +
> >> +      /* Skip lines which aren't part of the hunk text. */
> >> +      if (strcmp(line->data, "\\ No newline at end of file") == 0)
> >> +        continue;
> >
> > Such comparisons won't help for non-English users:
> >
> > $ echo -e "x\ny" > file1
> > $ echo -en "x\ny" > file2
> > $ diff -u file1 file2
> > --- file1       2009-11-06 18:19:48.000000000 +0100
> > +++ file2       2009-11-06 18:19:54.000000000 +0100
> > @@ -1,2 +1,2 @@
> >  x
> > -y
> > +y
> > \ Brak znaku nowej linii na końcu pliku
> > $
>
> Not commenting on the approach, but you could wrap that English string
> in _() to localize it before the strcmp (maybe still also falling back
> to the English translation).

In this case, translation is performed using data obtained from .mo files
from GNU Diffutils project. This string can be different in different
diff implementations and in different versions of the same implementation.

--
Arfrever Frehtes Taifersar Arahesis

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2415164

[Daniel Rall]

On Fri, Nov 6, 2009 at 9:38 AM, Branko Čibej <brane@...> wrote:

> Daniel Rall wrote:
>> On Fri, Nov 6, 2009 at 9:31 AM, Arfrever Frehtes Taifersar Arahesis
>> <Arfrever.FTA@...> wrote:
>>
>>> 2009-11-06 16:37:10 Stefan Sperling napisał(a):
>>>
>> ...
>>
>>>> @@ -248,6 +252,11 @@ parse_next_hunk(svn_hunk_t **hunk,
>>>>        last_line = pos;
>>>>        SVN_ERR(svn_stream_readline(stream, &line, patch->eol_str, &eof,
>>>>                                    iterpool));
>>>> +
>>>> +      /* Skip lines which aren't part of the hunk text. */
>>>> +      if (strcmp(line->data, "\\ No newline at end of file") == 0)
>>>> +        continue;
>>>>
>>> Such comparisons won't help for non-English users:
>>>
>>> $ echo -e "x\ny" > file1
>>> $ echo -en "x\ny" > file2
>>> $ diff -u file1 file2
>>> --- file1       2009-11-06 18:19:48.000000000 +0100
>>> +++ file2       2009-11-06 18:19:54.000000000 +0100
>>> @@ -1,2 +1,2 @@
>>>  x
>>> -y
>>> +y
>>> \ Brak znaku nowej linii na końcu pliku
>>> $
>>>
>>
>> Not commenting on the approach, but you could wrap that English string
>> in _() to localize it before the strcmp (maybe still also falling back
>> to the English translation).
>>
>
> In a patch file, a backslash in the first column means "no newline at
> end of file" -- the rest of the line is pure comment and *should* be
> igmored, but that backslash tells you important things about the hunk's
> context.

So, any line starting with a backslash after a hunk should be
discarded, but it must also modify the last line in the hunk, removing
the explicit newline that was part of the patch.

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2415168

[Stefan Sperling-7]

On Fri, Nov 06, 2009 at 09:47:12AM -0800, Daniel Rall wrote:
> On Fri, Nov 6, 2009 at 9:38 AM, Branko Čibej <brane@...> wrote:
> > Daniel Rall wrote:
> > In a patch file, a backslash in the first column means "no newline at
> > end of file" -- the rest of the line is pure comment and *should* be
> > igmored, but that backslash tells you important things about the hunk's
> > context.
>
> So, any line starting with a backslash after a hunk should be
> discarded,

Thanks, I'll fix this.
I was under the impression that the string was part of the special
marker and was not supposed to be localised.
Quoting libsvn_diff/diff_file.c:

          SVN_ERR(svn_utf_cstring_from_utf8_ex2
                  (&out_str,
                   /* The string below is intentionally not marked for
                      translation: it's vital to correct operation of
                      the diff(1)/patch(1) program pair. */
                   APR_EOL_STR "\\ No newline at end of file" APR_EOL_STR,
                   baton->header_encoding, baton->pool));

But as Arfrever pointed out other implementations do localise it.
I checked the UNIX patch code and it only looks for the backslash, too.

> but it must also modify the last line in the hunk, removing
> the explicit newline that was part of the patch.

All newlines are already removed by svn_stream_readline().
That's why using svn patch to apply a patch which only changes newlines
is currently a no-op, as explained in the log message.

Stefan

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2415174