gitk highlight feature

I just pushed out some changes to gitk which allow you to use one view to highlight another (see the "Highlight" submenu under the "View" menu), and which allow you to specify arbitrary git-rev-list arguments for a view. The arguments string uses shell quoting conventions.

I had been thinking of having fields in the view editor dialog where you could put in refs that you did and didn't want included, date specifiers, etc., all in separate fields with suitable labels. Now I'm thinking that it's probably just as convenient to put "ORIG_HEAD.." into the git-rev-list arguments field as it is to put "ORIG_HEAD" in the "Don't include commits reachable from this" field. There may be an argument for having fields for "Exclude commits before this date" and "Exclude commits after this date", because those things often have spaces in them (e.g. "2 weeks ago") which would have to be quoted in the git-rev-list arguments field.

Thoughts?

Paul.

Paul Mackerras <paulus@samba.org> writes:

Eh, the string entered by me is quoted by the program, or do I have to quote it myself? I suspect it should not be so bad to code, even if you have to do it with tcl ;-).

Calendar widgets. BTW, "rev-list --since=2.days.ago" would work rather well ;-).

Junio C Hamano writes:

It's split up into arguments at whitespace unless the whitespace is quoted, just like the shell does. I wrote tcl code to do the splitting and quoting according to the shell rules. So you can, for example, put:

	--unpacked --since='2 days ago'

in there and it will pass two arguments to git-rev-list, the second containing the embedded spaces (but without the single-quotes).

Paul.

On Wed, 3 May 2006, Paul Mackerras wrote:

Ok. It looks interesting. I don't have a particular issue to try it with, but the bold-face certainly stands out enough that it was easy to ask for a high-light of any commits that changed the kernel/ subdirectory, and it was visually very obvious.

The interface does feel a bit awkward, though. Separating out "view" and "highlight" into two separate things seems wrong. Wouldn't it be better to just have multiple views in the "view" menu, and just a way to mark one or more of them as "highliht views".

Ie not a separate menu, but having the current "view" radio buttons be more flexible.

Yeah. I think it's easier with a single thing, just let people stick it there.

I alwaus use "2.weeks.ago" instead, but I guess you could do a calendar widget or something.

		Linus

On Tue, 2 May 2006, Linus Torvalds wrote:

The obvious way to do it would be to just have two buttons per view: one exclusive one (for the main view - only one selected at a time), and the other one for the "highlight" one where you could allow multiple views to be selected for highlighting.

		Linus

Linus Torvalds writes:

That's hard to do in a menu, but I could do it in a separate pane. Or, I could draw a series of tabs between the menu bar and the graph/headline/author/date panes. Each tab would have the view name, a radiobutton to select it for highlighting, and a close button.

Paul.

On Wed, 3 May 2006, Paul Mackerras wrote:

Actually, having used this thing (and thought about) some more, I'm afraid that I'll have to just admit that my whole "highlight" thing was misdesigned.

One is just an interface issue, and I think that it would be fixed by just removing the highlight entry from the "view" menu, and making it a menu of its own. That, I think, would fix the user interface to be more obvious and intuitive.

But the real thing I found is that when I decided I wanted to highlight, I didn't actually want to highlight by "git-rev-list" at all. At least not most of the time.

So far, what I've wanted to highlight by is:

 - "does it touch this file/directory/pathspec"

   This is _close_ to "git-rev-list", and you can (and do) actually 
   implement it as that, but it's stupid to do it that way. You just spend 
   extra time. It's literally much better to do

	cat commit-list | git-diff-tree -s --stdin -- <pathspec>

   which is a hell of a lot more efficient, since you already have the 
   commit-list you're interested in (and, in fact, this allows you to do 
   things efficiently only for the current _visible_ commits, if you want 
   to, which might be an important optimization for large views).

 - "Does the author/committer match xyz*"

   I ended up using the "search" button for this, and it worked, but the 
   highlight feature would just have done it much better. Especially if 
   there was a way to do "go to next highlight", instead of just "go to 
   next commit"

   Again, this is actually most easily done by just using the commit-list 
   you already _have_. Not with adding a new argument to "git-rev-list" 
   and trying to filter two views. Also, again, this can actually be done 
   for just the "visible" ones, not the whole view.

 - "highlight the commits [not] reachable from the selected commit"

   Now, this literally is "git-rev-list", but it's a variation on it: you 
   don't want a new view, you really want the _old_ view (exact same 
   git-rev-list arguments), but you add "^commit" to the list of 
   revisions. And then you (conditionally) invert the highlighting if you 
   wanted "reachable" rather than "not reachable".

And the current "create a new view, and overlay that on the old one" approach really isn't that good. My mistake, it was me who suggested it.

What do you think? 

		Linus

Linus Torvalds writes:

On my G5:

$ git-rev-list HEAD >all-revs
$ time git-rev-list HEAD -- arch/powerpc >ppc-revs
real    0m2.463s
user    0m2.265s
sys     0m0.191s
$ time git-diff-tree -s --stdin -- arch/powerpc <all-revs >ppc-revs2
real    0m5.269s
user    0m4.794s
sys     0m0.462s
$ wc ppc-revs*
   892    892  36572 ppc-revs
  5875   5875 240875 ppc-revs2

Why does git-diff-tree -s --stdin produce so many more revisions than git-rev-list? The git-diff-tree output includes commits such as 6ba815de, which only affects arch/i386/kernel/timers/timer_tsc.c. Confused.

I'd have to try it. The overhead of fork/exec/process startup might be too much to do every time the user scrolls the window, though. Maybe what we need is some way to have git-diff-tree run as a helper process where I can write some commits to it, then write something that says "that's all for now" and have it finish what it's doing, write some "end of output" indicator and flush its output buffers - and still have the process and the pipes to/from it available for another round later.

The "/" and "?" keys go to the next and previous find hit. It sounds like the search function is actually what you want for this, since it does highlight the commits that match, but maybe the problem is that the yellow highlighting tends to go away too easily.

How about a way to do a search and use the results to create a bold highlight, which will persist even if you do another search? Either a separate "find & highlight" button, or a way to transfer the current find hits to the highlight?

As for "go to next/previous highlight", that sounds useful. Rusty wants "go to next/previous commit that is a child/parent of this one" and "go to next/previous commit that is a descendent/ancestor of this one and has a ref (tag, head, etc.)".

How about shift-up and shift-down for "go to previous/next highlight"?

Paul.

On Thu, 4 May 2006, Paul Mackerras wrote:

Right. One of them prunes the history (git-rev-list). The other one does not.

That said, you need an "-r", I guess, to "git-diff-tree".

I guess it's officially a FAQ by now: see the "bug in git log" thread the other day, and

        http://www.gelato.unsw.edu.au/archives/git/0604/19180.html

so in general you can get _more_ of the "real commits" with "git-rev-list 
| git-diff-tree --stdin", because it won't prune out history on 
uninteresting branches. At the same time, that effect is counter-acted by 
git-diff-tree normally ignoring merges by default, which is a bigger 
issue.

Your big problem is just the lack of "-r", though:

Without the -r, git-diff-tree won't actually recurse, so it hits the "arch/" part (which does differ, and matches te revspec), and decides it's done.

With the "-r" thing, you still have a noticeable difference, but now it's due to the difference between "log" and "diff":

   git-rev-list HEAD -- arch/powerpc | wc -l
    -> 892
   git-rev-list HEAD | git-diff-tree -r -s --stdin -- arch/powerpc/ | wc -l
    -> 838

ie "diff" doesn't show merges nomally (with -m, you _will_ get merges, but you'll sometimes get them twice - once against each parent ;)

		Linus

Linus Torvalds writes:

I just pushed some changes to the "new" branch of the gitk.git repository which change the way we do highlighting. There is now a row of controls across the middle of the window, just below the row containing the sha1 ID, "Find" button, etc., which controls the highlighting. There are (currently) three ways to do highlighting: by path, by view, and by author/committer. The author/committer matching is case-insensitive (since I'm clearly an insensitive sort of guy :) and matches any of the given strings anywhere in the author and committer fields. The path and author/committer entry widgets take a whitespace-delimited list of paths or names of interest, using shell quoting rules, so you can put for example:

"david s. miller" benh

in there and you'll get commits from either davem or benh highlighted.

Do people think this is useful and on the right track interface-wise?

Paul.

On Sat, 20 May 2006, Paul Mackerras wrote:

Ok, this is much nicer, I think this ends up being much closer to what I really wanted, even if I didn't know quite how I wanted it.

I think the "Find" field should highlight things too. Right now there's no way to get highlighting for somebody having signed-off on a patch, for example, even though you can _search_ for it.

Also, right now it says "Author/committer", but it actually only triggers on author. Which may be the right thing to do (it's certainly what I'd normally want to see), but it doesn't match the description.

		Linus

Linus Torvalds writes:

Yes, I think the way to go is to extend the highlight feature to be able to do everything that the "Find" function can do, and then I think the "Find" function won't be needed any more.

If the author matches, it highlights both the headline and the author. If the committer matches, it highlights only the headline (as it does for any other highlighting). Try it with "torvalds" in the author/committer field on commit a54c9d30 (compared to cb46c370, say). If that's confusing I can change it of course.

Paul.