You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Erik Huelsmann <eh...@gmail.com> on 2005/12/05 21:31:42 UTC
Fixing blame's eol sensitivity (plans for the last round)
svn_client_blame2 was reported to be broken with 'native' eol style on
Windows. This happens to be our only platform to need translation
from the normal form to have its eols fixed, so, if something is wrong
wrt eols, it's detected there.
The problem however is a bit bigger than I expected. In r17514 and
r17536 I commetted what I thought were fixes for issue #2431. Though
they happen to resolve the symptom, I'll need to change a lot to call
it an actual fix, because in the process, I broke multi-rev blames
(which is kind of the point of blame, isn't it :-).
So, what's wrong? Well, currently, trunk translates the file from
normal form into the right eols and then calculates the blame. So far
so good. On the next iteration however, it takes this detranslated
file and applies the svndiff to it which it should have applied to the
normal form. BAD.
Not as bad as I expected, because the file is being read on a
line-by-line basis in svn_client_blame, after the blame data has been
collected. Currently, this is done with a default "\n" line ending,
but r17514 added code to find out about the requested eol_str eol
style, so, if I translate the file to that line ending (on the fly, by
using the translating stream) and then read from it with the correct
line ending, the line endings issue should be solved.
Well, it's not: if I read from the stream (using svn_stream_readline),
the eol marker isn't part of the returned string. This means I need a
way to marshall it to the client. Creating a new blame receiver
callback to do that would require revving the blame receiver *and*
svn_client_blame2. Creating svn_client_blame3 and adding a parameter
to tell it to either in- or exclude the line ending would eliminate
the need to rev the blame receiver as well.
In libsvn_client/blame.c this will all get quite messy, since the
current code passes CR's to the blame receiver, even if it's part of
the eol marker. This behaviour should probably be kept for backward
compatibility, but the new code should eat the eol all the way :-)
So, to cut a long story short: I'll be creating svn_client_blame3,
which takes the same blame callback, but possibly includes eols in the
when returned line value. When a callback is passed to
svn_client_blame2, it keeps its current behaviour of
not-eating-CR's-but-eating-LFs.
Comments?
bye,
Erik.
Re: Fixing blame's eol sensitivity (plans for the last round)
Posted by Ivan Zhakov <ch...@gmail.com>.
On 12/7/05, Erik Huelsmann <eh...@gmail.com> wrote:
> On 12/7/05, Peter N. Lundblad <pe...@famlundblad.se> wrote:
> > On Wed, 7 Dec 2005, Erik Huelsmann wrote:
> >
> > > On 12/7/05, Julian Foad <ju...@btopenworld.com> wrote:
> > > Erik Huelsmann wrote:
> What I was proposing is that we create a blame callback which takes an
> extra argument 'const char *eol_str' which - in this example - would
> point to "\r\n" and be passed the "Hi! I'm line 500 in foo.txt." (ie
> without eol marker).
I'd like this way, I consider it is better for GUI clients. But you
should sure that it works when svn:eol-style doesn't defined. In other
words: guarantee that we detected and removed EOL characters in any
situation.
--
Ivan Zhakov
Re: Fixing blame's eol sensitivity (plans for the last round)
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Wed, 7 Dec 2005, Erik Huelsmann wrote:
> On 12/7/05, Peter N. Lundblad <pe...@famlundblad.se> wrote:
> On Wed, 7 Dec 2005, Erik Huelsmann wrote:
>
> > On 12/7/05, Julian Foad <ju...@btopenworld.com> wrote:
> > Erik Huelsmann wrote:
> > > So, to cut a long story short: I'll be creating svn_client_blame3,
> > > which takes the same blame callback, but possibly includes eols in the
> > > when returned line value. When a callback is passed to
> > > svn_client_blame2, it keeps its current behaviour of
> > > not-eating-CR's-but-eating-LFs.
> >
> > From an API point of view, that sounds too messy. Just pick a good interface
> > (either one that returns the EOLs, or one that doesn't) and stick with it. I
> > don't see the need for a run-time option.
>
> >Well, then we need an interface which marshalls the EOL style to the
> >blame callback, because in the XML output you *don't* want EOLs
> >included, whereas in the 'normal' case, you *do* want the correct EOLs
> >included....
>
> As far as I remember, we don't output the lines in XML at all because of
> encoding issues.
>Exactly. And because of it, we want to be able to tell the difference
>between the eol markers from the actual text on the blamed line.
You noted below that this is not a rpblem, so I don't need to say that.
Currently, the XML output is irrelevant for this discussion.
Let's take line 500 from a file foo.txt which has line ending style
'native'. This means the line looks like this when seen by the blame
algorithm:
"Hi! I'm line 500 in foo.txt.\n"
Now, on Windows, in a working copy, this line looks like this:
"Hi! I'm line 500 in foo.txt.\r\n"
Currently, the trailing \n gets eaten, but not the trailing \r. This
means that the blame callback is called with this argument:
"Hi! I'm line 500 in foo.txt.\r"
When writing this to the 'normal' blame output, this line gets a \n
added, leading to the intended:
"Hi! I'm line 500 in foo.txt.\r\n"
But that seems wrong. If a client doesn't want to output the newline,
it should be able to do so, resulting in
"Hi! I'm line 500 in foo.txt."
instead of the current
"Hi! I'm line 500 in foo.txt.\r"
>So, what I'm saying is that it should be possible to tell blame either to
>send
> "Hi! I'm line 500 in foo.txt."
>(no newline, as it does now for \n style eols), or
> "Hi! I'm line 500 in foo.txt.\r\n"
>which it currently never does.
>What I was proposing is that we create a blame callback which takes an
>extra argument 'const char *eol_str' which - in this example - would
>point to "\r\n" and be passed the "Hi! I'm line 500 in foo.txt." (ie
>without eol marker).
I think including the CR in the lines provided to the callback is a bug
and doesn't need to be preserved.
So, we have two choices:
a) Either keep the current bevaviour (but fixed in the CRLF (and CR?)
case)
or
b) Change the behaviour to include the eol markers when calling the
callback. This requires revving the svn_client_bblame2 API for a semantic
change.
a) means that we always normalize the output of svn blame to use the EOL
style of the platform. I think that's fine. If the caller really needs
the actual EOL marker, it can grab the file and use the line number. But
I hope there are more interesting content on the line than the eol
marker:-)
If we choose b), that means that the caller can choose to either output
the exact eol marker or normalize it to the platform's EOL marker. While
this might sound flexible, I have a hard time justifying the new API for
this obscure case. Note that you wouldn't need to provide the eol marker
separately, the caller can easily identify it at the end of the line
itself.
I like solution a). It menas that you don't need to rev the API. Note
that the diff library now supports all eol styles we support. I wonder
how many people use files with CR line endings, but anyway...
Thanks,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Fixing blame's eol sensitivity (plans for the last round)
Posted by Erik Huelsmann <eh...@gmail.com>.
On 12/7/05, Peter N. Lundblad <pe...@famlundblad.se> wrote:
> On Wed, 7 Dec 2005, Erik Huelsmann wrote:
>
> > On 12/7/05, Julian Foad <ju...@btopenworld.com> wrote:
> > Erik Huelsmann wrote:
> > > So, to cut a long story short: I'll be creating svn_client_blame3,
> > > which takes the same blame callback, but possibly includes eols in the
> > > when returned line value. When a callback is passed to
> > > svn_client_blame2, it keeps its current behaviour of
> > > not-eating-CR's-but-eating-LFs.
> >
> > From an API point of view, that sounds too messy. Just pick a good interface
> > (either one that returns the EOLs, or one that doesn't) and stick with it. I
> > don't see the need for a run-time option.
>
> >Well, then we need an interface which marshalls the EOL style to the
> >blame callback, because in the XML output you *don't* want EOLs
> >included, whereas in the 'normal' case, you *do* want the correct EOLs
> >included....
>
> As far as I remember, we don't output the lines in XML at all because of
> encoding issues.
Exactly. And because of it, we want to be able to tell the difference
between the eol markers from the actual text on the blamed line.
> And if we did, why wouldn't we want the EOL markers in
> the XML output?
>
>
> >As I said, I can also rev the blame callback api and include the eol
> >marker in each call to the callback, but that too seems ugly...
> Why is that ugly? Isn't the eol marker part of the line?
I see I'm making not much sense. I'll give the current and desired
situations below. The desired situation is what I'm trying to
establish (ie. is what I *think* is desired).
Let's take line 500 from a file foo.txt which has line ending style
'native'. This means the line looks like this when seen by the blame
algorithm:
"Hi! I'm line 500 in foo.txt.\n"
Now, on Windows, in a working copy, this line looks like this:
"Hi! I'm line 500 in foo.txt.\r\n"
Currently, the trailing \n gets eaten, but not the trailing \r. This
means that the blame callback is called with this argument:
"Hi! I'm line 500 in foo.txt.\r"
When writing this to the 'normal' blame output, this line gets a \n
added, leading to the intended:
"Hi! I'm line 500 in foo.txt.\r\n"
But that seems wrong. If a client doesn't want to output the newline,
it should be able to do so, resulting in
"Hi! I'm line 500 in foo.txt."
instead of the current
"Hi! I'm line 500 in foo.txt.\r"
So, what I'm saying is that it should be possible to tell blame either to send
"Hi! I'm line 500 in foo.txt."
(no newline, as it does now for \n style eols), or
"Hi! I'm line 500 in foo.txt.\r\n"
which it currently never does.
What I was proposing is that we create a blame callback which takes an
extra argument 'const char *eol_str' which - in this example - would
point to "\r\n" and be passed the "Hi! I'm line 500 in foo.txt." (ie
without eol marker).
BTW: I just saw there's absolutely *no* output included in the blame
xml, so that's not a problem at all.
I hope this is more clear?
bye,
Erik.
PS: If it's not, never mind, just propose a good way to resolve the
situation and I'll implement it. I myself think it won't be too bad to
choose to include the eol marker in the returned line content. But
that would require revving the svn_client_blame2 api, since the intent
seems to be not to include it.
Re: Fixing blame's eol sensitivity (plans for the last round)
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Wed, 7 Dec 2005, Erik Huelsmann wrote:
> On 12/7/05, Julian Foad <ju...@btopenworld.com> wrote:
> Erik Huelsmann wrote:
> > So, to cut a long story short: I'll be creating svn_client_blame3,
> > which takes the same blame callback, but possibly includes eols in the
> > when returned line value. When a callback is passed to
> > svn_client_blame2, it keeps its current behaviour of
> > not-eating-CR's-but-eating-LFs.
>
> From an API point of view, that sounds too messy. Just pick a good interface
> (either one that returns the EOLs, or one that doesn't) and stick with it. I
> don't see the need for a run-time option.
>Well, then we need an interface which marshalls the EOL style to the
>blame callback, because in the XML output you *don't* want EOLs
>included, whereas in the 'normal' case, you *do* want the correct EOLs
>included....
As far as I remember, we don't output the lines in XML at all because of
encoding issues. And if we did, why wouldn't we want the EOL markers in
the XML output?
>As I said, I can also rev the blame callback api and include the eol
>marker in each call to the callback, but that too seems ugly...
Why is that ugly? Isn't the eol marker part of the line?
Thanks,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Fixing blame's eol sensitivity (plans for the last round)
Posted by Erik Huelsmann <eh...@gmail.com>.
On 12/7/05, Julian Foad <ju...@btopenworld.com> wrote:
> Erik Huelsmann wrote:
> > svn_client_blame2 was reported to be broken with 'native' eol style on
> > Windows. This happens to be our only platform to need translation
> > from the normal form to have its eols fixed, so, if something is wrong
> > wrt eols, it's detected there.
> >
> > The problem however is a bit bigger than I expected. In r17514 and
> > r17536 I commetted what I thought were fixes for issue #2431. Though
> > they happen to resolve the symptom, I'll need to change a lot to call
> > it an actual fix, because in the process, I broke multi-rev blames
> > (which is kind of the point of blame, isn't it :-).
>
> [...]
>
> > In libsvn_client/blame.c this will all get quite messy, since the
> > current code passes CR's to the blame receiver, even if it's part of
> > the eol marker. This behaviour should probably be kept for backward
> > compatibility, but the new code should eat the eol all the way :-)
>
> That behaviour should perhaps be kept for svn_client_blame2() for backward
> compatibility, though if it's not documented we can call it a bug and fix it.
>
> That behaviour certainly does not need to be kept as an option in
> svn_client_blame3().
Exactly my thoughts.
> > So, to cut a long story short: I'll be creating svn_client_blame3,
> > which takes the same blame callback, but possibly includes eols in the
> > when returned line value. When a callback is passed to
> > svn_client_blame2, it keeps its current behaviour of
> > not-eating-CR's-but-eating-LFs.
>
> From an API point of view, that sounds too messy. Just pick a good interface
> (either one that returns the EOLs, or one that doesn't) and stick with it. I
> don't see the need for a run-time option.
Well, then we need an interface which marshalls the EOL style to the
blame callback, because in the XML output you *don't* want EOLs
included, whereas in the 'normal' case, you *do* want the correct EOLs
included....
As I said, I can also rev the blame callback api and include the eol
marker in each call to the callback, but that too seems ugly...
bye,
Erik.
Re: Fixing blame's eol sensitivity (plans for the last round)
Posted by Julian Foad <ju...@btopenworld.com>.
Erik Huelsmann wrote:
> svn_client_blame2 was reported to be broken with 'native' eol style on
> Windows. This happens to be our only platform to need translation
> from the normal form to have its eols fixed, so, if something is wrong
> wrt eols, it's detected there.
>
> The problem however is a bit bigger than I expected. In r17514 and
> r17536 I commetted what I thought were fixes for issue #2431. Though
> they happen to resolve the symptom, I'll need to change a lot to call
> it an actual fix, because in the process, I broke multi-rev blames
> (which is kind of the point of blame, isn't it :-).
[...]
> In libsvn_client/blame.c this will all get quite messy, since the
> current code passes CR's to the blame receiver, even if it's part of
> the eol marker. This behaviour should probably be kept for backward
> compatibility, but the new code should eat the eol all the way :-)
That behaviour should perhaps be kept for svn_client_blame2() for backward
compatibility, though if it's not documented we can call it a bug and fix it.
That behaviour certainly does not need to be kept as an option in
svn_client_blame3().
> So, to cut a long story short: I'll be creating svn_client_blame3,
> which takes the same blame callback, but possibly includes eols in the
> when returned line value. When a callback is passed to
> svn_client_blame2, it keeps its current behaviour of
> not-eating-CR's-but-eating-LFs.
From an API point of view, that sounds too messy. Just pick a good interface
(either one that returns the EOLs, or one that doesn't) and stick with it. I
don't see the need for a run-time option.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org