Comments (95)
I thought about this a bit over the holiday and got a bit frustrated by the
fact that JGit has no way to cache common blame info between successive
revisions of the same blob, let a lone across multiple requests. Obviously all
blame information for deadbeef^:foo is identical to deadbeef:foo except for
what was actually modified by foo. Caching one revision's worth of blame is an
obvious optimization, but someone clicking back through the blame history one
revision at a time would be recomputing huge chunks of identical blame data.
Fixing this, I think, would require substantial changes to JGit's blame
internals.
Original comment by [email protected]
on 26 Dec 2012 at 7:37
from gitiles.
Chromium wants this sooner rather than later. Incremental caching and AJAX
support may not be blockers.
Original comment by [email protected]
on 23 Jan 2014 at 9:50
from gitiles.
For our timeline, it would be wonderful to have the first pass on this around
Valentine's day. We just need something to show to the other Chrome devs so we
can focus on other aspects of the git migration. Is that at all feasible? If
not, is there a different date you can give us that will let us put our minds
and schedules at ease?
Original comment by [email protected]
on 27 Jan 2014 at 7:29
from gitiles.
Got a rough version of this working today, but found a bug in upstream JGit's
blame implementation that may make the results...unsatisfying:
$ ~/c/jgit/org.eclipse.jgit.pgm/target/jgit blame -L 25,35
61dcc102242e00d3b0061405d5c909564ebbd819
gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
d2fa1fdb (Shawn O. Pearce 2012-06-05 08:49:52 -0700 25) import static
org.eclipse.jgit.transport.ReceiveCommand.Result.REJECTED_OTHER_REASON;
06cb1d25 (Dave Borowitz 2012-02-29 11:39:00 -0800 26)
c545c090 (Shawn O. Pearce 2012-07-27 16:38:55 -0700 27) import
com.google.common.base.Function;
e6298f72 (Shawn O. Pearce 2012-07-26 12:36:55 -0700 28) import
com.google.common.base.Predicate;
( 29) import com.google.common.base.Splitter;
0795c58a (Shawn Pearce 2013-02-24 15:13:27 -0800 30) import
com.google.common.base.Strings;
( 31) import com.google.common.collect.ArrayListMultimap;
( 32) import com.google.common.collect.BiMap;
( 33) import com.google.common.collect.HashBiMap;
5972e244 (Bruce Zu 2013-05-28 11:12:16 +0800 34) import
com.google.common.collect.HashMultimap;
Original comment by [email protected]
on 29 Jan 2014 at 1:32
from gitiles.
Submitted:
https://gerrit-review.googlesource.com/#/q/project:gitiles+topic:blame,n,z
JGit bug still exists so results may not be totally accurate, but you can get
some idea of the UI. Should be live on *.googlesource.com later this week.
Original comment by [email protected]
on 30 Jan 2014 at 12:50
from gitiles.
This might be the upstream JGit bug:
https://bugs.eclipse.org/bugs/show_bug.cgi?id=374382
Doesn't look like anybody has attempted to fix it, but at least there's a
better description than "blame doesn't work sometimes."
Original comment by [email protected]
on 3 Feb 2014 at 11:49
from gitiles.
Original comment by [email protected]
on 5 Feb 2014 at 6:33
from gitiles.
This is awesome. I notice that the left column links on a blame page go to a
blame from that revision. Is this on purpose? It might be more intuitive for
the most prominent link to go to the actual revision, rather than the blame.
For me, it's pretty rare that I'd want to redo the blame at the revision
identified in the original blame. Frequently I will need to do a blame on the
parent of that revision though, if the identified commit was just a refactor or
rename.
Original comment by [email protected]
on 7 Feb 2014 at 10:44
from gitiles.
ilevy:
https://chromium.googlesource.com/chromium/src/+blame/e3a928b81366df875af873614f
0703f7733f0050/DEPS shows an example of blame links that take us to blame for
those revisions on this file. To get to the corresponding revision data, I
simply scroll to the top of the page and then click the revision.
If each line took me to the basic revision info, reblaming on that revision in
the same file would require going through the tree of files to find the file
again, and then blame from there (painful). Or changing the URL by hand (no
good).
Comparing the number of clicks between the cases, the greater cost would be on
those who are actively investigating blame data in a file going back previous
revisions. Also, the user has signaled their interest in blame data for that
file at that revision by finding that file in the web interface already.
Forcing people to do that over and over again as they go further and further
back seems bad.
Original comment by [email protected]
on 7 Feb 2014 at 10:54
from gitiles.
The decision to link to /+blame was somewhat arbitrary. I was on the fence
between going to the ref (with the downsides cmp mentioned), the file contents,
or the blame.
Original comment by [email protected]
on 7 Feb 2014 at 11:13
from gitiles.
On a chromium-dev thread, szager suggested I post this here (see
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/XUHR_iq5m1I/oq4iaji0
NI4J). Perhaps it's the same as to cmp's comment (I'm not sure). Also, the
performance was slow (nearly 2 minutes to blame this file).
If needed, I'm happy to sit with anyone and show them the workflow that I use
to track down code.
Just saw this update, nice to see that work is going on this tool. To try it
out, I picked a recent example that git users couldn't track down and which we
were investigating because of a bug. Let's say I want to look at
RenderViewImpl::OnMessageReceived, and the
"GetContentClient()->SetActiveURL(main_frame->document().url());" line near the
top and see the cl which added it. Currently using ViewVC my steps are:
1) go to src.chromium.org/viewvc/, find the file
2) 2s later
http://src.chromium.org/viewvc/chrome/trunk/src/content/renderer/render_view_imp
l.cc?revision=254479 loads, click on "show annotations"
3) 20s later the page loads, I see that line with a link
http://src.chromium.org/viewvc/chrome/trunk/src/content/renderer/render_view_imp
l.cc?r1=163060&r2=163061&. It's a cl from me moving code into a namespace.
However the link is helpful in that it has a side-by-side diff and the left
side has a link to the previous revision before this change, so I click on that
link.
4) I go through the above line 4 more times because that line changed by a
bunch of people. The total network load time is 35s. There's a bug in viewvc
when the change is before a file rename, since it tries to generate a url with
the old filename and that fails. However that bug is easy to workaround by
correcting the filename in the url, which I know since it's what I started
with. I finally get to the original cl that added this:
http://src.chromium.org/viewvc/chrome?revision=11337&view=revision
This process was more convoluted than normal, because that line had 5-6 changes
to it. However even then, it takes me about a minute or two to do this tracking.
I tried to do this with the git tool.
1) I click "blame" on
https://chromium.googlesource.com/chromium/src/+/master/content/renderer/render_
view_impl.cc
2) 108s later, I look for SetActiveURL and see that first change from me. It
has a link on the left to
https://chromium.googlesource.com/chromium/src/+blame/a80af12efcba198f4beda21c1d
88c63f04c4dc41/content/renderer/render_view_impl.cc
3) 60s later, it takes me to
https://chromium.googlesource.com/chromium/src/+blame/a80af12efcba198f4beda21c1d
88c63f04c4dc41/content/renderer/render_view_impl.cc, i.e. the same link where I
came from. At this point, I can't do anything.
Original comment by [email protected]
on 4 Mar 2014 at 12:20
from gitiles.
jam@ also wrote at the end of his message on chromium-dev:
> So it seems that the tool needs more work to find changes as it
> could only go one step backwards. Having it match viewvc to go
> back multiple changes is needed as this information is often
> crucial in figuring out why a piece of code is the way it is
> when refactoring.
>
> Hopefully these steps are useful to the team working on this
> tool in figuring out how to get this sort of investigation to
> work.
Original comment by [email protected]
on 4 Mar 2014 at 3:54
from gitiles.
pkasting@ wrote:
There are a couple problems here:
(1) Because git hashes aren't sequential in nature and the view here displays a
"rounded" last modified time, we have displays like:
12507e3 [email protected] - 1 year, 4 months ago ...
1f7d4b9 [email protected] - 1 year, 4 months ago ...
It's not possible to glance at this and tell which change came last. The
easiest fix I can think for this would be to change the "1 year, 4 months ago"
string to a timestamp:
12507e3 [email protected] - 12 Dec 2012 18:30 PST ...
1f7d4b9 [email protected] - 19 Dec 2012 4:27 PST ...
(2) Clicking on one of these blame lines sends you to an annotated version of
the file as of that commit. ViewVC's behavior, which is vastly more useful,
takes you to the diff from the selected commit against the previous file
version (from which you can get to the full file as of either version). With
this UI, I don't see a trivial mechanism to compute that diff.
To me both of these would be seriously detrimental to my workflow. It would be
nice to address both before the switch.
Original comment by [email protected]
on 4 Mar 2014 at 3:56
from gitiles.
Re: pkasting's (2), there's some discussion here about whether the link should
go to:
- the blame for the file at that revision (my comment)
- the diff for the file at that revision (pkasting's comment)
- the content of the file at that revision (ilevy's comment)
All I know is that the current UI doesn't make it easy to jump between these
modes from each other. It would be nice to have links at the top of each
modes' page that allowed getting to the other modes easily.
For example: on
https://chromium.googlesource.com/chromium/src/+blame/master/OWNERS, show:
[link]file history[/link] [link]raw[/link] [link]diff[/link] blame
And on https://chromium.googlesource.com/chromium/src/+/master/OWNERS, show:
[link]file history[/link] raw [link]diff[/link] [link]blame[/link]
etc
Dave, is that doable?
Original comment by [email protected]
on 4 Mar 2014 at 4:02
from gitiles.
Thanks all for the concrete suggestions. I'll take a look at some other
tools, rethink the top bar, and get back to you.
Original comment by [email protected]
on 4 Mar 2014 at 4:04
from gitiles.
Here's one more issue I found, from email:
"When trying to use gitiles' blame on the internal, pre-launch Chromium source
tree, I found that the line height in the "blame" column is not identical to
the line height in the "source" column, so that as you scroll down, the blame
lines rapidly get out-of-sync with the source lines, and at the bottom of the
page, you can see a discontinuity in the horizontal line that appears under the
two columns.
"This makes blame pretty much useless, because it's not clear where you should
click to actually see the blame for a particular line of code."
Also, regarding comment 14's three options:
* I don't see how "content of the file at that revision" is more useful than
"blame of the file at that revision". Showing the blame annotations is
strictly more info, so it seems strictly more likely to be useful. That
suggests not doing option 3.
* That said, I'm not sure what use cases are addressed by showing the blame of
the file at the chosen revision. If you show the diff as I suggested, you can
immediately distinguish between "yep, this is the change I was looking for" and
"no, this just reordered comments/fixed a typo/fixed line endings/etc., I need
to keep looking". If you show the blame, you have to click through to the diff
right away to determine this. So it seems to me like showing the diff is much
better. Certainly while I've been doing hours of version control spelunking
over the past few days (trying to assign owners for all 1000 Chrome
command-line flags), I've wanted the diff every time. But maybe I'm missing
some use case.
Original comment by [email protected]
on 5 Mar 2014 at 11:26
from gitiles.
Peter, re: your comment, the current gitiles view doesn't have links at the top
of the page to the other views of the file (so one can't easily go from blame
to diff, diff to content, content to blame, etc). I believe that will change
and those links will get added, then more options open up.
Going on in your comment, you say the goal is to determine if this is revision
that the user is looking for. Once the UI makes it easy to jump between
content/diff/blame for a file, I believe using "diff" for the default view
should be the best choice.
For the sake of the bug and the record, I will document the essential steps we
want to support as I'm aware of them:
1. find file in repo
2. blame on file
3. find line you want to know about and revision where line was changed
4. click on revision
5. see diff, this revision is not the right revision
6. go to blame view of this file at revision's parent
7. repeat from step 3 until step 5 finds a revision that is the right revision
Re: step 6: This feature is currently missing. When viewing a file in
content/diff/blame view, there should be a link to a previous revision for that
file. I know that we can edit the URL to do this, but we need the page to
contain the link to consider this feature presented. Expanding on comment 12,
jam@ had commented that this feature was missing when he tried to use this tool.
Original comment by [email protected]
on 12 Mar 2014 at 3:14
from gitiles.
For the record, the steps Chase outlines in c#17 are exactly what the gerrit
and Chrome Infra teams agreed was the right way to proceed in a weekly meeting
yesterday.
Original comment by [email protected]
on 12 Mar 2014 at 3:40
from gitiles.
FYI the bug in upstream JGit should soon be fixed:
https://git.eclipse.org/r/24916
Original comment by [email protected]
on 13 Apr 2014 at 7:38
from gitiles.
https://gerrit-review.googlesource.com/56202 adds a bunch of links to the blame
page.
I think it's pretty ugly, someone with a better eye for CSS feel free to take a
look ;)
Original comment by [email protected]
on 21 Apr 2014 at 10:43
from gitiles.
Added link to the parent blame view when viewing a single file diff:
https://gerrit-review.googlesource.com/56229
Original comment by [email protected]
on 22 Apr 2014 at 12:25
from gitiles.
Great to see the improvements landing, thank you for incorporating them. I
think these aren't pushed yet to the server on chromium.googlesource.com right?
Please let me know when they are so I can test it, or if there's an internal
canary server that works too.
Original comment by [email protected]
on 22 Apr 2014 at 2:55
from gitiles.
Not live on gogolesource.com, I wanted to get a round or two of feedback before
going through that push process. Take a look at this change series, which you
can fetch and run your own test server:
https://gerrit-review.googlesource.com/56229
Or, any Googlers, ping me on IM and I'll give you an internal test server URL.
Original comment by [email protected]
on 22 Apr 2014 at 4:29
from gitiles.
I tried out the link you sent me over IM. I may be missing something, but I
don't see how it's possible (and easy) to see previous blames of a piece of
code. Please see my description in comment 11 and let me know how to do the
equivalent.
Original comment by [email protected]
on 22 Apr 2014 at 4:45
from gitiles.
Did you hover over the revision and not get the "[commit] [diff] [blame]" popup?
Original comment by [email protected]
on 22 Apr 2014 at 4:54
from gitiles.
@mmoss: yep, I had. I clicked on "commit", and that takes me to the parent
commit. Then I see a "parent 1b12cf346932031c6da0a9114b100824f466e80a[diff]". I
clicked on "1b12cf346932031c6da0a9114b100824f466e80a", but then I have to find
the path of the file I was interested in again (by clicking "content", then
"renderer", then "render_view_impl.cc"). Note that this step doesn't have to be
done with viewvc, i.e. i can blame the parent revision directly. Anyways, after
I found the file, I clicked annotate and it showed a revision from 3 years ago.
i.e. it appears to have skipped a bunch of changes to that one line.
Please see my example in comment 5, and reproduce the steps but on gitiles to
show the starting cl.
Original comment by [email protected]
on 22 Apr 2014 at 5:07
from gitiles.
@jam: If you just want to step through previous blames, click "[blame]" instead
of commit.
If you want to do blame -> diff -> blame -> diff, from the first blame page,
click "[diff]" -> "[blame]" (next to the parent link) -> "[diff]" etc. That was
my understanding of the process you outlined in #11.
Original comment by [email protected]
on 22 Apr 2014 at 5:40
from gitiles.
By "blame -> diff -> blame -> diff" I mean "blame of C -> diff of C~1..C ->
blame of C~1 -> diff of C~2..C~1 -> ..."
Original comment by [email protected]
on 22 Apr 2014 at 5:43
from gitiles.
Yeah, I think the "blame -> diff (in the left-side hover popup) -> parent blame
-> diff -> parent blame ..." looks very similar to your viewvc workflow (it
actually avoids the extra file view at the end of your step #3, before the next
annotate). I don't think you want the "commit" link at all for that, which
would be like clicking the "Revision #" link at the top viewvc annotation page,
which you don't do.
Original comment by [email protected]
on 22 Apr 2014 at 6:00
from gitiles.
@dborowitz @mmoss: can you try out my specific example from comment 5? Clicking
"blame" or "commit" takes me to a different cl. I'm trying to figure out what
originally added the "
GetContentClient()->SetActiveURL(main_frame->document().url());" line, even as
the method call changes namespaces or the way to get the url changes.
Original comment by [email protected]
on 22 Apr 2014 at 6:09
from gitiles.
Ok, I'm going to run through your example in #11.
1. /chromium/chromium/+blame/trunk/content/renderer/render_view_impl.
Starting at l.1071:
GetContentClient()->SetActiveURL(main_frame->document().url());
2. Hover over 63135f4, click "[diff]"
=>
/chromium/chromium/+/63135f455920bed0b9b9a4758df5ce067253d4b4%5E%21/content/rend
erer/render_view_impl.cc
3. Read the commit message; see this was a move.
4. Click "[blame]" just to the right of
"parent f2e92bd3d3d580925574bf11c9374098dda88bb6 [diff]"
=>
/chromium/chromium/+blame/f2e92bd3d3d580925574bf11c9374098dda88bb6/content/rende
rer/render_view_impl.cc
5. Find that line again, now l. 947, click "[diff]"
=>
/chromium/chromium/+/a2470189f253b3bd17c37d2765b6f00f47ae4233%5E%21/content/rend
erer/render_view.cc
6. Read commit message, also not what you want (I think), click the parent
blame link again.
7. Repeat (5) and (6) till you find the commit you want.
Did I misunderstand your example?
Original comment by [email protected]
on 22 Apr 2014 at 6:19
from gitiles.
Aside:
I've also been toying with the idea of including either a one-line summary or
the full commit message in the hover popup. This might make you able to skip
the "[diff]" link in some cases, if you can tell from the commit message that
it's not the commit you're looking for.
The reason I haven't done this yet is doing it naively (i.e. server side) might
seriously bloat the page size, so it would be time to think about loading it
dynamically with JS, etc.
Original comment by [email protected]
on 22 Apr 2014 at 6:37
from gitiles.
@dborowitz: did you eventually get to
http://src.chromium.org/viewvc/chrome?revision=11337&view=revision?
I'm starting at:
http://serval.mtv.corp.google.com:8080/chromium/chromium/+blame/trunk/content/re
nderer/render_view_impl.cc
-searching for
"GetContentClient()->SetActiveURL(main_frame->document().url());" shows a link
to 3d9c9d7 (not sure where you got 63135f4?). I click [diff] over the 3d9c9d7
link. Then I click blame to the right of the parent link which takes me to
http://serval.mtv.corp.google.com:8080/chromium/chromium/+blame/3d9c9d736f955cea
a1a1076cba10859875fd667f%5E/content/renderer/render_view_impl.cc
-when I search for SetActiveURL in the new page, it shows "a705fea
[email protected] - 3 years, 7 months ago" which is not the second-last
change to that line.
I think I'm not being clear in my request. Let's try this a different way: can
you try the exact steps I outlined in step 5 using viewvc (with the links that
I supplied), and see the code changes to that specific function call
(SetActiveURL). Then show me how using gitiles goes through the exact same
changes to that one line.
Original comment by [email protected]
on 22 Apr 2014 at 7:57
from gitiles.
Can you send a screenshot of what you see on that first page? I'm seeing
63135f4, as attached. There are references to 3d9c9d7, but for me, the closest
one is about 40 lines above.
Original comment by [email protected]
on 22 Apr 2014 at 8:10
Attachments:
from gitiles.
And, yes, I can eventually get to svn r11337 (it was like 5 or 6 parents up):
http://serval.mtv.corp.google.com:8080/chromium/chromium/+/638e5b1b75e252a81daa9
5b9a3c6502d6397f4e0%5E%21/chrome/renderer/render_view.cc
Original comment by [email protected]
on 22 Apr 2014 at 8:22
from gitiles.
I think I got the right answer with gitiles. I'll check with John when he gets
back.
Original comment by [email protected]
on 22 Apr 2014 at 8:24
from gitiles.
ha, Brett figured out the problem. It looks like a font issue. On Linux this
works fine, but on Windows the font spacing is different. I'm attaching a
screenshot to show you how it looks like on my machine. I would try out on
Linux but I'm having hardware issues there.
Original comment by [email protected]
on 22 Apr 2014 at 8:58
Attachments:
from gitiles.
It seems the current layout loads the left column and then the right column and
expects them to line up. I think there are two problems with this:
1) There are too many things that can cause variable line spacing, such that
the two sides don'e line up properly. This is what John was seeing.
2) The intermediate load state is weird. The page loads slowly, and often
you'll be staring at only the annotations (left column) with a blank right
column. I was pretty confused about this the first time, I thought the file was
empty or the page was broken. Subsequent times it still tripped me up briefly
before I remembered what was going on and that I needed to wait longer.
I was watching John use it, and this weird intermediate state also tripped him
up slightly.
I think the layout should be by row instead of by column, which would eliminate
both of these issues.
Original comment by [email protected]
on 22 Apr 2014 at 9:03
from gitiles.
Ok, I'm going to take that as a vote for the current changes wrt to links.
Getting the fonts right cross-browser is going to be a nightmare, especially
since I have no access to Windows, but I already knew that :(
If anybody has pointers to docs/people about how fonts and line height work in
<pre> blocks, please send them my way.
But really we need to convert to CodeMirror.
Original comment by [email protected]
on 22 Apr 2014 at 9:04
from gitiles.
And don't take my last comment as a sign of despair. CodeMirror conversion
should be pretty easy.
Original comment by [email protected]
on 22 Apr 2014 at 9:10
from gitiles.
My opinion is that any design where you count on different blocks ending up
with exactly the same line spacing isn't going to work very well, especially
when you have stuff like italic floating around (which is normally a completely
different font file or might be emulated, depending on platform).
Original comment by [email protected]
on 22 Apr 2014 at 10:07
from gitiles.
I agree. Didn't mean it wasn't the easiest thing to get working in a pinch :)
CodeMirror will solve this problem. It allows annotating specific line numbers
with arbitrary DOM nodes.
Original comment by [email protected]
on 22 Apr 2014 at 10:12
from gitiles.
I haven't seen the new UI, but the description of "hover popup" above worries
me.
Hover-based UIs are problematic for many reasons, including primarily that
(1) accessibility suffers for users who can't hover a mouse over a visible
target (blind users, people who can't use mice)
(2) hover is not generally a good signal of user intent/attention (the majority
of the time, the user doesn't care or necessarily even know where the cursor is)
Accordingly, we try hard to avoid them both in Chrome and in the Google qeb
properties I'm aware of.
Consider instead other options, like a custom context menu or using additional
links (e.g. "abc@123 (b c)" where "abc@123" is a link to the diff and "b" and
"c" are links to the blame and commit).
Incidentally, base on the screenshot in comment 37, it looks like comment 13
point (1) has still not been addressed. This is fairly important when trying
to trace down changes older than a few weeks, and hopefully wouldn't be
ridiculously hard to implement.
Original comment by [email protected]
on 24 Apr 2014 at 12:26
from gitiles.
Thanks for reminding me about #13(1), I was planning on changing to absolute
dates anyway.
As for hover menus, I'm aware of the general arguments against hover, but there
are a few specific reasons why I went with them _for this application_:
1. There are already potentially dozens of links (one per line) visible on the
page; tripling the number of links just seems really busy.
2. Horizontal space is valuable here: every pixel we add to the gutter is a
pixel taken away from the source code.
3. The only link targets on this page are the blame lines, so a user indicates
intent by moving the cursor over those, at which point they will discover the
hover menu. (Also, I'm not sure how your suggestion of a custom context menu
addresses discoverability?)
4. The links in the hover menu are just for convenience and don't strictly add
functionality; their targets are reachable from the default non-hover link).
That said I'm not set on these, I would just like the discussion to focus on
this specific application rather than general objections. And it's easy enough
in the CSS for me to prepare some A/B demos for you guys.
(Also: "we try hard to avoid them both in Chrome and in the Google [w]eb
properties I'm aware of"...has somebody told the Gmail team? ;)
Original comment by [email protected]
on 24 Apr 2014 at 1:28
from gitiles.
Side-by-side (rough & buggy) comparison of hover/non-hover links:
http://serval.mtv.corp.google.com:8081/chromium/src/+blame/master/content/render
er/render_view_impl.cc
http://serval.mtv.corp.google.com:8082/chromium/src/+blame/master/content/render
er/render_view_impl.cc
Switched to absolute dates, could use a narrower format though.
Original comment by [email protected]
on 24 Apr 2014 at 3:51
from gitiles.
I like the non-hover links better.
The name and date should not be blue.
Original comment by [email protected]
on 24 Apr 2014 at 4:06
from gitiles.
The name and date are blue because they're a link. One of the CSS bugs is that
that link is covered by a transparent div.
I guess I can also kill [commit].
Would [b][d] be any better, or too short (for both understandability and
clickability)?
Original comment by [email protected]
on 24 Apr 2014 at 4:09
from gitiles.
Yeah, the date can be a lot shorter, and since it's mostly wanted for
chronological ordering, maybe something ISO 8601-ish, like "2013-01-01
18:00:00", which is fixed width and easy to compare numerically. As for the
links, you could maybe shorten them to something like "com | dif | blm" without
making it too confusing.
Original comment by [email protected]
on 24 Apr 2014 at 4:15
from gitiles.
+1 to removing hover: it doesn't work on touch devices and isn't discoverable.
I'm not too concerned about the extra width, since developers are generally on
a wide display. the two demo links above are a bit misleading because they
didn't take up the full width of the page.
i assume these changes will fix the font issue on windows?
thanks!
Original comment by [email protected]
on 24 Apr 2014 at 4:31
from gitiles.
big problem I saw now: why doesn't chrome's find-in-page work? the page
overrides ctrl+f with a dialog that technically works, but doesn't support
things like find as you type, and has an extra step (press enter). once i
search through that dialog, chrome's find works. but if i try chrome's find
first, it doesn't.
Original comment by [email protected]
on 24 Apr 2014 at 4:51
from gitiles.
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 [email protected]
on 24 Apr 2014 at 5:04
from gitiles.
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 [email protected]
on 24 Apr 2014 at 5:11
from gitiles.
Maybe Chrome's JavaScript engine is too slow and the problem is with V8? :-)
Original comment by [email protected]
on 24 Apr 2014 at 5:14
from gitiles.
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 [email protected]
on 24 Apr 2014 at 5:20
from gitiles.
@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 [email protected]
on 24 Apr 2014 at 5:25
from gitiles.
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 [email protected]
on 24 Apr 2014 at 5:30
from gitiles.
@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 [email protected]
on 24 Apr 2014 at 5:35
from gitiles.
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 [email protected]
on 24 Apr 2014 at 5:40
from gitiles.
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 [email protected]
on 24 Apr 2014 at 5:45
from gitiles.
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 [email protected]
on 24 Apr 2014 at 5:45
from gitiles.
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 [email protected]
on 24 Apr 2014 at 5:50
from gitiles.
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 [email protected]
on 24 Apr 2014 at 6:09
from gitiles.
@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 [email protected]
on 24 Apr 2014 at 6:15
from gitiles.
@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 [email protected]
on 24 Apr 2014 at 6:24
from gitiles.
FWIW raw file output is also coming to Gitiles but is incurring a lengthy
security review :)
Original comment by [email protected]
on 24 Apr 2014 at 6:26
from gitiles.
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 [email protected]
on 24 Apr 2014 at 6:41
from gitiles.
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 [email protected]
on 24 Apr 2014 at 6:47
from gitiles.
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 [email protected]
on 24 Apr 2014 at 6:48
from gitiles.
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 [email protected]
on 24 Apr 2014 at 7:13
from gitiles.
Sure, just ask the Chrome guys to add the user's timezone to the HTTP request
headers.
Original comment by [email protected]
on 24 Apr 2014 at 7:14
from gitiles.
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 [email protected]
on 24 Apr 2014 at 7:18
from gitiles.
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 [email protected]
on 24 Apr 2014 at 7:36
from gitiles.
@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 [email protected]
on 24 Apr 2014 at 7:40
from gitiles.
People who want UTC, do you want UTC in log and commit views as well?
Original comment by [email protected]
on 24 Apr 2014 at 7:42
from gitiles.
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 [email protected]
on 24 Apr 2014 at 7:47
from gitiles.
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 [email protected]
on 24 Apr 2014 at 7:58
from gitiles.
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 [email protected]
on 24 Apr 2014 at 8:21
from gitiles.
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 [email protected]
on 28 Apr 2014 at 12:57
Attachments:
- [Screen Shot 2014-04-27 at 6.51.54 PM.png](https://storage.googleapis.com/google-code-attachments/gitiles/issue-5/comment-78/Screen Shot 2014-04-27 at 6.51.54 PM.png)
from gitiles.
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 [email protected]
on 28 Apr 2014 at 3:54
from gitiles.
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 [email protected]
on 28 Apr 2014 at 4:56
from gitiles.
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 [email protected]
on 28 Apr 2014 at 6:14
Attachments:
- [Screen Shot 2014-04-28 at 11.13.19 AM.png](https://storage.googleapis.com/google-code-attachments/gitiles/issue-5/comment-81/Screen Shot 2014-04-28 at 11.13.19 AM.png)
from gitiles.
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:
[email protected] 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 [email protected]
on 28 Apr 2014 at 7:19
from gitiles.
Thanks, agree this looks much better. +1 to Peter's nit re which is the link.
Original comment by [email protected]
on 28 Apr 2014 at 7:47
from gitiles.
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 [email protected]
on 28 Apr 2014 at 7:50
from gitiles.
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 [email protected]
on 28 Apr 2014 at 7:53
from gitiles.
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 [email protected]
on 29 Apr 2014 at 12:31
from gitiles.
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 [email protected]
on 29 Apr 2014 at 12:39
from gitiles.
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 [email protected]
on 29 Apr 2014 at 4:19
from gitiles.
@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 [email protected]
on 29 Apr 2014 at 4:38
from gitiles.
'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 [email protected]
on 29 Apr 2014 at 4:59
from gitiles.
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 [email protected]
on 29 Apr 2014 at 7:32
from gitiles.
+1 for not wrapping; I would consider a web browser to be more like a pager
than not.
Original comment by [email protected]
on 29 Apr 2014 at 7:35
from gitiles.
+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 [email protected]
on 29 Apr 2014 at 7:37
from gitiles.
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 [email protected]
on 29 Apr 2014 at 7:48
from gitiles.
mmoss closed the corresponding crbug, so I'm closing this.
Original comment by [email protected]
on 30 Apr 2014 at 10:08
- Changed state: Fixed
from gitiles.
Related Issues (20)
- Markdown multilevel lists aren't recognized correctly
- Gitiles needs a git search feature.
- Gerrit is crashing trying to render a markdown page (I think) HOT 5
- [deleted issue]
- Double dash "--" is incorrectly changed to a hyphen &ndash HOT 4
- Would be great if gitiles could render .rst as well as .md documents
- Markdown: conflict between multilevel lists and automatic code block formatting
- Add support for <a name=></a> tags for cross referencing HOT 2
- Allow simple search terms on source pages HOT 2
- rendering of relative URLs in README.md for directory listing page is wrong HOT 5
- Feature request: view for plain version of the file HOT 1
- Feature request: Paginate the list of repositories
- Markdown should support symlinks
- [deleted issue]
- Relative links in markdown result in broken links
- [deleted issue]
- Document support for Markdown definition lists
- Bad JSON output HOT 2
- StringIndexOutOfBoundsException: String index out of range HOT 4
- Gerrit tarballs for the base packages aren't deterministic HOT 1
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from gitiles.