You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Näslund <da...@longitudo.com> on 2010/02/02 19:48:43 UTC
Why non matching eols in patch and target. (svn patch related)
Hi Stefan!
In match_hunk() we try to match lines from the context of the patch with
lines in the target. Earlier, in init_patch_target() we detect the eol
of the target and open streams to read from the target and to write the
patched result. Those streams does translation of keywords and eols.
In match_hunk we read a line from original_text and translate it. But we
don't get any translation of the eols in hunk_line_translated.
I've set the eol_str argument in svn_subst_translate_cstring2() to
"\r\n" for testing but it still doesn't use "\r\n" for eol. Can there be
an error in svn_subst_translate_cstring2()?
A piece of code from match_hunk() where I've inserted debug statements
and changed the eol_str argument of translate_cstring2().
[[[
const char *hunk_line_translated;
const char *eol_str;
svn_pool_clear(iterpool);
SVN_ERR(svn_stream_readline_detect_eol(hunk->original_text,
&hunk_line, &eol_str,
&hunk_eof, iterpool));
/* Contract keywords, if any, before matching. */
SVN_ERR(svn_subst_translate_cstring2(hunk_line->data,
&hunk_line_translated,
"\r\n, FALSE,
target->keywords, FALSE,
iterpool));
lines_read++;
SVN_ERR(read_line(target, &target_line, iterpool, iterpool));
if (strstr(hunk_line_translated, "\r\n"))
SVN_DBG(("hunk has \\r\\n \n"));
if (strstr(target_line, "\r\n"))
SVN_DBG(("target has \\r\\n \n"));
]]]
A piece of the output. No '\r\n' endings found for hunk_line_translated.
[[[
DBG: patch.c: 629: target has \r\n
DBG: patch.c: 629: target has \r\n
DBG: patch.c: 629: target has \r\n
C svn-test-work/working_copies/patch_tests-9/A/mu
> rejected hunk @@ -1,1 +1,3 @@
]]]
Do you have any clues to this?
Daniel
Re: Why non matching eols in patch and target. (svn patch related)
Posted by Daniel Näslund <da...@longitudo.com>.
On Tue, Feb 23, 2010 at 08:57:26PM +0100, Daniel Näslund wrote:
> On Tue, Feb 02, 2010 at 09:11:52PM +0100, Stefan Sperling wrote:
> > On Tue, Feb 02, 2010 at 08:48:43PM +0100, Daniel Näslund wrote:
[..]
> What to do?
> [ ] Write documentation saying we can only repair eols on targets with a
> svn:eol-style prop.
> [ ] Wrap the error for a better error message perhaps saying 'patch and
> target uses different eol' or similar.
> [ ] Adjust the code to allow for repairing eols even when there is no
> svn:eol-style prop. I would go for this one if it's doable.
>
> Before I throw myself to the mercy of the eol-translating code, I ask
> you if there is something you know of hindering us from repairing eols
> where there is no prop set?
>
> In init_patch_target() we decide how the eol translation of
> target->patched will be done:
> [[[
>
> eol_style_val = apr_hash_get(props, SVN_PROP_EOL_STYLE,
> APR_HASH_KEY_STRING);
> if (eol_style_val)
> {
> svn_subst_eol_style_from_value(&target->eol_style,
> &target->eol_str,
> eol_style_val->data);
> }
> else
> {
> /* Just use the first EOL sequence we can find in the file. */
> SVN_ERR(svn_eol__detect_file_eol(&target->eol_str,
> target->file, scratch_pool));
> /* But don't enforce any particular EOL-style. */
> target->eol_style = svn_subst_eol_style_none;
> }
>
> [...]
>
> target->patched = svn_subst_stream_translated(
> target->patched_raw,
> target->eol_str,
> target->eol_style ==
> svn_subst_eol_style_fixed,
> target->keywords, TRUE, result_pool);
> ]]]
>
> You decided to not enforce any particular eol-style if we don't have a
> prop. The eol_style is used for the cases where there is an eol-style
> prop. Fair enough. But couldn't we do some hack where we always say that
> the eol-style is fixed and use whatever eol we find in the target? I did
> a quick try and got a lot of rejected hunks...
>
> What can go wrong with this approach?
> -------------------------------------
> * We falsely label targets with inconsistent eols to be of type
> eol_style_fixed. But after patching the eols will be consistent and
> that's a good thing.
> * [Placeholder for more stuff that can go wrong]
>
> Or we could set the style to svn_subst_eol_style_native. We will end up
> with native newlines in the target then, right?
Oups, lost the thread there. The eol-style was only used to check for
style_fixed, and if so translate eols. The question remains; why
can't we translate files when there is no no eol-style prop. Is it worth
investigating further?
Daniel
Re: Why non matching eols in patch and target. (svn patch related)
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Feb 24, 2010 at 12:32:59PM +0100, Daniel Näslund wrote:
> Trying to understand the problem
> ----------------------------------
> A translated stream expects that the stuff to be written will have
> consistent line endings. It checks for consistent line endings by
> comparing the eol with the first eol written to the stream. If the first
> thing written is from the target, we will compare all subsequent eols
> with the targets eol.
Hmmm... is your understanding of the translation APIs correct?
The way I think it works is that you wrap a stream inside a translation
stream. At the point where you do the wrapping, you tell the translation
stream whether it should repair EOLs, and also what kind of EOL you'd
like the translated result to have.
Then, *if* you told the stream to repair newlines, the EOL-style you
specified at wrapping time is written to the output stream no matter
what EOL-style appears on the input stream.
Else, the stream is written unchanged.
Stefan
Re: Why non matching eols in patch and target. (svn patch related)
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Feb 24, 2010 at 12:32:59PM +0100, Daniel Näslund wrote:
> Trying to understand the problem
> ----------------------------------
> A translated stream expects that the stuff to be written will have
> consistent line endings. It checks for consistent line endings by
> comparing the eol with the first eol written to the stream. If the first
> thing written is from the target, we will compare all subsequent eols
> with the targets eol.
Hmmm... is your understanding of the translation APIs correct?
The way I think it works is that you wrap a stream inside a translation
stream. At the point where you do the wrapping, you tell the translation
stream whether it should repair EOLs, and also what kind of EOL you'd
like the translated result to have.
Then, *if* you told the stream to repair newlines, the EOL-style you
specified at wrapping time is written to the output stream no matter
what EOL-style appears on the input stream.
Else, the stream is written unchanged.
Stefan
Re: Why non matching eols in patch and target. (svn patch related)
Posted by Daniel Näslund <da...@longitudo.com>.
On Tue, Feb 23, 2010 at 11:48:07PM +0100, Stefan Sperling wrote:
> Can you post the test?
I've attached the test.
Error cases
--------------
If there are lines in the target before the hunk we will get the error
of inconsistent line endings when applying the hunk lines. (Remove the #
from the first line of mu_contents and change hunk offset to run).
[[[
Testing nr 1 of eols list for patch_eol
Testing nr 2 of eols list for patch_eol
subversion/svn/patch-cmd.c:81: (apr_err=135000)
subversion/libsvn_client/patch.c:1463: (apr_err=135000)
subversion/libsvn_client/patch.c:1410: (apr_err=135000)
subversion/libsvn_client/patch.c:1093: (apr_err=135000)
subversion/libsvn_client/patch.c:932: (apr_err=135000)
subversion/libsvn_client/patch.c:799: (apr_err=135000)
subversion/libsvn_subr/subst.c:876: (apr_err=135000)
subversion/libsvn_subr/subst.c:643: (apr_err=135000)
svn: Inconsistent line ending style
]]]
If there are no lines in the target before the hunk we will get the
error when copying the remaining lines.
[[[
Testing nr 1 of eols list for patch_eol
Testing nr 2 of eols list for patch_eol
subversion/svn/patch-cmd.c:81: (apr_err=135000)
subversion/libsvn_client/patch.c:1463: (apr_err=135000)
subversion/libsvn_client/patch.c:1410: (apr_err=135000)
subversion/libsvn_client/patch.c:1100: (apr_err=135000)
subversion/libsvn_client/patch.c:830: (apr_err=135000)
subversion/libsvn_client/patch.c:799: (apr_err=135000)
subversion/libsvn_subr/subst.c:876: (apr_err=135000)
subversion/libsvn_subr/subst.c:643: (apr_err=135000)
svn: Inconsistent line ending style
]]]
Naive attempts without truly understanding the problem
--------------------------------------------------------
1) If I if-zero the error in libsvn_subr/subst.c:643 and have one line
before the hunk I get:
[[[
=============================================================
EXPECTED NODE FOUND:
=============================================================
* Node name: mu
Path: __SVN_ROOT_NODE/A/mu
It is a promotional program aimed at encouraging internet users;d.w
Properties: {}
Attributes: {}
Children: None (node is probably a file)
=============================================================
ACTUAL NODE FOUND:
=============================================================
* Node name: mu
Path: __SVN_ROOT_NODE/A/mu
It is a promotional program aimed at encouraging internet users;d.w
Properties: {}
Attributes: {}
Children: None (node is probably a file)
]]]
I wonder what 'd.w' means and why I'm not getting the full text?
2) If I catch SVN_ERR_IO_INCONSISTENT_EOL when we write to target->patched
the patch deletes the file A/mu! Appearantly there's more to this than
meets the eye!
Trying to understand the problem
----------------------------------
A translated stream expects that the stuff to be written will have
consistent line endings. It checks for consistent line endings by
comparing the eol with the first eol written to the stream. If the first
thing written is from the target, we will compare all subsequent eols
with the targets eol. If it's from the patch, we will compare all
subsequent eols with the patchs eol.
What to do
-------------
[ ] Add a flag to svn_subst_stream_translated() to allow for
inconsistent line endings. It's ugly but what else can we do? We
don't want to reinvent the quirky logic to translate streams.
[ ] [Placeholder for suggestions]
Maybe a separate issue
--------------------------
If we if-zero the error in libsvn_subr/subst.c:643 and have one line
directly after the hunk with just a newline, that newline is not
inserted when:
target has eol '\n' ('\012' on unix) and patch has eol '\015' ('\r').
target has eol '\012' and patch has eol '\015' ('\r')
If we have this, the newline is inserted:
[[[
The hunk content
A line from target with more than just a newline
A newline from target
]]]
Daniel
Re: Why non matching eols in patch and target. (svn patch related)
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Feb 23, 2010 at 10:06:04PM +0100, Daniel Näslund wrote:
> On Tue, Feb 23, 2010 at 09:27:34PM +0100, Stefan Sperling wrote:
> > On Tue, Feb 23, 2010 at 08:57:26PM +0100, Daniel Näslund wrote:
> > > I have a test where the target uses '\r\n' and the patch uses '\r' . The
> > > eols are consistent within each file but we get a failure saying:
> > >
> > > [[[
> > > subversion/svn/patch-cmd.c:81: (apr_err=135000)
> > > subversion/libsvn_client/patch.c:1463: (apr_err=135000)
> > > subversion/libsvn_client/patch.c:1410: (apr_err=135000)
> > > subversion/libsvn_client/patch.c:1100: (apr_err=135000)
> > > subversion/libsvn_client/patch.c:830: (apr_err=135000)
> > > subversion/libsvn_client/patch.c:799: (apr_err=135000)
> > > subversion/libsvn_subr/subst.c:876: (apr_err=135000)
> > > subversion/libsvn_subr/subst.c:643: (apr_err=135000)
> > > svn: Inconsistent line ending style
> > > ]]]
> >
> > Where and how?
>
> It happens when we've written all our hunks and we want to copy the
> remaining lines. Here's a piece of apply_one_patch(). It's the call to
> copy_lines_to_target() that throws the error.
> The test has one line after the last hunk. If I remove that line, the
> test passes.
Can you post the test?
Thanks,
Stefan
Re: Why non matching eols in patch and target. (svn patch related)
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Feb 23, 2010 at 10:06:04PM +0100, Daniel Näslund wrote:
> On Tue, Feb 23, 2010 at 09:27:34PM +0100, Stefan Sperling wrote:
> > On Tue, Feb 23, 2010 at 08:57:26PM +0100, Daniel Näslund wrote:
> > > I have a test where the target uses '\r\n' and the patch uses '\r' . The
> > > eols are consistent within each file but we get a failure saying:
> > >
> > > [[[
> > > subversion/svn/patch-cmd.c:81: (apr_err=135000)
> > > subversion/libsvn_client/patch.c:1463: (apr_err=135000)
> > > subversion/libsvn_client/patch.c:1410: (apr_err=135000)
> > > subversion/libsvn_client/patch.c:1100: (apr_err=135000)
> > > subversion/libsvn_client/patch.c:830: (apr_err=135000)
> > > subversion/libsvn_client/patch.c:799: (apr_err=135000)
> > > subversion/libsvn_subr/subst.c:876: (apr_err=135000)
> > > subversion/libsvn_subr/subst.c:643: (apr_err=135000)
> > > svn: Inconsistent line ending style
> > > ]]]
> >
> > Where and how?
>
> It happens when we've written all our hunks and we want to copy the
> remaining lines. Here's a piece of apply_one_patch(). It's the call to
> copy_lines_to_target() that throws the error.
> The test has one line after the last hunk. If I remove that line, the
> test passes.
Can you post the test?
Thanks,
Stefan
Re: Why non matching eols in patch and target. (svn patch related)
Posted by Daniel Näslund <da...@longitudo.com>.
On Tue, Feb 23, 2010 at 09:27:34PM +0100, Stefan Sperling wrote:
> On Tue, Feb 23, 2010 at 08:57:26PM +0100, Daniel Näslund wrote:
> > On Tue, Feb 02, 2010 at 09:11:52PM +0100, Stefan Sperling wrote:
> > > On Tue, Feb 02, 2010 at 08:48:43PM +0100, Daniel Näslund wrote:
> > > > Hi Stefan!
> > > >
> > > > In match_hunk() we try to match lines from the context of the patch with
> > > > lines in the target. Earlier, in init_patch_target() we detect the eol
> > > > of the target and open streams to read from the target and to write the
> > > > patched result. Those streams does translation of keywords and eols.
> > > >
> > > > In match_hunk we read a line from original_text and translate it. But we
> > > > don't get any translation of the eols in hunk_line_translated.
> > >
> > > svn patch only repairs EOLs if the svn:eol-style enforces a fixed
> > > value (such as CR or CRLF). Try setting svn:eol-style to 'CRLF' on
> > > the patch target. Then you'll see dos-style newlines in the patched result.
> > >
> > > Admittedly, we may want to repair EOLs in more scenarios (such as
> > > eol-style = native).
> >
> > I have a test where the target uses '\r\n' and the patch uses '\r' . The
> > eols are consistent within each file but we get a failure saying:
> >
> > [[[
> > subversion/svn/patch-cmd.c:81: (apr_err=135000)
> > subversion/libsvn_client/patch.c:1463: (apr_err=135000)
> > subversion/libsvn_client/patch.c:1410: (apr_err=135000)
> > subversion/libsvn_client/patch.c:1100: (apr_err=135000)
> > subversion/libsvn_client/patch.c:830: (apr_err=135000)
> > subversion/libsvn_client/patch.c:799: (apr_err=135000)
> > subversion/libsvn_subr/subst.c:876: (apr_err=135000)
> > subversion/libsvn_subr/subst.c:643: (apr_err=135000)
> > svn: Inconsistent line ending style
> > ]]]
>
> Where and how?
It happens when we've written all our hunks and we want to copy the
remaining lines. Here's a piece of apply_one_patch(). It's the call to
copy_lines_to_target() that throws the error.
[[[
/* Apply or reject hunks. */
for (i = 0; i < target->hunks->nelts; i++)
{
hunk_info_t *hi;
svn_pool_clear(iterpool);
hi = APR_ARRAY_IDX(target->hunks, i, hunk_info_t *);
if (hi->rejected)
SVN_ERR(reject_hunk(target, hi, iterpool));
else
SVN_ERR(apply_hunk(target, hi, iterpool));
}
svn_pool_destroy(iterpool);
if (target->kind == svn_node_file)
{
/* Copy any remaining lines to target. */
SVN_ERR(copy_lines_to_target(target, 0, scratch_pool));
if (! target->eof)
{
/* We could not copy the entire target file to the temporary file,
* and would truncate the target if we copied the temporary file
* on top of it. Skip this target. */
target->skipped = TRUE;
}
}
]]]
The test has one line after the last hunk. If I remove that line, the
test passes.
Daniel
Re: Why non matching eols in patch and target. (svn patch related)
Posted by Daniel Näslund <da...@longitudo.com>.
On Tue, Feb 23, 2010 at 09:27:34PM +0100, Stefan Sperling wrote:
> On Tue, Feb 23, 2010 at 08:57:26PM +0100, Daniel Näslund wrote:
> > On Tue, Feb 02, 2010 at 09:11:52PM +0100, Stefan Sperling wrote:
> > > On Tue, Feb 02, 2010 at 08:48:43PM +0100, Daniel Näslund wrote:
> > > > Hi Stefan!
> > > >
> > > > In match_hunk() we try to match lines from the context of the patch with
> > > > lines in the target. Earlier, in init_patch_target() we detect the eol
> > > > of the target and open streams to read from the target and to write the
> > > > patched result. Those streams does translation of keywords and eols.
> > > >
> > > > In match_hunk we read a line from original_text and translate it. But we
> > > > don't get any translation of the eols in hunk_line_translated.
> > >
> > > svn patch only repairs EOLs if the svn:eol-style enforces a fixed
> > > value (such as CR or CRLF). Try setting svn:eol-style to 'CRLF' on
> > > the patch target. Then you'll see dos-style newlines in the patched result.
> > >
> > > Admittedly, we may want to repair EOLs in more scenarios (such as
> > > eol-style = native).
> >
> > I have a test where the target uses '\r\n' and the patch uses '\r' . The
> > eols are consistent within each file but we get a failure saying:
> >
> > [[[
> > subversion/svn/patch-cmd.c:81: (apr_err=135000)
> > subversion/libsvn_client/patch.c:1463: (apr_err=135000)
> > subversion/libsvn_client/patch.c:1410: (apr_err=135000)
> > subversion/libsvn_client/patch.c:1100: (apr_err=135000)
> > subversion/libsvn_client/patch.c:830: (apr_err=135000)
> > subversion/libsvn_client/patch.c:799: (apr_err=135000)
> > subversion/libsvn_subr/subst.c:876: (apr_err=135000)
> > subversion/libsvn_subr/subst.c:643: (apr_err=135000)
> > svn: Inconsistent line ending style
> > ]]]
>
> Where and how?
It happens when we've written all our hunks and we want to copy the
remaining lines. Here's a piece of apply_one_patch(). It's the call to
copy_lines_to_target() that throws the error.
[[[
/* Apply or reject hunks. */
for (i = 0; i < target->hunks->nelts; i++)
{
hunk_info_t *hi;
svn_pool_clear(iterpool);
hi = APR_ARRAY_IDX(target->hunks, i, hunk_info_t *);
if (hi->rejected)
SVN_ERR(reject_hunk(target, hi, iterpool));
else
SVN_ERR(apply_hunk(target, hi, iterpool));
}
svn_pool_destroy(iterpool);
if (target->kind == svn_node_file)
{
/* Copy any remaining lines to target. */
SVN_ERR(copy_lines_to_target(target, 0, scratch_pool));
if (! target->eof)
{
/* We could not copy the entire target file to the temporary file,
* and would truncate the target if we copied the temporary file
* on top of it. Skip this target. */
target->skipped = TRUE;
}
}
]]]
The test has one line after the last hunk. If I remove that line, the
test passes.
Daniel
Re: Why non matching eols in patch and target. (svn patch related)
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Feb 23, 2010 at 08:57:26PM +0100, Daniel Näslund wrote:
> On Tue, Feb 02, 2010 at 09:11:52PM +0100, Stefan Sperling wrote:
> > On Tue, Feb 02, 2010 at 08:48:43PM +0100, Daniel Näslund wrote:
> > > Hi Stefan!
> > >
> > > In match_hunk() we try to match lines from the context of the patch with
> > > lines in the target. Earlier, in init_patch_target() we detect the eol
> > > of the target and open streams to read from the target and to write the
> > > patched result. Those streams does translation of keywords and eols.
> > >
> > > In match_hunk we read a line from original_text and translate it. But we
> > > don't get any translation of the eols in hunk_line_translated.
> >
> > svn patch only repairs EOLs if the svn:eol-style enforces a fixed
> > value (such as CR or CRLF). Try setting svn:eol-style to 'CRLF' on
> > the patch target. Then you'll see dos-style newlines in the patched result.
> >
> > Admittedly, we may want to repair EOLs in more scenarios (such as
> > eol-style = native).
>
> I have a test where the target uses '\r\n' and the patch uses '\r' . The
> eols are consistent within each file but we get a failure saying:
>
> [[[
> subversion/svn/patch-cmd.c:81: (apr_err=135000)
> subversion/libsvn_client/patch.c:1463: (apr_err=135000)
> subversion/libsvn_client/patch.c:1410: (apr_err=135000)
> subversion/libsvn_client/patch.c:1100: (apr_err=135000)
> subversion/libsvn_client/patch.c:830: (apr_err=135000)
> subversion/libsvn_client/patch.c:799: (apr_err=135000)
> subversion/libsvn_subr/subst.c:876: (apr_err=135000)
> subversion/libsvn_subr/subst.c:643: (apr_err=135000)
> svn: Inconsistent line ending style
> ]]]
Where and how?
> What to do?
> [ ] Write documentation saying we can only repair eols on targets with a
> svn:eol-style prop.
That's my plan. Don't try to repair if no svn:eol-style is set.
> [ ] Wrap the error for a better error message perhaps saying 'patch and
> target uses different eol' or similar.
> [ ] Adjust the code to allow for repairing eols even when there is no
> svn:eol-style prop. I would go for this one if it's doable.
>
> Before I throw myself to the mercy of the eol-translating code, I ask
> you if there is something you know of hindering us from repairing eols
> where there is no prop set?
The property is the only way for the user to tell us what the EOL
style of the file should be. If the property isn't set, we simply
don't know what the user wants. Then, if the patch and target are
a mixed bag of EOLs, we should go for a best-effort approach.
Let's just say that in case of ambiguity, the target wins.
Else the patch wins.
For example, assuming mixed eol styles in both patch and target,
and no fixed EOL-style on the target:
Original line in patch says '\n', target says '\r'
If modified line in patch changes to '\r': use '\r'
If modified line in patch says at '\n': use '\r'
Original line in patch says '\n', target says '\n'
If modified line in patch changes to '\r': use '\r'
If modified line in patch says at '\n': use '\n'
This second example makes patches which change only line endings work.
Note that this is per *line*, not per hunk. We make those
decisions on a per-line basis. There are 3 supported EOL styles
so the above examples need to be generalised.
> * We falsely label targets with inconsistent eols to be of type
> eol_style_fixed. But after patching the eols will be consistent and
> that's a good thing.
But what if people really want files with mixed EOLs?
> * [Placeholder for more stuff that can go wrong]
>
> Or we could set the style to svn_subst_eol_style_native. We will end up
> with native newlines in the target then, right?
If the target has svn:eol-style 'native', it will have a consistent
EOL-style, unless the file has been messed with. But if it has been
messed with, writing data through the translation stream will normalise
newlines again. So the patching process will repair broken EOLs in this
case.
Stefan
Re: Why non matching eols in patch and target. (svn patch related)
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Feb 23, 2010 at 08:57:26PM +0100, Daniel Näslund wrote:
> On Tue, Feb 02, 2010 at 09:11:52PM +0100, Stefan Sperling wrote:
> > On Tue, Feb 02, 2010 at 08:48:43PM +0100, Daniel Näslund wrote:
> > > Hi Stefan!
> > >
> > > In match_hunk() we try to match lines from the context of the patch with
> > > lines in the target. Earlier, in init_patch_target() we detect the eol
> > > of the target and open streams to read from the target and to write the
> > > patched result. Those streams does translation of keywords and eols.
> > >
> > > In match_hunk we read a line from original_text and translate it. But we
> > > don't get any translation of the eols in hunk_line_translated.
> >
> > svn patch only repairs EOLs if the svn:eol-style enforces a fixed
> > value (such as CR or CRLF). Try setting svn:eol-style to 'CRLF' on
> > the patch target. Then you'll see dos-style newlines in the patched result.
> >
> > Admittedly, we may want to repair EOLs in more scenarios (such as
> > eol-style = native).
>
> I have a test where the target uses '\r\n' and the patch uses '\r' . The
> eols are consistent within each file but we get a failure saying:
>
> [[[
> subversion/svn/patch-cmd.c:81: (apr_err=135000)
> subversion/libsvn_client/patch.c:1463: (apr_err=135000)
> subversion/libsvn_client/patch.c:1410: (apr_err=135000)
> subversion/libsvn_client/patch.c:1100: (apr_err=135000)
> subversion/libsvn_client/patch.c:830: (apr_err=135000)
> subversion/libsvn_client/patch.c:799: (apr_err=135000)
> subversion/libsvn_subr/subst.c:876: (apr_err=135000)
> subversion/libsvn_subr/subst.c:643: (apr_err=135000)
> svn: Inconsistent line ending style
> ]]]
Where and how?
> What to do?
> [ ] Write documentation saying we can only repair eols on targets with a
> svn:eol-style prop.
That's my plan. Don't try to repair if no svn:eol-style is set.
> [ ] Wrap the error for a better error message perhaps saying 'patch and
> target uses different eol' or similar.
> [ ] Adjust the code to allow for repairing eols even when there is no
> svn:eol-style prop. I would go for this one if it's doable.
>
> Before I throw myself to the mercy of the eol-translating code, I ask
> you if there is something you know of hindering us from repairing eols
> where there is no prop set?
The property is the only way for the user to tell us what the EOL
style of the file should be. If the property isn't set, we simply
don't know what the user wants. Then, if the patch and target are
a mixed bag of EOLs, we should go for a best-effort approach.
Let's just say that in case of ambiguity, the target wins.
Else the patch wins.
For example, assuming mixed eol styles in both patch and target,
and no fixed EOL-style on the target:
Original line in patch says '\n', target says '\r'
If modified line in patch changes to '\r': use '\r'
If modified line in patch says at '\n': use '\r'
Original line in patch says '\n', target says '\n'
If modified line in patch changes to '\r': use '\r'
If modified line in patch says at '\n': use '\n'
This second example makes patches which change only line endings work.
Note that this is per *line*, not per hunk. We make those
decisions on a per-line basis. There are 3 supported EOL styles
so the above examples need to be generalised.
> * We falsely label targets with inconsistent eols to be of type
> eol_style_fixed. But after patching the eols will be consistent and
> that's a good thing.
But what if people really want files with mixed EOLs?
> * [Placeholder for more stuff that can go wrong]
>
> Or we could set the style to svn_subst_eol_style_native. We will end up
> with native newlines in the target then, right?
If the target has svn:eol-style 'native', it will have a consistent
EOL-style, unless the file has been messed with. But if it has been
messed with, writing data through the translation stream will normalise
newlines again. So the patching process will repair broken EOLs in this
case.
Stefan
Re: Why non matching eols in patch and target. (svn patch related)
Posted by Daniel Näslund <da...@longitudo.com>.
On Tue, Feb 23, 2010 at 08:57:26PM +0100, Daniel Näslund wrote:
> On Tue, Feb 02, 2010 at 09:11:52PM +0100, Stefan Sperling wrote:
> > On Tue, Feb 02, 2010 at 08:48:43PM +0100, Daniel Näslund wrote:
[..]
> What to do?
> [ ] Write documentation saying we can only repair eols on targets with a
> svn:eol-style prop.
> [ ] Wrap the error for a better error message perhaps saying 'patch and
> target uses different eol' or similar.
> [ ] Adjust the code to allow for repairing eols even when there is no
> svn:eol-style prop. I would go for this one if it's doable.
>
> Before I throw myself to the mercy of the eol-translating code, I ask
> you if there is something you know of hindering us from repairing eols
> where there is no prop set?
>
> In init_patch_target() we decide how the eol translation of
> target->patched will be done:
> [[[
>
> eol_style_val = apr_hash_get(props, SVN_PROP_EOL_STYLE,
> APR_HASH_KEY_STRING);
> if (eol_style_val)
> {
> svn_subst_eol_style_from_value(&target->eol_style,
> &target->eol_str,
> eol_style_val->data);
> }
> else
> {
> /* Just use the first EOL sequence we can find in the file. */
> SVN_ERR(svn_eol__detect_file_eol(&target->eol_str,
> target->file, scratch_pool));
> /* But don't enforce any particular EOL-style. */
> target->eol_style = svn_subst_eol_style_none;
> }
>
> [...]
>
> target->patched = svn_subst_stream_translated(
> target->patched_raw,
> target->eol_str,
> target->eol_style ==
> svn_subst_eol_style_fixed,
> target->keywords, TRUE, result_pool);
> ]]]
>
> You decided to not enforce any particular eol-style if we don't have a
> prop. The eol_style is used for the cases where there is an eol-style
> prop. Fair enough. But couldn't we do some hack where we always say that
> the eol-style is fixed and use whatever eol we find in the target? I did
> a quick try and got a lot of rejected hunks...
>
> What can go wrong with this approach?
> -------------------------------------
> * We falsely label targets with inconsistent eols to be of type
> eol_style_fixed. But after patching the eols will be consistent and
> that's a good thing.
> * [Placeholder for more stuff that can go wrong]
>
> Or we could set the style to svn_subst_eol_style_native. We will end up
> with native newlines in the target then, right?
Oups, lost the thread there. The eol-style was only used to check for
style_fixed, and if so translate eols. The question remains; why
can't we translate files when there is no no eol-style prop. Is it worth
investigating further?
Daniel
Re: Why non matching eols in patch and target. (svn patch related)
Posted by Daniel Näslund <da...@longitudo.com>.
On Tue, Feb 02, 2010 at 09:11:52PM +0100, Stefan Sperling wrote:
> On Tue, Feb 02, 2010 at 08:48:43PM +0100, Daniel Näslund wrote:
> > Hi Stefan!
> >
> > In match_hunk() we try to match lines from the context of the patch with
> > lines in the target. Earlier, in init_patch_target() we detect the eol
> > of the target and open streams to read from the target and to write the
> > patched result. Those streams does translation of keywords and eols.
> >
> > In match_hunk we read a line from original_text and translate it. But we
> > don't get any translation of the eols in hunk_line_translated.
>
> svn patch only repairs EOLs if the svn:eol-style enforces a fixed
> value (such as CR or CRLF). Try setting svn:eol-style to 'CRLF' on
> the patch target. Then you'll see dos-style newlines in the patched result.
>
> Admittedly, we may want to repair EOLs in more scenarios (such as
> eol-style = native).
I have a test where the target uses '\r\n' and the patch uses '\r' . The
eols are consistent within each file but we get a failure saying:
[[[
subversion/svn/patch-cmd.c:81: (apr_err=135000)
subversion/libsvn_client/patch.c:1463: (apr_err=135000)
subversion/libsvn_client/patch.c:1410: (apr_err=135000)
subversion/libsvn_client/patch.c:1100: (apr_err=135000)
subversion/libsvn_client/patch.c:830: (apr_err=135000)
subversion/libsvn_client/patch.c:799: (apr_err=135000)
subversion/libsvn_subr/subst.c:876: (apr_err=135000)
subversion/libsvn_subr/subst.c:643: (apr_err=135000)
svn: Inconsistent line ending style
]]]
What to do?
[ ] Write documentation saying we can only repair eols on targets with a
svn:eol-style prop.
[ ] Wrap the error for a better error message perhaps saying 'patch and
target uses different eol' or similar.
[ ] Adjust the code to allow for repairing eols even when there is no
svn:eol-style prop. I would go for this one if it's doable.
Before I throw myself to the mercy of the eol-translating code, I ask
you if there is something you know of hindering us from repairing eols
where there is no prop set?
In init_patch_target() we decide how the eol translation of
target->patched will be done:
[[[
eol_style_val = apr_hash_get(props, SVN_PROP_EOL_STYLE,
APR_HASH_KEY_STRING);
if (eol_style_val)
{
svn_subst_eol_style_from_value(&target->eol_style,
&target->eol_str,
eol_style_val->data);
}
else
{
/* Just use the first EOL sequence we can find in the file. */
SVN_ERR(svn_eol__detect_file_eol(&target->eol_str,
target->file, scratch_pool));
/* But don't enforce any particular EOL-style. */
target->eol_style = svn_subst_eol_style_none;
}
[...]
target->patched = svn_subst_stream_translated(
target->patched_raw,
target->eol_str,
target->eol_style ==
svn_subst_eol_style_fixed,
target->keywords, TRUE, result_pool);
]]]
You decided to not enforce any particular eol-style if we don't have a
prop. The eol_style is used for the cases where there is an eol-style
prop. Fair enough. But couldn't we do some hack where we always say that
the eol-style is fixed and use whatever eol we find in the target? I did
a quick try and got a lot of rejected hunks...
What can go wrong with this approach?
-------------------------------------
* We falsely label targets with inconsistent eols to be of type
eol_style_fixed. But after patching the eols will be consistent and
that's a good thing.
* [Placeholder for more stuff that can go wrong]
Or we could set the style to svn_subst_eol_style_native. We will end up
with native newlines in the target then, right?
What say you?
Daniel
Re: Why non matching eols in patch and target. (svn patch related)
Posted by Daniel Näslund <da...@longitudo.com>.
On Tue, Feb 02, 2010 at 09:11:52PM +0100, Stefan Sperling wrote:
> On Tue, Feb 02, 2010 at 08:48:43PM +0100, Daniel Näslund wrote:
> > Hi Stefan!
> >
> > In match_hunk() we try to match lines from the context of the patch with
> > lines in the target. Earlier, in init_patch_target() we detect the eol
> > of the target and open streams to read from the target and to write the
> > patched result. Those streams does translation of keywords and eols.
> >
> > In match_hunk we read a line from original_text and translate it. But we
> > don't get any translation of the eols in hunk_line_translated.
>
> svn patch only repairs EOLs if the svn:eol-style enforces a fixed
> value (such as CR or CRLF). Try setting svn:eol-style to 'CRLF' on
> the patch target. Then you'll see dos-style newlines in the patched result.
>
> Admittedly, we may want to repair EOLs in more scenarios (such as
> eol-style = native).
I have a test where the target uses '\r\n' and the patch uses '\r' . The
eols are consistent within each file but we get a failure saying:
[[[
subversion/svn/patch-cmd.c:81: (apr_err=135000)
subversion/libsvn_client/patch.c:1463: (apr_err=135000)
subversion/libsvn_client/patch.c:1410: (apr_err=135000)
subversion/libsvn_client/patch.c:1100: (apr_err=135000)
subversion/libsvn_client/patch.c:830: (apr_err=135000)
subversion/libsvn_client/patch.c:799: (apr_err=135000)
subversion/libsvn_subr/subst.c:876: (apr_err=135000)
subversion/libsvn_subr/subst.c:643: (apr_err=135000)
svn: Inconsistent line ending style
]]]
What to do?
[ ] Write documentation saying we can only repair eols on targets with a
svn:eol-style prop.
[ ] Wrap the error for a better error message perhaps saying 'patch and
target uses different eol' or similar.
[ ] Adjust the code to allow for repairing eols even when there is no
svn:eol-style prop. I would go for this one if it's doable.
Before I throw myself to the mercy of the eol-translating code, I ask
you if there is something you know of hindering us from repairing eols
where there is no prop set?
In init_patch_target() we decide how the eol translation of
target->patched will be done:
[[[
eol_style_val = apr_hash_get(props, SVN_PROP_EOL_STYLE,
APR_HASH_KEY_STRING);
if (eol_style_val)
{
svn_subst_eol_style_from_value(&target->eol_style,
&target->eol_str,
eol_style_val->data);
}
else
{
/* Just use the first EOL sequence we can find in the file. */
SVN_ERR(svn_eol__detect_file_eol(&target->eol_str,
target->file, scratch_pool));
/* But don't enforce any particular EOL-style. */
target->eol_style = svn_subst_eol_style_none;
}
[...]
target->patched = svn_subst_stream_translated(
target->patched_raw,
target->eol_str,
target->eol_style ==
svn_subst_eol_style_fixed,
target->keywords, TRUE, result_pool);
]]]
You decided to not enforce any particular eol-style if we don't have a
prop. The eol_style is used for the cases where there is an eol-style
prop. Fair enough. But couldn't we do some hack where we always say that
the eol-style is fixed and use whatever eol we find in the target? I did
a quick try and got a lot of rejected hunks...
What can go wrong with this approach?
-------------------------------------
* We falsely label targets with inconsistent eols to be of type
eol_style_fixed. But after patching the eols will be consistent and
that's a good thing.
* [Placeholder for more stuff that can go wrong]
Or we could set the style to svn_subst_eol_style_native. We will end up
with native newlines in the target then, right?
What say you?
Daniel
Re: Why non matching eols in patch and target. (svn patch related)
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Feb 02, 2010 at 08:48:43PM +0100, Daniel Näslund wrote:
> Hi Stefan!
>
> In match_hunk() we try to match lines from the context of the patch with
> lines in the target. Earlier, in init_patch_target() we detect the eol
> of the target and open streams to read from the target and to write the
> patched result. Those streams does translation of keywords and eols.
>
> In match_hunk we read a line from original_text and translate it. But we
> don't get any translation of the eols in hunk_line_translated.
svn patch only repairs EOLs if the svn:eol-style enforces a fixed
value (such as CR or CRLF). Try setting svn:eol-style to 'CRLF' on
the patch target. Then you'll see dos-style newlines in the patched result.
Admittedly, we may want to repair EOLs in more scenarios (such as
eol-style = native).
> I've set the eol_str argument in svn_subst_translate_cstring2() to
> "\r\n" for testing but it still doesn't use "\r\n" for eol. Can there be
> an error in svn_subst_translate_cstring2()?
No, it's working as expected.
> /* Contract keywords, if any, before matching. */
> SVN_ERR(svn_subst_translate_cstring2(hunk_line->data,
> &hunk_line_translated,
> "\r\n, FALSE,
Try passing TRUE for the 'repair' parameter:
"\r\n", TRUE,
Stefan
> target->keywords, FALSE,
> iterpool));
Re: Why non matching eols in patch and target. (svn patch related)
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Feb 02, 2010 at 08:48:43PM +0100, Daniel Näslund wrote:
> Hi Stefan!
>
> In match_hunk() we try to match lines from the context of the patch with
> lines in the target. Earlier, in init_patch_target() we detect the eol
> of the target and open streams to read from the target and to write the
> patched result. Those streams does translation of keywords and eols.
>
> In match_hunk we read a line from original_text and translate it. But we
> don't get any translation of the eols in hunk_line_translated.
svn patch only repairs EOLs if the svn:eol-style enforces a fixed
value (such as CR or CRLF). Try setting svn:eol-style to 'CRLF' on
the patch target. Then you'll see dos-style newlines in the patched result.
Admittedly, we may want to repair EOLs in more scenarios (such as
eol-style = native).
> I've set the eol_str argument in svn_subst_translate_cstring2() to
> "\r\n" for testing but it still doesn't use "\r\n" for eol. Can there be
> an error in svn_subst_translate_cstring2()?
No, it's working as expected.
> /* Contract keywords, if any, before matching. */
> SVN_ERR(svn_subst_translate_cstring2(hunk_line->data,
> &hunk_line_translated,
> "\r\n, FALSE,
Try passing TRUE for the 'repair' parameter:
"\r\n", TRUE,
Stefan
> target->keywords, FALSE,
> iterpool));