Reviving the Blame Branch
Tim Hatch
touched in about a year. This would be pretty much impossible to merge
with trunk at this point, so what does everyone think about creating a
new one under sandbox for collaborative work[1]?
Tim
Matt Good
Tim Hatch wrote:
> I notice that there's a cmlenz-dev/blame branch that hasn't been
> touched in about a year. This would be pretty much impossible to merge
> with trunk at this point, so what does everyone think about creating a
> new one under sandbox for collaborative work[1]?
+1. The blame support itself would be handy, but it'll also enable
other useful annotations, such as enabling Bitten to provide a view of
the test coverage for a file.
-- Matt Good
Christian Boos
I'm also OK to revive this effort, of course, but this is a quite
complex topic.
There's the annotation part, which are perhaps better done in the
mimeview refactoring branch, as we go.
Then, there's the annotate algorithm itself. There are several
approaches here: make it backend specific, make it generic (at the cache
level).
Note that this operation is going to be _very_ inefficient if done the
naive way (i.e. get the full text of all the involved revision, and see
what changed by ourselves). So probably better is to do it the backend
specific way.
For Mercurial, it's near to trivial to reuse the existing
filectxt.annotate() method and get the needed sequence of (changeset,
line) out of it.
For Subversion, it looks a lot more complex to get it done right, as
we'd need to use the svnclient API and I'm not sure if all the bits and
pieces needed are available to the bindings.
-- Christian
||
Christian Boos
> ...
> For Subversion, it looks a lot more complex to get it done right, as
> we'd need to use the svnclient API and I'm not sure if all the bits and
> pieces needed are available to the bindings.
>
Just a quick update: I got curious and wanted to check the above.
It seems that the needed API is actually usable... that's great news, so
let's do it ;)
This seems to work for file: and svn: URLs only, at least for me with
Subversion 1.4.0 on windows, as I got crashes in _client.dll when trying
out http: URLs...
-- Christian
Erik Huelsmann
Could you send a reproduction recipe for that to
d...@subversion.tigris.org please? We'll want to fix that in 1.4.4 as
soon as it comes out.
bye,
Erik Huelsmann
Subversion developer.
Noah Kantrowitz
Also you might want to look at the PeerReview plugin (warning:
insanely ugly code), as that would greatly benefit from a generic
annotation API.
--Noah
Tim Hatch
> It seems that the needed API is actually usable...
I'm also working on a better naïve way to make the blame, in case the
vc api doesn't provide one. I think if possible it's best to start
from the head (so once all lines are annotated, it will stop fetching
older revs).
Tim
Christian Boos
> ...
>> This seems to work for file: and svn: URLs only, at least for me with
>> Subversion 1.4.0 on windows, as I got crashes in _client.dll when trying
>> out http: URLs...
>>
>
> Could you send a reproduction recipe for that to
> d...@subversion.tigris.org please? We'll want to fix that in 1.4.4 as
> soon as it comes out.
>
Done. For reference:
http://groups.google.com/group/Subversion-development/browse_frm/thread/a8c7a00980f24a6f/ec0970c5cd73f5eb
-- Christian
Christian Boos
>> Just a quick update: I got curious and wanted to check the above.
>> It seems that the needed API is actually usable...
>>
>
> I'm also working on a better naïve way to make the blame, in case the
> vc api doesn't provide one.
Great, that could be used to annotate content stored in our "naive"
version control system, i.e. the wiki pages, the ticket descriptions,
etc. ;)
> I think if possible it's best to start
> from the head (so once all lines are annotated, it will stop fetching
> older revs).
Well, we should start with whatever version is currently browsed.
Remember that not everything is always visible in the head, and that we
often want to see what was there /before/ a given version (hence the
idea to add annotate links in the annotation cell itself, see
http://trac.edgewall.org/ticket/3023#comment:4).
-- Christian
Christian Boos
> I notice that there's a cmlenz-dev/blame branch that hasn't been
> touched in about a year. This would be pretty much impossible to merge
> with trunk at this point, so what does everyone think about creating a
> new one under sandbox for collaborative work[1]?
>
Ok, to get the ball rolling, I've committed a first shot at the blame
support, see:
http://trac.edgewall.org/changeset/4481/sandbox/blame
As expected, the support for annotate was quite trivial to add for
Mercurial (r4482), but as opposed to what I expected, turned out to be
pretty easy for Subversion as well ;)
Have fun,
-- Christian
Christian Boos
> Tim Hatch wrote:
>> I notice that there's a cmlenz-dev/blame branch that hasn't been
>> touched in about a year. This would be pretty much impossible to merge
>> with trunk at this point, so what does everyone think about creating a
>> new one under sandbox for collaborative work[1]?
>>
>
> Ok, to get the ball rolling, I've committed a first shot at the blame
> support, see:
>
Hello everybody, and happy new year!
I've enhanced the blame support quite a bit, and now I think it looks
really good:
http://trac.edgewall.org/attachment/wiki/ChristianBoos/blame-screenshot2.png
Please try out the blame branch (svn co
https://svn.edgewall.org/repos/trac/sandbox/blame).
Tim is also working on doing our own blame algorithm, which will be
useful in order to annotate other versioned content (wiki pages, ticket
description, ...). To that end, I'm thinking of adding a resource API,
in connection with the WikiContext API and the work Alec started in the
security branch. The idea would be to get to the versioned content
starting from the context:
Currently I do (in the BlameAnnotator):
node = self.repos.get_node(self.context.id, rev)
# -- get revision numbers for each line
self.annotations = node.get_annotations()
# -- from the annotations, retrieve changesets and
# determine the span of dates covered, for the color code.
self.changesets = []
chgset = self.repos.get_changeset(rev)
This would become something like:
node = self.context.resource # (*)
self.annotations = node.get_annotations()
self.changesets = []
chgset = node.get_changeset(rev)
and would work for any kind of resource, not just repository source files.
(*) after the rename suggested by Alec: s/resource/realm/ and
s/object/resource/
-- Christian
W Craig Trader
Very pretty! How soon before you think you could merge it into the trunk?
- Craig -
Jonas Borgström
> Christian Boos wrote:
>> Tim Hatch wrote:
>>> I notice that there's a cmlenz-dev/blame branch that hasn't been
>>> touched in about a year. This would be pretty much impossible to merge
>>> with trunk at this point, so what does everyone think about creating a
>>> new one under sandbox for collaborative work[1]?
>>>
>> Ok, to get the ball rolling, I've committed a first shot at the blame
>> support, see:
>>
>
> Hello everybody, and happy new year!
>
> I've enhanced the blame support quite a bit, and now I think it looks
> really good:
>
> http://trac.edgewall.org/attachment/wiki/ChristianBoos/blame-screenshot2.png
>
Cool, looks really nice!
Since I guess these pages are pretty expensive to generate, would it be
a good idea to add "<meta name="ROBOTS" content="NOINDEX"/>"-tags to the
template?
(To stop search engines like Google from killing public trac sites)
Cheers,
Jonas
Christian Boos
> Very pretty! How soon before you think you could merge it into the
> trunk?
I'd like to integrate it in two steps. First, what's already there, with
some additional polishing (like what jonas suggested). This can happen
quite soon. Then, go on working on the annotation of other contents,
like outlined in my previous mail. That work will probably require a
longer stabilization period, more design discussions. It could even not
make it in time for 0.11 and I don't want to see the /whole/ blame
feature stay in a branch in the meantime.
-- Christian
Christian Boos
> ...
> Since I guess these pages are pretty expensive to generate, would it be
> a good idea to add "<meta name="ROBOTS" content="NOINDEX"/>"-tags to the
> template?
Right, those pages shouldn't show up in searches (noindex), but we
should probably also prevent following the numerous changeset links
found there as well (nofollow):
<meta name="robots" content="noindex,nofollow">
We should probably disable following links for regular browser pages as
well (directory listings at the very least), as spidering a Subversion
repository with lots of branches is also very expensive. We should allow
those pages to be indexed, though.
<meta name="robots" content="nofollow">
(Reference: http://www.robotstxt.org/wc/meta-user.html)
-- Christian
Christian Boos
The blame branch looks ready for integration in trunk to me, so I'm
asking for review (1) and approval.
> Jonas Borgström wrote:
>
>> ...
>> Since I guess these pages are pretty expensive to generate, would it be
>> a good idea to add "<meta name="ROBOTS" content="NOINDEX"/>"-tags to the
>> template?
>>
I've implemented the above in r4540 and synced with trunk in the next
changeset.
As mentioned in a previous mail, I'd like to get at least the current
state of the blame support in trunk, so that we can be sure it'll work
fine for 0.11. This doesn't necessarily mean we're done with the blame
support: further improvements can be made later on that branch, like
blame support for wiki and ticket descriptions.
There are a few changes in that blame branch that I'd like to build upon
for finishing #1601: in fact, I'm mostly done with the rendering of
custom property in the browser (2) /except/ the ability to display
additional columns. I'm not sure how to do it at this point, but as this
will likely happen on the areas also modified by the blame branch, I
don't want to create needless conflicts.
-- Christian
(1) -
http://trac.edgewall.org/changeset?old_path=trunk&old=4541&new_path=sandbox%2Fblame&new=4541
(2) -
http://trac.edgewall.org/log/sandbox/property-renderers-tmp?action=stop_on_copy&rev=4530&stop_rev=&mode=stop_on_copy&verbose=on
Tim Hatch
> The blame branch looks ready for integration in trunk to me, so I'm
> asking for review (1) and approval.
+1 on the merge, it's working okay with Subversion for me.
Tim
Jonas Borgström
> Hello again,
>
> The blame branch looks ready for integration in trunk to me, so I'm
> asking for review (1) and approval.
I've tested the branch a bit now and as far as I can tell it works ok.
One thing I'm not sure I like yet is the way the age column in the
directory browser is colorized. IMHO it's too eye-catching since the
rest of the trac ui is pretty colorless. Maybe coloring border-bottom or
something might work better.
Or maybe even dropping the color coding completely and use icons to
indicate when the file/folder was last modified. being able to tell if a
file was modified today/this week/this month/older/ would be a high
enough resolution. Using slightly different colors for "9 months" and
"10 months" is IMHO just confusing.
Cheers,
Jonas
Christian Boos
> Christian Boos wrote:
>
>> Hello again,
>>
>> The blame branch looks ready for integration in trunk to me, so I'm
>> asking for review (1) and approval.
>>
>
> I've tested the branch a bit now and as far as I can tell it works ok.
>
> One thing I'm not sure I like yet is the way the age column in the
> directory browser is colorized. IMHO it's too eye-catching since the
> rest of the trac ui is pretty colorless.
Agreed, that was one of the things I still wanted to improve. What I had
in mind:
- first make it configurable, enabled by default but allow people who
don't like it to turn off the feature
- use a narrow column in front of the age column for the color scale,
not the background of age column itself
> Maybe coloring border-bottom or something might work better.
>
Ok, so I tried both approaches and the above, implemented in r4543,
looks better.
Another idea would be to colorize the text itself , which also looks
good, but that would conflict with the age being turned into a link to
the timeline by the #975 changes, as I couldn't find a way to force the
"color" of the cell to be inherited by the <a> it contains....
Another fine tuning for the CSS would be to improve the rendering of the
annotated source code in IExplorer, as the cell borders are too visible
there.
> Or maybe even dropping the color coding completely and use icons to
> indicate when the file/folder was last modified. being able to tell if a
> file was modified today/this week/this month/older/ would be a high
> enough resolution. Using slightly different colors for "9 months" and
> "10 months" is IMHO just confusing.
>
Well, I'm not sure why you would find that confusing, it's the same
principle used on the annotation page itself.
I don't see what kind of icons you have in mind in this case. I can
think of a (new!) icon for the most recent changes, but don't see which
ones could be used to convey the "today/this week/this month/older"
concepts. OTOH, a color /code/ could achieve that (e.g. today => red,
... older => blue).
Also, suggestions for making the color scale nicer and perhaps more
accessible are welcomed.
In particular, people that can make a test "blame" Trac available on the
net could check the accessibility of the color scale to color-blind
people using http://colorfilter.wickline.org/ (I suspect the current
scale to be quite bad in that respect).
-- Christian
Christian Boos
> ...
> Another idea would be to colorize the text itself , which also looks
> good, but that would conflict with the age being turned into a link to
> the timeline by the #975 changes, as I couldn't find a way to force the
> "color" of the cell to be inherited by the <a> it contains...
Nevermind, Tim showed me the way to do this :-)
So please compare r4543 with r4544 and tell me what looks best.
Btw, I didn't mention so far the great support I got from Tim while I
was working on the blame branch, as he did a lot of testing and gave me
very useful feedback, so ... YATTTH :-)
-- Christian
Matt Good
> Christian Boos wrote:
> > ...
> > Another idea would be to colorize the text itself , which also looks
> > good, but that would conflict with the age being turned into a link to
> > the timeline by the #975 changes, as I couldn't find a way to force the
> > "color" of the cell to be inherited by the <a> it contains...
>
> Nevermind, Tim showed me the way to do this :-)
> So please compare r4543 with r4544 and tell me what looks best.
Welll, I missed Jonas' earlier comment, but I also felt like the colors
were a bit over-saturated for the standard Trac look. This uses a more
consistent color based on the red from the diff view and an equally
saturated blue:
def colorize_age(age):
new = 255,136,136
old = 136,136,255
return tuple([int(b+(a-b)*age) for a,b in zip(new,old)])
We could use a config option for this, since it can't be configured
through CSS like the rest of the Trac L&F.
-- Matt Good
Christian Boos
> Christian Boos wrote:
>
>> Christian Boos wrote:
>>
>>> ...
>>> Another idea would be to colorize the text itself , which also looks
>>> good, but that would conflict with the age being turned into a link to
>>> the timeline by the #975 changes, as I couldn't find a way to force the
>>> "color" of the cell to be inherited by the <a> it contains...
>>>
>> Nevermind, Tim showed me the way to do this :-)
>> So please compare r4543 with r4544 and tell me what looks best.
>>
>
> Welll, I missed Jonas' earlier comment, but I also felt like the colors
> were a bit over-saturated for the standard Trac look. This uses a more
> consistent color based on the red from the diff view and an equally
> saturated blue:
>
> def colorize_age(age):
> new = 255,136,136
> old = 136,136,255
> return tuple([int(b+(a-b)*age) for a,b in zip(new,old)])
>
Perfect! That was somehow the default color scheme I was looking for, no
kidding.
> We could use a config option for this, since it can't be configured
> through CSS like the rest of the Trac L&F.
>
I made those default colors configurable in r4553, as well as adding an
extra (optional) twist: you can configure it in a way that the very last
revisions are highlighted differently than the "normal" scale, so this
can make the latest changeset stand out even more, as illustrated in the
example configuration given in that changeset. But that's not enable by
default. By default, one get exactly the color scale you suggested above.
Also, I've updated the <meta> tags as you suggested elsewhere in r4554.
Looks like we're ready for a merge...
-- Christian
Manuzhai
> Looks like we're ready for a merge...
I upped my install to r4561 and tried to annotate one of my files, but
it merely gained a completely empty column on the left of the source
display. Any ideas on how to fix/debug this?
Regards,
Manuzhai
Christian Boos
Sure, bug the SVN people about it, and djames in particular ;)
Subversion thinks that this file is a binary one, and refuses to blame
it (click on the Rev column header to get the actual error message,
bonus points if you find a way to improve the error rendering - should
be visible but not too obtrusive).
Only blame3 gives the ability to /force/ an annotation when we think
it's not binary, but blame3 can't be used right now, only blame2.
There's a short thread on Subversion-dev about that, you can maybe revive it
(google for blame2 blame3 python)
-- Christian
Manuzhai
> Subversion thinks that this file is a binary one, and refuses to blame
> it (click on the Rev column header to get the actual error message,
> bonus points if you find a way to improve the error rendering - should
> be visible but not too obtrusive).
Clicking the Rev column header doesn't do a thing for me... I do see
this in my log file:
2007-01-13 17:53:46,369 Trac[api] WARNING: Can't use annotator
'blame': svn blame failed: ('Unable to open an ra_local session to
URL', 180001)
Regards,
Manuzhai
Christian Boos
Ah, that's a different beast then.
First the above message should have showed up in the "title" of the th
corresponding to the blame column.
Then, look again in the log, you should see the actual URL used for the
ra_local session, because of the following statement:
self.repos.log.info('opening ra_local session to' + repo_url)
Can you 'svn ls' that URL?
Oh, are you using a scoped repos?
If yes, try this patch:
Index: trac/versioncontrol/svn_fs.py
===================================================================
--- trac/versioncontrol/svn_fs.py (revision 4561)
+++ trac/versioncontrol/svn_fs.py (working copy)
@@ -672,8 +672,7 @@
annotations.append(revision)
rev = _svn_rev(self.rev)
start = _svn_rev(0)
- repo_url = 'file:///%s%s' % (self.repos.path,
- self._scoped_svn_path)
+ repo_url = 'file:///%s' % self.repos.path
self.repos.log.info('opening ra_local session to' + repo_url)
from svn import client
try:
That's most certainly it.
-- Christian
Manuzhai
> Ah, that's a different beast then.
> First the above message should have showed up in the "title" of the th
> corresponding to the blame column.
Right, I'm seeing that now (it just shows when I hover the column
header, not when I click it -- in fact, I think clicking the header
probably prevents it from showing).
> Then, look again in the log, you should see the actual URL used for the
> ra_local session, because of the following statement:
>
> self.repos.log.info('opening ra_local session to' + repo_url)
>
> Can you 'svn ls' that URL?
Since I'm using a scoped repos, I applied your patch first. After
that, it still didn't work.
djc@enrai nts $ tail log/trac.log
2007-01-13 18:27:56,866 Trac[svn_fs] INFO: opening ra_local session
tofile:////var/svn/xm
2007-01-13 18:27:56,949 Trac[api] WARNING: Can't use annotator
'blame': svn blame failed: ("'' is not a file", 160017)
djc@enrai nts $ svn ls file:////var/svn/xm
aycha/
equilex/
hebe/
(...)
djc@enrai nts $
> Oh, are you using a scoped repos?
> If yes, try this patch:
>
> Index: trac/versioncontrol/svn_fs.py
> ===================================================================
> --- trac/versioncontrol/svn_fs.py (revision 4561)
> +++ trac/versioncontrol/svn_fs.py (working copy)
> @@ -672,8 +672,7 @@
> annotations.append(revision)
> rev = _svn_rev(self.rev)
> start = _svn_rev(0)
> - repo_url = 'file:///%s%s' % (self.repos.path,
> - self._scoped_svn_path)
> + repo_url = 'file:///%s' % self.repos.path
> self.repos.log.info('opening ra_local session to' + repo_url)
> from svn import client
> try:
>
>
> That's most certainly it.
So, what now? And it shouldn't it become file:///var instead of
file:////var (though it obviously doesn't matter)?
Regards,
Manuzhai
Christian Boos
> On 1/13/07, Christian Boos <cb...@neuf.fr> wrote:
>
>> Ah, that's a different beast then.
>> First the above message should have showed up in the "title" of the th
>> corresponding to the blame column.
>>
>
> Right, I'm seeing that now (it just shows when I hover the column
> header, not when I click it -- in fact, I think clicking the header
> probably prevents it from showing).
>
>
Ok, as I said, this could probably be improved. I have a few ideas, like
allowing the annotator to output a full row in the table heading; that
could be used for error reporting as well as for other fancy stuff...
>> Then, look again in the log, you should see the actual URL used for the
>> ra_local session, because of the following statement:
>>
>> self.repos.log.info('opening ra_local session to' + repo_url)
>>
>> Can you 'svn ls' that URL?
>>
>
> Since I'm using a scoped repos, I applied your patch first. After
> that, it still didn't work.
>
Sorry, that patch was not OK, the URL should point to the file itself.
> ...
> So, what now? And it shouldn't it become file:///var instead of
> file:////var (though it obviously doesn't matter)?
>
So this might have been the reason...
Let's try something else.
Index: svn_fs.py
===================================================================
--- svn_fs.py (revision 4561)
+++ svn_fs.py (working copy)
@@ -672,9 +672,9 @@
annotations.append(revision)
rev = _svn_rev(self.rev)
start = _svn_rev(0)
- repo_url = 'file:///%s%s' % (self.repos.path,
+ repo_url = 'file:///%s%s' % (self.repos.path.lstrip('/'),
self._scoped_svn_path)
- self.repos.log.info('opening ra_local session to' + repo_url)
+ self.repos.log.info('opening ra_local session to ' + repo_url)
from svn import client
try:
client.blame2(repo_url, rev, start, rev, blame_receiver,
-- Christian
Manuzhai
> So this might have been the reason...
> Let's try something else.
I figured it out, I think. See [1] for the patch I came up with... The
problem is with the way SubversionNode stores the repos scope. Instead
of having a scope like SubversionRepository, which always starts and
ends with /, its self._scoped_repos_path is run through _to_svn(),
strips outer slashes. Therefore, the path that blame tries to work
with doesn't contain a slash between the actual repository path and
the actual file path within that repository.
Regards,
Manuzhai
Christian Boos
> On 1/13/07, Christian Boos <cb...@neuf.fr> wrote:
>
>> So this might have been the reason...
>> Let's try something else.
>>
>
> I figured it out, I think. See [1] for the patch I came up with...
Thanks!
> The
> problem is with the way SubversionNode stores the repos scope. Instead
> of having a scope like SubversionRepository, which always starts and
> ends with /, its self._scoped_repos_path is run through _to_svn(),
> strips outer slashes. Therefore, the path that blame tries to work
> with doesn't contain a slash between the actual repository path and
> the actual file path within that repository.
>
Actually, _scoped_repos_path used to start with a '/' in unscoped
repositories and without in scoped repositories. I fixed that in
http://trac.edgewall.org/changeset/4563, now blame should work in both
cases (it does for me on Windows, at least).
-- Christian
Manuzhai
WORKSFORME, thanks!
Regards,
Manuzhai