search

sunsheng / gitiles

Automatically exported from code.google.com/p/gitiles
0 stars 0 forks source link

Display annotated "blame" version of a file #5

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
Annotate the lines of a file with the revision they came from using the blame 
algorithm.

JGit blame is not identical to git-core's blame algorithm. Its a close 
approximation, but there are some differences in how JGit tracks the scoreboard 
and what annotations it can find.

Use /+blame/revision/path as the URL.

Because annotation is slow, results might want to be cached, and the page might 
want to use AJAX to feed down hunks of blame as they are discovered by the 
algorithm. This would be like the incremental blame display visible in git 
gui's blame window.

Original issue reported on code.google.com by dborowitz@google.com on 11 Nov 2012 at 11:25

GoogleCodeExporter commented 9 years ago 
CodeMirror is clever and only renders lines it needs to display. Rendering all 
of render_view_impl.cc with blame takes my MacBook Air >10s so I'm not going to 
do that, at least not by default.

That dialog can be made to capture the system shortcuts, find-as-you-type, find 
next, etc. I just haven't gotten to it yet. What other features of Chrome's 
native search do you consider essential?

It is possible we could make it an option stored in a cookie, but I don't want 
to resort to that until it's been proven we can't emulate Chrome's native 
search to your satisfaction.

Original comment by dborowitz@google.com on 24 Apr 2014 at 5:04

GoogleCodeExporter commented 9 years ago 
I'm strongly against a page that tries to be clever by not rendering 
everything. If code mirror is that slow, then the problem is with code mirror 
and perhaps that's not the best solution to use.

Overriding chrome's find is not feasible. It's the standard way of searching in 
the browser and is what users are trained for. if you use the wrench menu, or 
have the cursor outside the content area and do ctrl+f, it will always come up. 
Same for touch devices, where there won't be a keyboard shortcut and users just 
use the browser's UI.

I think using a cookie to change this search thing is only a hack around code 
mirror being slow. if you insist on using a slow system, then the default 
should be to render everything by default, and a cookie can disable that.

Original comment by jam@chromium.org on 24 Apr 2014 at 5:11

GoogleCodeExporter commented 9 years ago 
Maybe Chrome's JavaScript engine is too slow and the problem is with V8?  :-)

Original comment by sop@google.com on 24 Apr 2014 at 5:14

GoogleCodeExporter commented 9 years ago 
FWIW, Android's gerrit instance also does magic scrolling and magic find to 
deal with large files, and it's a huge PITA for the gerrit-uninitiated (who 
have chrome's ctrl+f wired into muscle-memory).  OTOH, it's also incredibly 
slow on large (2-4kLOC files) when the magic is disabled.  On the third hand, 
viewvc provides an existence proof that it's possible to display all the lines, 
(minimally) syntax-highlighted, linkified, and still have a snappy experience.  
Just sayin'.

Original comment by fischman@chromium.org on 24 Apr 2014 at 5:20

GoogleCodeExporter commented 9 years ago 
@sop: Is this faster in other browsers? If you have a test case that is slower, 
I'm sure the V8 team would be happy to take a look.

I'm not sure what code mirror javascript is doing. For an alternative, see 
viewvc's annotate view which renders pretty much as soon as the data comes off 
the network: 
http://src.chromium.org/viewvc/chrome/trunk/src/content/renderer/render_view_imp
l.cc?annotate=265892

Original comment by jam@chromium.org on 24 Apr 2014 at 5:25

GoogleCodeExporter commented 9 years ago 
I am not convinced we can have everything. ViewVC is fast because it uses a 
table layout, but I promise you if we tried a table layout like ViewVC does we 
would get complaints about breaking multi-line selections :)

Being clever obviously has its drawbacks. Sounds like we're making the same 
tradeoffs as Gerrit, then. I'll add the preference cookie and default to 
rendering the full file.

Original comment by dborowitz@google.com on 24 Apr 2014 at 5:30

GoogleCodeExporter commented 9 years ago 
@dborowitz: multi-line selection is not important in the annotate view. We have 
never had that with viewvc, and I have never heard anyone complain about it. In 
the very rare case that someone needs that, they can view that file at that 
given revision and select multi-lines. Can you try with table layout, and we 
can collect data on how often people complain that they can't do multi-line 
copy?

Basically, what I'm trying to say is that we shouldn't break the most-often 
used case (searching for a line) to fix a very unlikely scenario (multi-line 
copy).

Original comment by jam@chromium.org on 24 Apr 2014 at 5:35

GoogleCodeExporter commented 9 years ago 
viewvc renders highlights server-side, not in javascript, which I have to 
agree, does seem better for this type of application. It would also allow the 
cached pages to be pre-highlighted, which might improve client-side rendering 
performance even more.

Original comment by mmoss@chromium.org on 24 Apr 2014 at 5:40

GoogleCodeExporter commented 9 years ago 
Maybe you're right, switching to server-side syntax highlighting shouldn't be 
too hard. And it's theoretically possible to emulate multiline selections 
across a table in JS (which is basically what CM does), so we can focus our 
cleverness there if/when there are complaints.

And I've been saying all along Gitiles should keep JS to a minimum and do as 
much as possible server side as possible. So thanks for pushing it back to its 
roots there.

Original comment by dborowitz@google.com on 24 Apr 2014 at 5:45

GoogleCodeExporter commented 9 years ago 
Also agree that I don't think multi-line select is that useful, since it's 
readily available in other views. Most of the time, the only thing I'm trying 
to select in a blame view is either a revision # to investigate further, or an 
author email to pester about a particular change, but rarely the code itself. 
In fact, I think the only time I ever tried to select a big chunk of text in a 
blame view was the other day when I was trying to highlight text in the 
screenshot I made for jam, to point out what I was seeing in my browser, so I 
wanted to highlight the code and annotations together, and I couldn't do it :)

Original comment by mmoss@chromium.org on 24 Apr 2014 at 5:45

GoogleCodeExporter commented 9 years ago 
btw some minor comments about the UI
-please don't put it in a frame and instead use the full width of the page
-selection doesn't work on the part to the left of the code (i.e. I can't 
select author emails or hash id)
-the "+0000" aren't adding any information and are just using up width, so no 
need to have them
-regarding the suggestion to abbreviate commit/diff/blame: I think doing so 
will really confuse newcomers, so I hope we don't do that

Original comment by jam@chromium.org on 24 Apr 2014 at 5:50

GoogleCodeExporter commented 9 years ago 
First two comments wouldn't apply to a table layout.

For timezones, +0000 only doesn't add information because the old Chromium SVN 
importer only used GMT; it does add information in native Git repos. See also 
previous discussions on the Git mailing list for why Git has this in the first 
place:
http://thread.gmane.org/gmane.comp.version-control.git/52

Thanks for the feedback on abbreviations.

Original comment by dborowitz@google.com on 24 Apr 2014 at 6:09

GoogleCodeExporter commented 9 years ago 
@mmoss: Does multi-line select work in other views on ViewVC? Doesn't look like 
it to me:
http://src.chromium.org/viewvc/chrome/trunk/src/content/renderer/render_view_imp
l.cc?revision=265892

But then again if Chromium has been living with it that's again evidence it's 
not all that useful.

Whatever we use for blame I'd really like to use for plain file views. But 
maybe we can do it with a list layout like prettify.js (current 
googlesource.com) uses, and at least share the same syntax highlighter on the 
back end.

Original comment by dborowitz@google.com on 24 Apr 2014 at 6:15

GoogleCodeExporter commented 9 years ago 
@dborowitz: that's interesting, I can't find the raw output anymore. I think it 
used to work before but it must have changed. That's probably a good signal 
that no one cares about the raw output and/or multi-line select: no one in 
chrome has complained.

Original comment by jam@chromium.org on 24 Apr 2014 at 6:24

GoogleCodeExporter commented 9 years ago 
FWIW raw file output is also coming to Gitiles but is incurring a lengthy 
security review :)

Original comment by dborowitz@google.com on 24 Apr 2014 at 6:26

GoogleCodeExporter commented 9 years ago 
I didn't mean that multi-line worked in viewvc, since I rarely use that anymore 
and wouldn't really know. I just meant that it does work in the normal gitiles 
file view (e.g. 
https://chromium.googlesource.com/chromium/src.git/+/master/.DEPS.git), and I 
think that's sufficient, and makes it OK if it doesn't work in the blame view.

Original comment by mmoss@chromium.org on 24 Apr 2014 at 6:41

GoogleCodeExporter commented 9 years ago 
As for the timezones, yes that's useful for git to know about that, but I don't 
think it's useful for the web UI, which should show commits all in the same 
timezone for easier comparison. As for which timezone to use, I'm sure people 
would prefer their local timezone (which might imply client-side rendering), 
but as long as the timestamps are easy to compare, without the additional 
hassle of offsets calculations, it probably doesn't matter all that much, so 
maybe should just use GMT.

Original comment by mmoss@chromium.org on 24 Apr 2014 at 6:47

GoogleCodeExporter commented 9 years ago 
Ah, ok, I don't want to break that, but like I said we can use the same syntax 
highlighter with a <li> instead of a table for the non-blame file view.

Original comment by dborowitz@google.com on 24 Apr 2014 at 6:48

GoogleCodeExporter commented 9 years ago 
I've found non-local timezones (including GMT) to be painful to deal with.  
Using local timezone would be better.  Any chance we can get the client's TZ 
and use that during server-side rendering?

Original comment by cmp@chromium.org on 24 Apr 2014 at 7:13

GoogleCodeExporter commented 9 years ago 
Sure, just ask the Chrome guys to add the user's timezone to the HTTP request 
headers.

Original comment by sop@google.com on 24 Apr 2014 at 7:14

GoogleCodeExporter commented 9 years ago 
Assuming account data was easy to store and pass to gitiles, I was thinking my 
account could have a configurable TZ pref that gitiles can use.

Original comment by cmp@chromium.org on 24 Apr 2014 at 7:18

GoogleCodeExporter commented 9 years ago 
As a first pass, I'd rather just see it consistently in UTC. We can always 
layer localization on that, maybe a server-side pref, or maybe just some 
javascript or Chrome extension that automatically rewrites timestamps based on 
the browser's timezone, but this shouldn't depend it or be delayed by it.

Original comment by mmoss@chromium.org on 24 Apr 2014 at 7:36

GoogleCodeExporter commented 9 years ago 
@mmoss: sounds very reasonable. as a data point, viewvc just displays UTC. I 
think the exact time isn't so important as much as the relative ordering.

I would caution against falling back to chrome extensions to fix up UI. Not 
everyone will have it installed (and even then, not on all chrome profiles). 
Also it's not available on mobile devices or other browsers.

Original comment by jam@chromium.org on 24 Apr 2014 at 7:40

GoogleCodeExporter commented 9 years ago 
People who want UTC, do you want UTC in log and commit views as well?

Original comment by dborowitz@google.com on 24 Apr 2014 at 7:42

GoogleCodeExporter commented 9 years ago 
Yeah, I was just throwing extension out as an option, though if anything, it 
would more likely be a stopgap. Since we can add the handling to the page or 
server, that's probably where it should be (eventually).

Original comment by mmoss@chromium.org on 24 Apr 2014 at 7:47

GoogleCodeExporter commented 9 years ago 
Yeah, consistent TZ would be nice for log view too. Commit view doesn't seem as 
important, since you wouldn't be comparing times. Maybe you'd want it there for 
global consistency, or just because you're reusing template components, but 
definitely not a priority.

Original comment by mmoss@chromium.org on 24 Apr 2014 at 7:58

GoogleCodeExporter commented 9 years ago 
There's been lots of feedback above, so I'll be brief:

Hover versus non-hover: I like the non-hover variant better.  You can omit the 
[Diff] link if you make the date + name link go there, and fix the issue I have 
right now where I can't actually click that link (presumably because of the div 
you mentioned?).  I would avoid abbreviating [Commit] or [Blame].

Timezones: As long as everything is displayed in some consistent timezone, it 
doesn't matter what timezone it is.  UTC is fine.  User's local time is fine.  
Showing things with their original offsets, so you have effectively "3 PM + 8 
hours" for one commit and "4 PM + 0 hours" for another, is bad.

Frames: I agree with John that this should use the full page width.  This will 
be especially important for Blink code which has no line length limit.

Find in page: I agree that it would be better to break multi-line selection in 
the blame view than to break the browser's find-in-page.

Thanks for working on all this stuff.  It's great to see such responsiveness, 
and for things to be making progress.  Much appreciated.

Original comment by pkasting@chromium.org on 24 Apr 2014 at 8:21

GoogleCodeExporter commented 9 years ago 
Not quite ready for review but here's a new layout I hacked up on the plane, 
with server-side syntax highlighting and a table layout.

$ time curl -so /dev/null 
http://localhost:8080/chromium/src/+blame/master/content/renderer/render_view_im
pl.cc
curl -so /dev/null   0.01s user 0.01s system 2% cpu 0.416 total

Full page starts rendering immediately and is finished after maybe a second on 
my MBA.

Original comment by dborowitz@google.com on 28 Apr 2014 at 12:57

Attachments:

GoogleCodeExporter commented 9 years ago 
LGTM with a couple nits. Would it be possible to separate the timestamps from 
the author info, and have all the fields align vertically? It's hard to scan 
them quickly, and thus do quick comparisons, when they run on with the email 
addresses. And can we get rid of the +0000 and just let people assume UTC?

Original comment by mmoss@chromium.org on 28 Apr 2014 at 3:54

GoogleCodeExporter commented 9 years ago 
It's a table, aligning the columns will be no problem :)

For the timezone issue I filed a new bug:
https://code.google.com/p/gitiles/issues/detail?id=48

I don't consider that bug to be blocking this one; if we change the timezone 
format used by absolute timestamps everywhere on the server, blame should pick 
it up automatically.

Original comment by dborowitz@google.com on 28 Apr 2014 at 4:56

GoogleCodeExporter commented 9 years ago 
New screenshot attached.

I'm going to go ahead and submit this so we can get it in a googlesource.com 
release ASAP. Nits on the CSS still welcome, they'll just go in a followup.

Original comment by dborowitz@google.com on 28 Apr 2014 at 6:14

Attachments:

GoogleCodeExporter commented 9 years ago 
Cool, this is a big improvement over today!

My primary suggestion would be to make the commit link be on the hash fragment 
instead of the committer email.  Making the email a link suggests it will be a 
"mailto:" link, whereas making the hash a link suggests clicking it will go to 
that commit.

It might also be the case that this would make more sense ordered as:

pkasting@chomium.org 2014-04-28 12:17:38 8976ab2 [diff] [blame]

...i.e. with the hash moved to the right.  This puts all three links in close 
proximity.  I'm less confident of this suggestion.

Original comment by pkasting@chromium.org on 28 Apr 2014 at 7:19

GoogleCodeExporter commented 9 years ago 
Thanks, agree this looks much better. +1 to Peter's nit re which is the link.

Original comment by jam@chromium.org on 28 Apr 2014 at 7:47

GoogleCodeExporter commented 9 years ago 
I think Peter's onto something with the idea of moving the revision to the 
right of the timestamp but defer to others' opinions.

Original comment by cmp@google.com on 28 Apr 2014 at 7:50

GoogleCodeExporter commented 9 years ago 
FWIW, hard to tell in that screenshot, but the SHA-1 is currently a link as 
well. I will go ahead and unlink the author, and move the SHA-1 to the right. 
This is a minor deviation from the output of "git blame" but makes sense in a 
Gitiles context given the extra links.

Original comment by dborowitz@google.com on 28 Apr 2014 at 7:53

GoogleCodeExporter commented 9 years ago 
This is live in one US datacenter and rolling out to others:
https://us1-mirror-chromium.googlesource.com/chromium/src/+blame/808d10c70ad4a45
3ef1830046ff26e8d3dd68e24/content/renderer/render_view_impl.cc

There are a couple other tweaks in the pipeline 
(https://gerrit-review.googlesource.com/56536) but if it's ok with you guys I'd 
kind of like to close this bug :)

Original comment by dborowitz@google.com on 29 Apr 2014 at 12:31

GoogleCodeExporter commented 9 years ago 
Thanks Dave, all your improvements have fixed the performance issues and the UI 
now is very usable for the scenarios we gave :)

Original comment by jam@chromium.org on 29 Apr 2014 at 12:39

GoogleCodeExporter commented 9 years ago 
Would it be possible to wrap really long lines? For instance, see the lines at 
the end of:
https://chromium.googlesource.com/chromium/src/+blame/36.0.1964.1/DEPS

Not a blocker, and it looks like viewvc doesn't wrap this either 
(http://src.chromium.org/viewvc/chrome/releases/36.0.1964.1/DEPS?annotate=266785
), but would be useful for files like that. Other than that, LGTM.

Original comment by mmoss@chromium.org on 29 Apr 2014 at 4:19

GoogleCodeExporter commented 9 years ago 
@mmoss, wouldn't wrapping a long line at some arbitrary point make the tool 
less useful to see a file's "true contents" ?  Besides ViewVC, what do 'svn 
blame' and 'git blame' do in those cases?

Original comment by cmp@google.com on 29 Apr 2014 at 4:38

GoogleCodeExporter commented 9 years ago 
'git blame' behavior depends on the value of GIT_PAGER. The default doesn't 
wrap, but with no pager, it does wrap (or probably more accurately, the 
terminal wraps). For gitiles, I was thinking it should be like codereview 
side-by-side view, which does wrap and which seems a lot more convenient than 
horizontal scrolling, and it's pretty obvious based on the line-numbering when 
a line is wrapped. Not a big deal either way.

Original comment by mmoss@chromium.org on 29 Apr 2014 at 4:59

GoogleCodeExporter commented 9 years ago 
I'd rather not wrap.  Especially in Blink code, that leads to wrapping a lot, 
and can make reading the code trickier.

Original comment by pkasting@chromium.org on 29 Apr 2014 at 7:32

GoogleCodeExporter commented 9 years ago 
+1 for not wrapping; I would consider a web browser to be more like a pager 
than not.

Original comment by dborowitz@google.com on 29 Apr 2014 at 7:35

GoogleCodeExporter commented 9 years ago 
+1 for not wrapping. "really long lines" is the exception, so no need to 
complicate for such a corner case. As Peter mentioned, Blink code regularly is 
over 100 or 120 chars, so we wouldn't be able to pick a small number which 
would limit its usefulness.

Original comment by jam@chromium.org on 29 Apr 2014 at 7:37

GoogleCodeExporter commented 9 years ago 
Oh, I didn't mean a hard wrap limit, but just letting the browser wrap based on 
the width of the window (table), which I believe is how codereview does it as 
well. But again, not a big deal.

Original comment by mmoss@chromium.org on 29 Apr 2014 at 7:48

GoogleCodeExporter commented 9 years ago 
mmoss closed the corresponding crbug, so I'm closing this.

Original comment by dborowitz@google.com on 30 Apr 2014 at 10:08