Git development
 help / color / mirror / Atom feed
* Re: [RFC PATCH 10/10] gitweb: Show appropriate "Generating..." page when regenerating cache (WIP)
From: Jakub Narebski @ 2010-01-26  1:49 UTC (permalink / raw)
  To: J.H.; +Cc: Petr Baudis, git, John 'Warthog9' Hawley
In-Reply-To: <4B5DFFE5.6060908@eaglescrag.net>

On Mon, 25 Jan 2010, J.H. wrote:
> On 01/25/2010 05:56 AM, Petr Baudis wrote:
>> On Mon, Jan 25, 2010 at 02:48:26PM +0100, Jakub Narebski wrote:

>>> Now those patches (mine and J.H. both) make gitweb use locking
>>> (it is IIRC configurable in J.H. patch) to make only one process
>>> generate the page if it is missing from cache, or is stale.  Now
>>> if it is missing, we have to wait until it is generated in full
>>> before being able to show it to client.  While it is possible to
>>> "tee" output (using PerlIO::tee, or Capture::Tiny, or tie like
>>> CGI::Cache) writing it simultaneously to browser and to cache for 
>>> the process that is generating data, it is as far as I understand
>>> it impossible for processes which are waiting for data.  Therefore
>>> the need for "Generating..." page, so the user does not think that
>>> web server hung or something, and is not generating output.
>> 
>> Ah, ok, so the message is there to cover up for a technical problem. ;-)
>> I didn't quite realize. Then, it would be great to tweak the mechanisms
>> so that the user does not really have to wait.
> 
> No, that is an incorrect assumption on how the 'Generating...' page
> works, and your missing a bit of the point.
> 
> (1) The message itself 'Generating...' is a cue to the user that
> something is happening and that the browser is not actually hanging.
> Web users are at the point where if things are not instantaneous and
> show immediately they will either browse away completely or hit the
> refresh button incessantly until content does appear.  While the page is
> usually only seen for about a second, and I'll admit it can be annoying,
> it's nothing more than a 'sit tight a second'.  For things like the
> front page it can take upwards of 7 seconds to generate for a single
> user, a lot to ask for a no response scenario.

Pasky is asking later whether instead of separate "Generating..." page
(be it server side using trick with http-equiv refresh taking place when
page/connection is closed, or JavaScript Ajax-y one) couldn't gitweb
use streaming and have early parts of generated page (in particular the
page header and navigation bar) serve as activity indicator.  I think
it is possible, if a bit more complicated, and such feature can be 
introduced in a separate commit.

As to 'sit tight for a second': the code can be modified to print/show 
"Generating..." page only if we are waiting more than second (or more
than half of second with utime from Time::HiRes) for the data.  Pasky,
would such change address some of your concerns about "Generating..."
page as activity indicator?

> 
> (2) It prevents the stampeding herd problem, which was very vehemently
> discussed 4 years ago by HPA and myself and roughly boils down to this:
> 
> When a single user comes into the site, in particular the front page, it
> kicks off a process that will start to generate at it, causing a huge
> amount of git requests into individual repositories and a lot of disk
> i/o.  A second user will then come in and the same requests will start
> to be done from the beginning again, and so on until you basically kill
> the machine because the disk i/o goes up enough that it can't ever
> service the requests fast enough.

As you can see from the way my proof of concept (of splitting) caching
series is split, I think that those two features are separate.  Using
locking to have one writer, and protect against cache miss stampede 
problem is one issue, and having "Generating..." page is another issue.
They are not orthogonal as they are now, as "Generating..." page depends
on locking to check if page got generated, but it could simply check
whether the cache file is present already.

> 
> This does 2 things in the end:
> 
> 1) means there's only 1 copy of the page ever being generated, thus
> meaning there isn't extraneous and dangerous disk i/o going on on the system
> 
> 2) prevents a user from reporting to the website that it's broken by
> giving them a visual cue that things aren't broken.

1) is about locking ('expires_variance' can help there, too), 2) is about
"Generating..." but could be done with streaming: "tee"/"tail" solution.

>> So, I wonder about two things:
>> 
>> (i) How often does it happen that two requests for the same page are
>> received? Has anyone measured it? Or is at least able to make
>> a minimally educated guess? IOW, isn't this premature optimization?
> 
> For most pages, not many but it happens more often than you think.  The
> data I have is much too old to be useful now but the front page could,
> at times, have up to 30 people waiting for it without caching.  This is
> a very important patch believe it or not.  For a site the size of
> kernel.org it cannot exist without this.
> 
> But here's a quick stat, in 36 hours git.kernel.org has had
> 156099 accesses world wide or about 1.2 accesses a second.
> 
> android.git.kernel.org, in the same time period has had 115818 accesses.
> 
> If the first request takes 7 seconds to generate, by the time it's done
> there are now 3 additional requests running.  If it again takes 7
> seconds to generate there are now another 3 requests running, etc.  Very
> quickly you've got so much i/o running the box more or less is useless.

On the other hand do cache for front page (projects list) ever expire
so that there are no stale data to serve?  So the issue with growing I/O
is I guess only with priming the cache.  

BTW. I wonder how well the 'expires_variance' solution used in CHI would
protect against cache miss stampedes; that probably depends on the ratio
of variance to time it takes gitweb to generate the page.

>> (ii) Can't the locked gitwebs do the equivalent of tail -f?
> 
> Not really going to help much, most of the gitweb operations won't
> output much of anything beyond the header until it's collected all of
> the data it needs anyway and then there will be a flurry of output.  

Well, it might be enough to have headers as activity indicator... but
on the other hand users can think that server is broken if they see no
further data, as opposed to the "Generating..." page.

> It also means that this 'Generating...' page will only work for caching
> schemes that tail can read out of, which I'm not sure it would work all
> that well with things like memcached or a non-custom caching layer where
> we don't necessarily have direct access to the file being written to.

I don't think that caching interfaces such as CHI or Cache::Cache, or
individual caching engines such as Cache::Memcached or Cache::FastMmap
offer streaming access.  OTOH we can read from the side as the data
is generated ("tee" to a temporary file, or something), but it is even
more complication.

> 
> At least the way I had it (and I'll admit I haven't read through Jakub's
> re-working of my patches so I don't know if it's still there) is that
> with background caching you only get the 'Generating...' page if it's
> new or the content is grossly out of data.  If it's a popular page and
> it's not grossly out of date it shows you the 'stale' data while it
> generates the new content in the background anyway, only locking you out
> when the new file is being written.  Or at least that's how I had it.

It is there, i.e. gitweb would serve stale data if it can be found,
but it lacks 'too stale to serve as stale data' timeout/expire time.
I skipped this feature to get split series out of door faster.

-- 
Jakub Narebski
Poland

^ permalink raw reply

* [PATCH] t5541-http-push: make grep expression check for one line only
From: Tay Ray Chuan @ 2010-01-26  1:37 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano
In-Reply-To: <be6fef0d1001251733o57042a99q9f1f8b6e849187ee@mail.gmail.com>

Don't feed multiple-line pattern to grep and expect them to match with
lines in order.

Simplify the grep expressions in the non-fast-forward tests to check
only for the first line of the non-fast-forward warning - having that
line should be enough assurance that the full warning is printed.

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
 t/t5541-http-push.sh |   12 ++++--------
 1 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/t/t5541-http-push.sh b/t/t5541-http-push.sh
index 83a8e14..53f54a2 100755
--- a/t/t5541-http-push.sh
+++ b/t/t5541-http-push.sh
@@ -105,10 +105,8 @@ test_expect_success 'non-fast-forward push show ref status' '
 '
 
 test_expect_success 'non-fast-forward push shows help message' '
-	grep \
-"To prevent you from losing history, non-fast-forward updates were rejected
-Merge the remote changes before pushing again.  See the '"'non-fast-forward'"'
-section of '"'git push --help'"' for details." output
+	grep "To prevent you from losing history, non-fast-forward updates were rejected" \
+		output
 '
 
 test_expect_success 'push fails for non-fast-forward refs unmatched by remote helper' '
@@ -126,10 +124,8 @@ test_expect_success 'push fails for non-fast-forward refs unmatched by remote he
 	grep "^ + [a-f0-9]*\.\.\.[a-f0-9]* *master -> master (forced update)$" output &&
 	grep "^ ! \[rejected\] *master -> retsam (non-fast-forward)$" output &&
 
-	grep \
-"To prevent you from losing history, non-fast-forward updates were rejected
-Merge the remote changes before pushing again.  See the '"'non-fast-forward'"'
-section of '"'git push --help'"' for details." output
+	grep "To prevent you from losing history, non-fast-forward updates were rejected" \
+		output
 '
 
 stop_httpd
-- 
1.6.6.1.436.gaba7d

^ permalink raw reply related

* [PATCH] git-gui: use themed tk widgets with Tk 8.5
From: Pat Thoyts @ 2010-01-26  0:05 UTC (permalink / raw)
  To: git; +Cc: Shawn O. Pearce, msysgit


This patch enables the use of themed Tk widgets with Tk 8.5 and above.
These make a significant difference on Windows in making the
application appear native. On Windows and MacOSX ttk defaults to the
native look as much as possible. On X11 the user may select a theme
using the TkTheme XRDB resource class by adding an line to the
.Xresources file. The set of installed theme names is available using
the Tk command 'ttk::themes'. The default on X11 is similar to the current
un-themed style - a kind of thin bordered motif look.

A new git config variable 'gui.usettk' may be set to disable this if
the user prefers the classic Tk look. Using Tk 8.4 will also avoid the
use of themed widgets as these are only available since 8.5.

Some support is included for Tk 8.6 features (themed spinbox and native
font chooser for MacOSX and Windows).

Signed-off-by: Pat Thoyts <patthoyts@users.sourceforge.net>
---

There is a screenshot of this running on Windows 7 at
http://www.patthoyts.tk/screenshots/git-gui-win32.png
It has been tested on Linux but not on MacOSX yet.

 git-gui.sh                   |  158 ++++++++++++++++++++++++--------------
 lib/about.tcl                |   37 +++-------
 lib/blame.tcl                |   22 +++---
 lib/branch_checkout.tcl      |   20 +++--
 lib/branch_create.tcl        |   45 ++++++-----
 lib/branch_delete.tcl        |   24 +++---
 lib/branch_rename.tcl        |   32 +++++---
 lib/browser.tcl              |   41 ++++++-----
 lib/choose_font.tcl          |   31 ++++----
 lib/choose_repository.tcl    |   94 +++++++++++------------
 lib/choose_rev.tcl           |   51 +++++++------
 lib/class.tcl                |    7 ++
 lib/console.tcl              |   17 ++--
 lib/database.tcl             |   20 +++---
 lib/error.tcl                |   15 ++--
 lib/index.tcl                |   13 ++-
 lib/merge.tcl                |   14 ++--
 lib/option.tcl               |   67 +++++++++-------
 lib/remote_add.tcl           |   37 +++++-----
 lib/remote_branch_delete.tcl |   51 ++++++------
 lib/search.tcl               |   11 ++-
 lib/sshkey.tcl               |   20 +++--
 lib/status_bar.tcl           |   14 ++--
 lib/themed.tcl               |  174 ++++++++++++++++++++++++++++++++++++++++++
 lib/tools_dlg.tcl            |   87 ++++++++++-----------
 lib/transport.tcl            |   48 ++++++-----
 26 files changed, 697 insertions(+), 453 deletions(-)
 create mode 100644 lib/themed.tcl

diff --git a/git-gui.sh b/git-gui.sh
index 6f47624..5749547 100755
--- a/git-gui.sh
+++ b/git-gui.sh
@@ -677,12 +677,17 @@ if {[is_Windows]} {
 ## config defaults
 
 set cursor_ptr arrow
-font create font_diff -family Courier -size 10
 font create font_ui
-catch {
-	label .dummy
-	eval font configure font_ui [font actual [.dummy cget -font]]
-	destroy .dummy
+if {[lsearch -exact [font names] TkDefaultFont] != -1} {
+	eval [linsert [font actual TkDefaultFont] 0 font configure font_ui]
+	eval [linsert [font actual TkFixedFont] 0 font create font_diff]
+} else {
+	font create font_diff -family Courier -size 10
+	catch {
+		label .dummy
+		eval font configure font_ui [font actual [.dummy cget -font]]
+		destroy .dummy
+	}
 }
 
 font create font_uiitalic
@@ -697,6 +702,9 @@ foreach class {Button Checkbutton Entry Label
 }
 if {![is_MacOSX]} {
 	option add *Menu.font font_ui
+	option add *Entry.borderWidth 1 startupFile
+	option add *Entry.relief sunken startupFile
+	option add *RadioButton.anchor w startupFile
 }
 unset class
 
@@ -749,6 +757,18 @@ proc apply_config {} {
 		font configure ${font}bold -weight bold
 		font configure ${font}italic -slant italic
 	}
+
+	global use_ttk NS
+	set use_ttk 0
+	set NS {}
+	if {$repo_config(gui.usettk)} {
+		set use_ttk [package vsatisfies [package provide Tk] 8.5]
+		if {$use_ttk} {
+			set NS ttk
+			bind [winfo class .] <<ThemeChanged>> [list InitTheme]
+			pave_toplevel .
+		}
+	}
 }
 
 set default_config(branch.autosetupmerge) true
@@ -775,6 +795,7 @@ set default_config(gui.fontui) [font configure font_ui]
 set default_config(gui.fontdiff) [font configure font_diff]
 # TODO: this option should be added to the git-config documentation
 set default_config(gui.maxfilesdisplayed) 5000
+set default_config(gui.usettk) 1
 set font_descs {
 	{fontui   font_ui   {mc "Main Font"}}
 	{fontdiff font_diff {mc "Diff/Console Font"}}
@@ -2093,7 +2114,7 @@ proc do_quit {{rc {1}}} {
 	global ui_comm is_quitting repo_config commit_type
 	global GITGUI_BCK_exists GITGUI_BCK_i
 	global ui_comm_spell
-	global ret_code
+	global ret_code use_ttk
 
 	if {$is_quitting} return
 	set is_quitting 1
@@ -2151,8 +2172,13 @@ proc do_quit {{rc {1}}} {
 		}
 		set cfg_geometry [list]
 		lappend cfg_geometry [wm geometry .]
-		lappend cfg_geometry [lindex [.vpane sash coord 0] 0]
-		lappend cfg_geometry [lindex [.vpane.files sash coord 0] 1]
+		if {$use_ttk} {
+			lappend cfg_geometry [.vpane sashpos 0]
+			lappend cfg_geometry [.vpane.files sashpos 0]
+		} else {
+			lappend cfg_geometry [lindex [.vpane sash coord 0] 0]
+			lappend cfg_geometry [lindex [.vpane.files sash coord 0] 1]
+		}
 		if {[catch {set rc_geometry $repo_config(gui.geometry)}]} {
 			set rc_geometry {}
 		}
@@ -2919,14 +2945,13 @@ default {
 
 # -- Branch Control
 #
-frame .branch \
-	-borderwidth 1 \
-	-relief sunken
-label .branch.l1 \
+${NS}::frame .branch
+if {!$use_ttk} {.branch configure -borderwidth 1 -relief sunken}
+${NS}::label .branch.l1 \
 	-text [mc "Current Branch:"] \
 	-anchor w \
 	-justify left
-label .branch.cb \
+${NS}::label .branch.cb \
 	-textvariable current_branch \
 	-anchor w \
 	-justify left
@@ -2936,15 +2961,20 @@ pack .branch -side top -fill x
 
 # -- Main Window Layout
 #
-panedwindow .vpane -orient horizontal
-panedwindow .vpane.files -orient vertical
-.vpane add .vpane.files -sticky nsew -height 100 -width 200
+${NS}::panedwindow .vpane -orient horizontal
+${NS}::panedwindow .vpane.files -orient vertical
+if {$use_ttk} {
+	.vpane add .vpane.files
+} else {
+	.vpane add .vpane.files -sticky nsew -height 100 -width 200
+}
 pack .vpane -anchor n -side top -fill both -expand 1
 
 # -- Index File List
 #
-frame .vpane.files.index -height 100 -width 200
-label .vpane.files.index.title -text [mc "Staged Changes (Will Commit)"] \
+${NS}::frame .vpane.files.index -height 100 -width 200
+tlabel .vpane.files.index.title \
+	-text [mc "Staged Changes (Will Commit)"] \
 	-background lightgreen -foreground black
 text $ui_index -background white -foreground black \
 	-borderwidth 0 \
@@ -2954,8 +2984,8 @@ text $ui_index -background white -foreground black \
 	-xscrollcommand {.vpane.files.index.sx set} \
 	-yscrollcommand {.vpane.files.index.sy set} \
 	-state disabled
-scrollbar .vpane.files.index.sx -orient h -command [list $ui_index xview]
-scrollbar .vpane.files.index.sy -orient v -command [list $ui_index yview]
+${NS}::scrollbar .vpane.files.index.sx -orient h -command [list $ui_index xview]
+${NS}::scrollbar .vpane.files.index.sy -orient v -command [list $ui_index yview]
 pack .vpane.files.index.title -side top -fill x
 pack .vpane.files.index.sx -side bottom -fill x
 pack .vpane.files.index.sy -side right -fill y
@@ -2963,8 +2993,8 @@ pack $ui_index -side left -fill both -expand 1
 
 # -- Working Directory File List
 #
-frame .vpane.files.workdir -height 100 -width 200
-label .vpane.files.workdir.title -text [mc "Unstaged Changes"] \
+${NS}::frame .vpane.files.workdir -height 100 -width 200
+tlabel .vpane.files.workdir.title -text [mc "Unstaged Changes"] \
 	-background lightsalmon -foreground black
 text $ui_workdir -background white -foreground black \
 	-borderwidth 0 \
@@ -2974,15 +3004,19 @@ text $ui_workdir -background white -foreground black \
 	-xscrollcommand {.vpane.files.workdir.sx set} \
 	-yscrollcommand {.vpane.files.workdir.sy set} \
 	-state disabled
-scrollbar .vpane.files.workdir.sx -orient h -command [list $ui_workdir xview]
-scrollbar .vpane.files.workdir.sy -orient v -command [list $ui_workdir yview]
+${NS}::scrollbar .vpane.files.workdir.sx -orient h -command [list $ui_workdir xview]
+${NS}::scrollbar .vpane.files.workdir.sy -orient v -command [list $ui_workdir yview]
 pack .vpane.files.workdir.title -side top -fill x
 pack .vpane.files.workdir.sx -side bottom -fill x
 pack .vpane.files.workdir.sy -side right -fill y
 pack $ui_workdir -side left -fill both -expand 1
 
-.vpane.files add .vpane.files.workdir -sticky nsew
-.vpane.files add .vpane.files.index -sticky nsew
+.vpane.files add .vpane.files.workdir
+.vpane.files add .vpane.files.index
+if {!$use_ttk} {
+	.vpane.files paneconfigure .vpane.files.workdir -sticky news
+	.vpane.files paneconfigure .vpane.files.index -sticky news
+}
 
 foreach i [list $ui_index $ui_workdir] {
 	rmsel_tag $i
@@ -2992,68 +3026,69 @@ unset i
 
 # -- Diff and Commit Area
 #
-frame .vpane.lower -height 300 -width 400
-frame .vpane.lower.commarea
-frame .vpane.lower.diff -relief sunken -borderwidth 1
+${NS}::frame .vpane.lower -height 300 -width 400
+${NS}::frame .vpane.lower.commarea
+${NS}::frame .vpane.lower.diff -relief sunken -borderwidth 1
 pack .vpane.lower.diff -fill both -expand 1
 pack .vpane.lower.commarea -side bottom -fill x
-.vpane add .vpane.lower -sticky nsew
+.vpane add .vpane.lower
+if {!$use_ttk} {.vpane paneconfigure .vpane.lower -sticky nsew}
 
 # -- Commit Area Buttons
 #
-frame .vpane.lower.commarea.buttons
-label .vpane.lower.commarea.buttons.l -text {} \
+${NS}::frame .vpane.lower.commarea.buttons
+${NS}::label .vpane.lower.commarea.buttons.l -text {} \
 	-anchor w \
 	-justify left
 pack .vpane.lower.commarea.buttons.l -side top -fill x
 pack .vpane.lower.commarea.buttons -side left -fill y
 
-button .vpane.lower.commarea.buttons.rescan -text [mc Rescan] \
+${NS}::button .vpane.lower.commarea.buttons.rescan -text [mc Rescan] \
 	-command ui_do_rescan
 pack .vpane.lower.commarea.buttons.rescan -side top -fill x
 lappend disable_on_lock \
 	{.vpane.lower.commarea.buttons.rescan conf -state}
 
-button .vpane.lower.commarea.buttons.incall -text [mc "Stage Changed"] \
+${NS}::button .vpane.lower.commarea.buttons.incall -text [mc "Stage Changed"] \
 	-command do_add_all
 pack .vpane.lower.commarea.buttons.incall -side top -fill x
 lappend disable_on_lock \
 	{.vpane.lower.commarea.buttons.incall conf -state}
 
 if {![is_enabled nocommitmsg]} {
-	button .vpane.lower.commarea.buttons.signoff -text [mc "Sign Off"] \
+	${NS}::button .vpane.lower.commarea.buttons.signoff -text [mc "Sign Off"] \
 		-command do_signoff
 	pack .vpane.lower.commarea.buttons.signoff -side top -fill x
 }
 
-button .vpane.lower.commarea.buttons.commit -text [commit_btn_caption] \
+${NS}::button .vpane.lower.commarea.buttons.commit -text [commit_btn_caption] \
 	-command do_commit
 pack .vpane.lower.commarea.buttons.commit -side top -fill x
 lappend disable_on_lock \
 	{.vpane.lower.commarea.buttons.commit conf -state}
 
 if {![is_enabled nocommit]} {
-	button .vpane.lower.commarea.buttons.push -text [mc Push] \
+	${NS}::button .vpane.lower.commarea.buttons.push -text [mc Push] \
 		-command do_push_anywhere
 	pack .vpane.lower.commarea.buttons.push -side top -fill x
 }
 
 # -- Commit Message Buffer
 #
-frame .vpane.lower.commarea.buffer
-frame .vpane.lower.commarea.buffer.header
+${NS}::frame .vpane.lower.commarea.buffer
+${NS}::frame .vpane.lower.commarea.buffer.header
 set ui_comm .vpane.lower.commarea.buffer.t
 set ui_coml .vpane.lower.commarea.buffer.header.l
 
 if {![is_enabled nocommit]} {
-	radiobutton .vpane.lower.commarea.buffer.header.new \
+	${NS}::radiobutton .vpane.lower.commarea.buffer.header.new \
 		-text [mc "New Commit"] \
 		-command do_select_commit_type \
 		-variable selected_commit_type \
 		-value new
 	lappend disable_on_lock \
 		[list .vpane.lower.commarea.buffer.header.new conf -state]
-	radiobutton .vpane.lower.commarea.buffer.header.amend \
+	${NS}::radiobutton .vpane.lower.commarea.buffer.header.amend \
 		-text [mc "Amend Last Commit"] \
 		-command do_select_commit_type \
 		-variable selected_commit_type \
@@ -3062,7 +3097,7 @@ if {![is_enabled nocommit]} {
 		[list .vpane.lower.commarea.buffer.header.amend conf -state]
 }
 
-label $ui_coml \
+${NS}::label $ui_coml \
 	-anchor w \
 	-justify left
 proc trace_commit_type {varname args} {
@@ -3094,7 +3129,7 @@ text $ui_comm -background white -foreground black \
 	-width $repo_config(gui.commitmsgwidth) -height 9 -wrap none \
 	-font font_diff \
 	-yscrollcommand {.vpane.lower.commarea.buffer.sby set}
-scrollbar .vpane.lower.commarea.buffer.sby \
+${NS}::scrollbar .vpane.lower.commarea.buffer.sby \
 	-command [list $ui_comm yview]
 pack .vpane.lower.commarea.buffer.header -side top -fill x
 pack .vpane.lower.commarea.buffer.sby -side right -fill y
@@ -3160,19 +3195,19 @@ proc trace_current_diff_path {varname args} {
 }
 trace add variable current_diff_path write trace_current_diff_path
 
-frame .vpane.lower.diff.header -background gold
-label .vpane.lower.diff.header.status \
+gold_frame .vpane.lower.diff.header
+tlabel .vpane.lower.diff.header.status \
 	-background gold \
 	-foreground black \
 	-width $max_status_desc \
 	-anchor w \
 	-justify left
-label .vpane.lower.diff.header.file \
+tlabel .vpane.lower.diff.header.file \
 	-background gold \
 	-foreground black \
 	-anchor w \
 	-justify left
-label .vpane.lower.diff.header.path \
+tlabel .vpane.lower.diff.header.path \
 	-background gold \
 	-foreground black \
 	-anchor w \
@@ -3196,7 +3231,7 @@ bind_button3 .vpane.lower.diff.header.path "tk_popup $ctxm %X %Y"
 
 # -- Diff Body
 #
-frame .vpane.lower.diff.body
+${NS}::frame .vpane.lower.diff.body
 set ui_diff .vpane.lower.diff.body.t
 text $ui_diff -background white -foreground black \
 	-borderwidth 0 \
@@ -3205,9 +3240,9 @@ text $ui_diff -background white -foreground black \
 	-xscrollcommand {.vpane.lower.diff.body.sbx set} \
 	-yscrollcommand {.vpane.lower.diff.body.sby set} \
 	-state disabled
-scrollbar .vpane.lower.diff.body.sbx -orient horizontal \
+${NS}::scrollbar .vpane.lower.diff.body.sbx -orient horizontal \
 	-command [list $ui_diff xview]
-scrollbar .vpane.lower.diff.body.sby -orient vertical \
+${NS}::scrollbar .vpane.lower.diff.body.sby -orient vertical \
 	-command [list $ui_diff yview]
 pack .vpane.lower.diff.body.sbx -side bottom -fill x
 pack .vpane.lower.diff.body.sby -side right -fill y
@@ -3428,12 +3463,17 @@ $main_status show [mc "Initializing..."]
 catch {
 set gm $repo_config(gui.geometry)
 wm geometry . [lindex $gm 0]
-.vpane sash place 0 \
-	[lindex $gm 1] \
-	[lindex [.vpane sash coord 0] 1]
-.vpane.files sash place 0 \
-	[lindex [.vpane.files sash coord 0] 0] \
-	[lindex $gm 2]
+if {$use_ttk} {
+	.vpane sashpos 0 [lindex $gm 1]
+	.vpane.files sashpos 0 [lindex $gm 2]
+} else {
+	.vpane sash place 0 \
+		[lindex $gm 1] \
+		[lindex [.vpane sash coord 0] 1]
+	.vpane.files sash place 0 \
+		[lindex [.vpane.files sash coord 0] 0] \
+		[lindex $gm 2]
+}
 unset gm
 }
 
@@ -3701,3 +3741,9 @@ if {[is_enabled retcode]} {
 if {$picked && [is_config_true gui.autoexplore]} {
 	do_explore
 }
+
+# Local variables:
+# mode: tcl
+# indent-tabs-mode: t
+# tab-width: 4
+# End:
diff --git a/lib/about.tcl b/lib/about.tcl
index 241ab89..cfa50fc 100644
--- a/lib/about.tcl
+++ b/lib/about.tcl
@@ -4,31 +4,26 @@
 proc do_about {} {
 	global appvers copyright oguilib
 	global tcl_patchLevel tk_patchLevel
-	global ui_comm_spell
+	global ui_comm_spell NS use_ttk
 
 	set w .about_dialog
-	toplevel $w
+	Dialog $w
 	wm geometry $w "+[winfo rootx .]+[winfo rooty .]"
 
 	pack [git_logo $w.git_logo] -side left -fill y -padx 10 -pady 10
-	label $w.header -text [mc "About %s" [appname]] \
-		-font font_uibold
+	${NS}::label $w.header -text [mc "About %s" [appname]] \
+		-font font_uibold -anchor center
 	pack $w.header -side top -fill x
 
-	frame $w.buttons
-	button $w.buttons.close -text {Close} \
+	${NS}::frame $w.buttons
+	${NS}::button $w.buttons.close -text {Close} \
 		-default active \
 		-command [list destroy $w]
 	pack $w.buttons.close -side right
 	pack $w.buttons -side bottom -fill x -pady 10 -padx 10
 
-	label $w.desc \
-		-text "[mc "git-gui - a graphical user interface for Git."]\n$copyright" \
-		-padx 5 -pady 5 \
-		-justify left \
-		-anchor w \
-		-borderwidth 1 \
-		-relief solid
+	paddedlabel $w.desc \
+		-text "[mc "git-gui - a graphical user interface for Git."]\n$copyright"
 	pack $w.desc -side top -fill x -padx 5 -pady 5
 
 	set v {}
@@ -52,22 +47,10 @@ proc do_about {} {
 	append d "git exec dir: [gitexec]\n"
 	append d "git-gui lib: $oguilib"
 
-	label $w.vers \
-		-text $v \
-		-padx 5 -pady 5 \
-		-justify left \
-		-anchor w \
-		-borderwidth 1 \
-		-relief solid
+	paddedlabel $w.vers -text $v
 	pack $w.vers -side top -fill x -padx 5 -pady 5
 
-	label $w.dirs \
-		-text $d \
-		-padx 5 -pady 5 \
-		-justify left \
-		-anchor w \
-		-borderwidth 1 \
-		-relief solid
+	paddedlabel $w.dirs -text $d
 	pack $w.dirs -side top -fill x -padx 5 -pady 5
 
 	menu $w.ctxm -tearoff 0
diff --git a/lib/blame.tcl b/lib/blame.tcl
index 8525b79..786b50b 100644
--- a/lib/blame.tcl
+++ b/lib/blame.tcl
@@ -61,7 +61,7 @@ field tooltip_timer     {} ; # Current timer event for our tooltip
 field tooltip_commit    {} ; # Commit(s) in tooltip
 
 constructor new {i_commit i_path i_jump} {
-	global cursor_ptr M1B M1T have_tk85
+	global cursor_ptr M1B M1T have_tk85 use_ttk NS
 	variable active_color
 	variable group_colors
 
@@ -73,15 +73,15 @@ constructor new {i_commit i_path i_jump} {
 
 	set font_w [font measure font_diff "0"]
 
-	frame $w.header -background gold
-	label $w.header.commit_l \
+	gold_frame $w.header
+	tlabel $w.header.commit_l \
 		-text [mc "Commit:"] \
 		-background gold \
 		-foreground black \
 		-anchor w \
 		-justify left
 	set w_back $w.header.commit_b
-	label $w_back \
+	tlabel $w_back \
 		-image ::blame::img_back_arrow \
 		-borderwidth 0 \
 		-relief flat \
@@ -94,20 +94,20 @@ constructor new {i_commit i_path i_jump} {
 			[cb _history_menu]
 		}
 		"
-	label $w.header.commit \
+	tlabel $w.header.commit \
 		-textvariable @commit \
 		-background gold \
 		-foreground black \
 		-anchor w \
 		-justify left
-	label $w.header.path_l \
+	tlabel $w.header.path_l \
 		-text [mc "File:"] \
 		-background gold \
 		-foreground black \
 		-anchor w \
 		-justify left
 	set w_path $w.header.path
-	label $w_path \
+	tlabel $w_path \
 		-background gold \
 		-foreground black \
 		-anchor w \
@@ -209,10 +209,10 @@ constructor new {i_commit i_path i_jump} {
 
 	set w_columns [list $w_amov $w_asim $w_line $w_file]
 
-	scrollbar $w.file_pane.out.sbx \
+	${NS}::scrollbar $w.file_pane.out.sbx \
 		-orient h \
 		-command [list $w_file xview]
-	scrollbar $w.file_pane.out.sby \
+	${NS}::scrollbar $w.file_pane.out.sby \
 		-orient v \
 		-command [list scrollbar2many $w_columns yview]
 	eval grid $w_columns $w.file_pane.out.sby -sticky nsew
@@ -254,10 +254,10 @@ constructor new {i_commit i_path i_jump} {
 		-background $active_color \
 		-font font_ui
 	$w_cviewer tag raise sel
-	scrollbar $w.file_pane.cm.sbx \
+	${NS}::scrollbar $w.file_pane.cm.sbx \
 		-orient h \
 		-command [list $w_cviewer xview]
-	scrollbar $w.file_pane.cm.sby \
+	${NS}::scrollbar $w.file_pane.cm.sby \
 		-orient v \
 		-command [list $w_cviewer yview]
 	pack $w.file_pane.cm.sby -side right -fill y
diff --git a/lib/branch_checkout.tcl b/lib/branch_checkout.tcl
index 6603703..2e459a8 100644
--- a/lib/branch_checkout.tcl
+++ b/lib/branch_checkout.tcl
@@ -10,21 +10,24 @@ field opt_fetch     1; # refetch tracking branch if used?
 field opt_detach    0; # force a detached head case?
 
 constructor dialog {} {
-	make_toplevel top w
+	global use_ttk NS
+	make_dialog top w
+	wm withdraw $w
 	wm title $top [append "[appname] ([reponame]): " [mc "Checkout Branch"]]
 	if {$top ne {.}} {
 		wm geometry $top "+[winfo rootx .]+[winfo rooty .]"
 	}
 
-	label $w.header -text [mc "Checkout Branch"] -font font_uibold
+	${NS}::label $w.header -text [mc "Checkout Branch"] \
+		-font font_uibold -anchor center
 	pack $w.header -side top -fill x
 
-	frame $w.buttons
-	button $w.buttons.create -text [mc Checkout] \
+	${NS}::frame $w.buttons
+	${NS}::button $w.buttons.create -text [mc Checkout] \
 		-default active \
 		-command [cb _checkout]
 	pack $w.buttons.create -side right
-	button $w.buttons.cancel -text [mc Cancel] \
+	${NS}::button $w.buttons.cancel -text [mc Cancel] \
 		-command [list destroy $w]
 	pack $w.buttons.cancel -side right -padx 5
 	pack $w.buttons -side bottom -fill x -pady 10 -padx 10
@@ -33,14 +36,14 @@ constructor dialog {} {
 	$w_rev bind_listbox <Double-Button-1> [cb _checkout]
 	pack $w.rev -anchor nw -fill both -expand 1 -pady 5 -padx 5
 
-	labelframe $w.options -text [mc Options]
+	${NS}::labelframe $w.options -text [mc Options]
 
-	checkbutton $w.options.fetch \
+	${NS}::checkbutton $w.options.fetch \
 		-text [mc "Fetch Tracking Branch"] \
 		-variable @opt_fetch
 	pack $w.options.fetch -anchor nw
 
-	checkbutton $w.options.detach \
+	${NS}::checkbutton $w.options.detach \
 		-text [mc "Detach From Local Branch"] \
 		-variable @opt_detach
 	pack $w.options.detach -anchor nw
@@ -50,6 +53,7 @@ constructor dialog {} {
 	bind $w <Visibility> [cb _visible]
 	bind $w <Key-Escape> [list destroy $w]
 	bind $w <Key-Return> [cb _checkout]\;break
+	wm deiconify $w
 	tkwait window $w
 }
 
diff --git a/lib/branch_create.tcl b/lib/branch_create.tcl
index 3817771..4bb9077 100644
--- a/lib/branch_create.tcl
+++ b/lib/branch_create.tcl
@@ -16,48 +16,48 @@ field opt_fetch     1; # refetch tracking branch if used?
 field reset_ok      0; # did the user agree to reset?
 
 constructor dialog {} {
-	global repo_config
+	global repo_config use_ttk NS
 
-	make_toplevel top w
+	make_dialog top w
+	wm withdraw $w
 	wm title $top [append "[appname] ([reponame]): " [mc "Create Branch"]]
 	if {$top ne {.}} {
 		wm geometry $top "+[winfo rootx .]+[winfo rooty .]"
 	}
 
-	label $w.header -text [mc "Create New Branch"] -font font_uibold
+	${NS}::label $w.header -text [mc "Create New Branch"] \
+		-font font_uibold -anchor center
 	pack $w.header -side top -fill x
 
-	frame $w.buttons
-	button $w.buttons.create -text [mc Create] \
+	${NS}::frame $w.buttons
+	${NS}::button $w.buttons.create -text [mc Create] \
 		-default active \
 		-command [cb _create]
 	pack $w.buttons.create -side right
-	button $w.buttons.cancel -text [mc Cancel] \
+	${NS}::button $w.buttons.cancel -text [mc Cancel] \
 		-command [list destroy $w]
 	pack $w.buttons.cancel -side right -padx 5
 	pack $w.buttons -side bottom -fill x -pady 10 -padx 10
 
-	labelframe $w.desc -text [mc "Branch Name"]
-	radiobutton $w.desc.name_r \
-		-anchor w \
+	${NS}::labelframe $w.desc -text [mc "Branch Name"]
+	${NS}::radiobutton $w.desc.name_r \
 		-text [mc "Name:"] \
 		-value user \
 		-variable @name_type
+	if {!$use_ttk} {$w.desc.name_r configure -anchor w}
 	set w_name $w.desc.name_t
-	entry $w_name \
-		-borderwidth 1 \
-		-relief sunken \
+	${NS}::entry $w_name \
 		-width 40 \
 		-textvariable @name \
 		-validate key \
 		-validatecommand [cb _validate %d %S]
 	grid $w.desc.name_r $w_name -sticky we -padx {0 5}
 
-	radiobutton $w.desc.match_r \
-		-anchor w \
+	${NS}::radiobutton $w.desc.match_r \
 		-text [mc "Match Tracking Branch Name"] \
 		-value match \
 		-variable @name_type
+	if {!$use_ttk} {$w.desc.match_r configure -anchor w}
 	grid $w.desc.match_r -sticky we -padx {0 5} -columnspan 2
 
 	grid columnconfigure $w.desc 1 -weight 1
@@ -66,34 +66,34 @@ constructor dialog {} {
 	set w_rev [::choose_rev::new $w.rev [mc "Starting Revision"]]
 	pack $w.rev -anchor nw -fill both -expand 1 -pady 5 -padx 5
 
-	labelframe $w.options -text [mc Options]
+	${NS}::labelframe $w.options -text [mc Options]
 
-	frame $w.options.merge
-	label $w.options.merge.l -text [mc "Update Existing Branch:"]
+	${NS}::frame $w.options.merge
+	${NS}::label $w.options.merge.l -text [mc "Update Existing Branch:"]
 	pack $w.options.merge.l -side left
-	radiobutton $w.options.merge.no \
+	${NS}::radiobutton $w.options.merge.no \
 		-text [mc No] \
 		-value none \
 		-variable @opt_merge
 	pack $w.options.merge.no -side left
-	radiobutton $w.options.merge.ff \
+	${NS}::radiobutton $w.options.merge.ff \
 		-text [mc "Fast Forward Only"] \
 		-value ff \
 		-variable @opt_merge
 	pack $w.options.merge.ff -side left
-	radiobutton $w.options.merge.reset \
+	${NS}::radiobutton $w.options.merge.reset \
 		-text [mc Reset] \
 		-value reset \
 		-variable @opt_merge
 	pack $w.options.merge.reset -side left
 	pack $w.options.merge -anchor nw
 
-	checkbutton $w.options.fetch \
+	${NS}::checkbutton $w.options.fetch \
 		-text [mc "Fetch Tracking Branch"] \
 		-variable @opt_fetch
 	pack $w.options.fetch -anchor nw
 
-	checkbutton $w.options.checkout \
+	${NS}::checkbutton $w.options.checkout \
 		-text [mc "Checkout After Creation"] \
 		-variable @opt_checkout
 	pack $w.options.checkout -anchor nw
@@ -109,6 +109,7 @@ constructor dialog {} {
 	bind $w <Visibility> [cb _visible]
 	bind $w <Key-Escape> [list destroy $w]
 	bind $w <Key-Return> [cb _create]\;break
+	wm deiconify $w
 	tkwait window $w
 }
 
diff --git a/lib/branch_delete.tcl b/lib/branch_delete.tcl
index 20d5e42..867938e 100644
--- a/lib/branch_delete.tcl
+++ b/lib/branch_delete.tcl
@@ -9,41 +9,40 @@ field w_check         ; # revision picker for merge test
 field w_delete        ; # delete button
 
 constructor dialog {} {
-	global current_branch
+	global current_branch use_ttk NS
 
-	make_toplevel top w
+	make_dialog top w
+	wm withdraw $w
 	wm title $top [append "[appname] ([reponame]): " [mc "Delete Branch"]]
 	if {$top ne {.}} {
 		wm geometry $top "+[winfo rootx .]+[winfo rooty .]"
 	}
 
-	label $w.header -text [mc "Delete Local Branch"] -font font_uibold
+	${NS}::label $w.header -text [mc "Delete Local Branch"] \
+		-font font_uibold -anchor center
 	pack $w.header -side top -fill x
 
-	frame $w.buttons
+	${NS}::frame $w.buttons
 	set w_delete $w.buttons.delete
-	button $w_delete \
+	${NS}::button $w_delete \
 		-text [mc Delete] \
 		-default active \
 		-state disabled \
 		-command [cb _delete]
 	pack $w_delete -side right
-	button $w.buttons.cancel \
+	${NS}::button $w.buttons.cancel \
 		-text [mc Cancel] \
 		-command [list destroy $w]
 	pack $w.buttons.cancel -side right -padx 5
 	pack $w.buttons -side bottom -fill x -pady 10 -padx 10
 
-	labelframe $w.list -text [mc "Local Branches"]
+	${NS}::labelframe $w.list -text [mc "Local Branches"]
 	set w_heads $w.list.l
-	listbox $w_heads \
+	slistbox $w_heads \
 		-height 10 \
 		-width 70 \
 		-selectmode extended \
-		-exportselection false \
-		-yscrollcommand [list $w.list.sby set]
-	scrollbar $w.list.sby -command [list $w.list.l yview]
-	pack $w.list.sby -side right -fill y
+		-exportselection false
 	pack $w.list.l -side left -fill both -expand 1
 	pack $w.list -fill both -expand 1 -pady 5 -padx 5
 
@@ -67,6 +66,7 @@ constructor dialog {} {
 	"
 	bind $w <Key-Escape> [list destroy $w]
 	bind $w <Key-Return> [cb _delete]\;break
+	wm deiconify $w
 	tkwait window $w
 }
 
diff --git a/lib/branch_rename.tcl b/lib/branch_rename.tcl
index 1665388..6398877 100644
--- a/lib/branch_rename.tcl
+++ b/lib/branch_rename.tcl
@@ -8,9 +8,10 @@ field oldname
 field newname
 
 constructor dialog {} {
-	global current_branch
+	global current_branch use_ttk NS
 
-	make_toplevel top w
+	make_dialog top w
+	wm withdraw $w
 	wm title $top [append "[appname] ([reponame]): " [mc "Rename Branch"]]
 	if {$top ne {.}} {
 		wm geometry $top "+[winfo rootx .]+[winfo rooty .]"
@@ -19,27 +20,31 @@ constructor dialog {} {
 	set oldname $current_branch
 	set newname [get_config gui.newbranchtemplate]
 
-	label $w.header -text [mc "Rename Branch"] -font font_uibold
+	${NS}::label $w.header -text [mc "Rename Branch"]\
+		-font font_uibold -anchor center
 	pack $w.header -side top -fill x
 
-	frame $w.buttons
-	button $w.buttons.rename -text [mc Rename] \
+	${NS}::frame $w.buttons
+	${NS}::button $w.buttons.rename -text [mc Rename] \
 		-default active \
 		-command [cb _rename]
 	pack $w.buttons.rename -side right
-	button $w.buttons.cancel -text [mc Cancel] \
+	${NS}::button $w.buttons.cancel -text [mc Cancel] \
 		-command [list destroy $w]
 	pack $w.buttons.cancel -side right -padx 5
 	pack $w.buttons -side bottom -fill x -pady 10 -padx 10
 
-	frame $w.rename
-	label $w.rename.oldname_l -text [mc "Branch:"]
-	eval tk_optionMenu $w.rename.oldname_m @oldname [load_all_heads]
+	${NS}::frame $w.rename
+	${NS}::label $w.rename.oldname_l -text [mc "Branch:"]
+	if {$use_ttk} {
+		ttk::combobox $w.rename.oldname_m -textvariable @oldname \
+			-values [load_all_heads] -state readonly
+	} else {
+		eval tk_optionMenu $w.rename.oldname_m @oldname [load_all_heads]
+	}
 
-	label $w.rename.newname_l -text [mc "New Name:"]
-	entry $w.rename.newname_t \
-		-borderwidth 1 \
-		-relief sunken \
+	${NS}::label $w.rename.newname_l -text [mc "New Name:"]
+	${NS}::entry $w.rename.newname_t \
 		-width 40 \
 		-textvariable @newname \
 		-validate key \
@@ -60,6 +65,7 @@ constructor dialog {} {
 		$w.rename.newname_t icursor end
 		focus $w.rename.newname_t
 	"
+	wm deiconify $w
 	tkwait window $w
 }
 
diff --git a/lib/browser.tcl b/lib/browser.tcl
index 0410cc6..c241572 100644
--- a/lib/browser.tcl
+++ b/lib/browser.tcl
@@ -21,23 +21,23 @@ field browser_busy   1
 field ls_buf     {}; # Buffered record output from ls-tree
 
 constructor new {commit {path {}}} {
-	global cursor_ptr M1B
-	make_toplevel top w
+	global cursor_ptr M1B use_ttk NS
+	make_dialog top w
+	wm withdraw $top
 	wm title $top [append "[appname] ([reponame]): " [mc "File Browser"]]
 
 	set browser_commit $commit
 	set browser_path $browser_commit:$path
 
-	label $w.path \
+	${NS}::label $w.path \
 		-textvariable @browser_path \
 		-anchor w \
 		-justify left \
-		-borderwidth 1 \
-		-relief sunken \
 		-font font_uibold
+	if {!$use_ttk} { $w.path configure -borderwidth 1 -relief sunken}
 	pack $w.path -anchor w -side top -fill x
 
-	frame $w.list
+	${NS}::frame $w.list
 	set w_list $w.list.l
 	text $w_list -background white -foreground black \
 		-borderwidth 0 \
@@ -49,19 +49,18 @@ constructor new {commit {path {}}} {
 		-xscrollcommand [list $w.list.sbx set] \
 		-yscrollcommand [list $w.list.sby set]
 	rmsel_tag $w_list
-	scrollbar $w.list.sbx -orient h -command [list $w_list xview]
-	scrollbar $w.list.sby -orient v -command [list $w_list yview]
+	${NS}::scrollbar $w.list.sbx -orient h -command [list $w_list xview]
+	${NS}::scrollbar $w.list.sby -orient v -command [list $w_list yview]
 	pack $w.list.sbx -side bottom -fill x
 	pack $w.list.sby -side right -fill y
 	pack $w_list -side left -fill both -expand 1
 	pack $w.list -side top -fill both -expand 1
 
-	label $w.status \
+	${NS}::label $w.status \
 		-textvariable @browser_status \
 		-anchor w \
-		-justify left \
-		-borderwidth 1 \
-		-relief sunken
+		-justify left
+	if {!$use_ttk} { $w.status configure -borderwidth 1 -relief sunken}
 	pack $w.status -anchor w -side bottom -fill x
 
 	bind $w_list <Button-1>        "[cb _click 0 @%x,%y];break"
@@ -78,6 +77,7 @@ constructor new {commit {path {}}} {
 	bind $w_list <Right>           break
 
 	bind $w_list <Visibility> [list focus $w_list]
+	wm deiconify $top
 	set w $w_list
 	if {$path ne {}} {
 		_ls $this $browser_commit:$path $path
@@ -263,23 +263,27 @@ field w              ; # widget path
 field w_rev          ; # mega-widget to pick the initial revision
 
 constructor dialog {} {
-	make_toplevel top w
+	global use_ttk NS
+	make_dialog top w
+	wm withdraw $top
 	wm title $top [append "[appname] ([reponame]): " [mc "Browse Branch Files"]]
 	if {$top ne {.}} {
 		wm geometry $top "+[winfo rootx .]+[winfo rooty .]"
+		wm transient $top .
 	}
 
-	label $w.header \
+	${NS}::label $w.header \
 		-text [mc "Browse Branch Files"] \
-		-font font_uibold
+		-font font_uibold \
+		-anchor center
 	pack $w.header -side top -fill x
 
-	frame $w.buttons
-	button $w.buttons.browse -text [mc Browse] \
+	${NS}::frame $w.buttons
+	${NS}::button $w.buttons.browse -text [mc Browse] \
 		-default active \
 		-command [cb _open]
 	pack $w.buttons.browse -side right
-	button $w.buttons.cancel -text [mc Cancel] \
+	${NS}::button $w.buttons.cancel -text [mc Cancel] \
 		-command [list destroy $w]
 	pack $w.buttons.cancel -side right -padx 5
 	pack $w.buttons -side bottom -fill x -pady 10 -padx 10
@@ -291,6 +295,7 @@ constructor dialog {} {
 	bind $w <Visibility> [cb _visible]
 	bind $w <Key-Escape> [list destroy $w]
 	bind $w <Key-Return> [cb _open]\;break
+	wm deiconify $top
 	tkwait window $w
 }
 
diff --git a/lib/choose_font.tcl b/lib/choose_font.tcl
index 56443b0..ebe50bd 100644
--- a/lib/choose_font.tcl
+++ b/lib/choose_font.tcl
@@ -17,6 +17,7 @@ variable all_families [list]  ; # All fonts known to Tk
 
 constructor pick {path title a_family a_size} {
 	variable all_families
+	global use_ttk NS
 
 	set v_family $a_family
 	set v_size $a_size
@@ -27,29 +28,30 @@ constructor pick {path title a_family a_size} {
 	set f_family $pv_family
 	set f_size $pv_size
 
-	make_toplevel top w
+	make_dialog top w
+	wm withdraw $top
 	wm title $top "[appname] ([reponame]): $title"
 	wm geometry $top "+[winfo rootx $path]+[winfo rooty $path]"
 
-	label $w.header -text $title -font font_uibold
+	${NS}::label $w.header -text $title -font font_uibold -anchor center
 	pack $w.header -side top -fill x
 
-	frame $w.buttons
-	button $w.buttons.select \
+	${NS}::frame $w.buttons
+	${NS}::button $w.buttons.select \
 		-text [mc Select] \
 		-default active \
 		-command [cb _select]
-	button $w.buttons.cancel \
+	${NS}::button $w.buttons.cancel \
 		-text [mc Cancel] \
 		-command [list destroy $w]
 	pack $w.buttons.select -side right
 	pack $w.buttons.cancel -side right -padx 5
 	pack $w.buttons -side bottom -fill x -pady 10 -padx 10
 
-	frame $w.inner
+	${NS}::frame $w.inner
 
-	frame $w.inner.family
-	label $w.inner.family.l \
+	${NS}::frame $w.inner.family
+	${NS}::label $w.inner.family.l \
 		-text [mc "Font Family"] \
 		-anchor w
 	set w_family $w.inner.family.v
@@ -64,16 +66,16 @@ constructor pick {path title a_family a_size} {
 		-height 10 \
 		-yscrollcommand [list $w.inner.family.sby set]
 	rmsel_tag $w_family
-	scrollbar $w.inner.family.sby -command [list $w_family yview]
+	${NS}::scrollbar $w.inner.family.sby -command [list $w_family yview]
 	pack $w.inner.family.l -side top -fill x
 	pack $w.inner.family.sby -side right -fill y
 	pack $w_family -fill both -expand 1
 
-	frame $w.inner.size
-	label $w.inner.size.l \
+	${NS}::frame $w.inner.size
+	${NS}::label $w.inner.size.l \
 		-text [mc "Font Size"] \
 		-anchor w
-	spinbox $w.inner.size.v \
+	tspinbox $w.inner.size.v \
 		-textvariable @f_size \
 		-from 2 -to 80 -increment 1 \
 		-width 3
@@ -86,8 +88,8 @@ constructor pick {path title a_family a_size} {
 	grid columnconfigure $w.inner 0 -weight 1
 	pack $w.inner -fill both -expand 1 -padx 5 -pady 5
 
-	frame $w.example
-	label $w.example.l \
+	${NS}::frame $w.example
+	${NS}::label $w.example.l \
 		-text [mc "Font Example"] \
 		-anchor w
 	set w_example $w.example.t
@@ -129,6 +131,7 @@ constructor pick {path title a_family a_size} {
 		grab $w
 		focus $w
 	"
+	wm deiconify $w
 	tkwait window $w
 }
 
diff --git a/lib/choose_repository.tcl b/lib/choose_repository.tcl
index 3f8f303..64f0674 100644
--- a/lib/choose_repository.tcl
+++ b/lib/choose_repository.tcl
@@ -22,9 +22,9 @@ field readtree_err        ; # Error output from read-tree (if any)
 field sorted_recent       ; # recent repositories (sorted)
 
 constructor pick {} {
-	global M1T M1B
+	global M1T M1B use_ttk NS
 
-	make_toplevel top w
+	make_dialog top w
 	wm title $top [mc "Git Gui"]
 
 	if {$top eq {.}} {
@@ -71,11 +71,11 @@ constructor pick {} {
 
 	set w_body $w.body
 	set opts $w_body.options
-	frame $w_body
+	${NS}::frame $w_body
 	text $opts \
 		-cursor $::cursor_ptr \
 		-relief flat \
-		-background [$w_body cget -background] \
+		-background [get_bg_color $w_body] \
 		-wrap none \
 		-spacing1 5 \
 		-width 50 \
@@ -132,15 +132,15 @@ constructor pick {} {
 				-label [mc "Recent Repositories"]
 		}
 
-		label $w_body.space
-		label $w_body.recentlabel \
+		${NS}::label $w_body.space
+		${NS}::label $w_body.recentlabel \
 			-anchor w \
 			-text [mc "Open Recent Repository:"]
 		set w_recentlist $w_body.recentlist
 		text $w_recentlist \
 			-cursor $::cursor_ptr \
 			-relief flat \
-			-background [$w_body.recentlabel cget -background] \
+			-background [get_bg_color $w_body.recentlabel] \
 			-wrap none \
 			-width 50 \
 			-height 10
@@ -176,10 +176,10 @@ constructor pick {} {
 	}
 	pack $w_body -fill x -padx 10 -pady 10
 
-	frame $w.buttons
+	${NS}::frame $w.buttons
 	set w_next $w.buttons.next
 	set w_quit $w.buttons.quit
-	button $w_quit \
+	${NS}::button $w_quit \
 		-text [mc "Quit"] \
 		-command exit
 	pack $w_quit -side right -padx 5
@@ -203,6 +203,7 @@ constructor pick {} {
 	wm deiconify $top
 	tkwait variable @done
 
+	grab release $top
 	if {$top eq {.}} {
 		eval destroy [winfo children $top]
 	}
@@ -284,9 +285,10 @@ method _open_recent_path {p} {
 }
 
 method _next {action} {
+	global NS
 	destroy $w_body
 	if {![winfo exists $w_next]} {
-		button $w_next -default active
+		${NS}::button $w_next -default active
 		pack $w_next -side right -padx 5 -before $w_quit
 	}
 	_do_$action $this
@@ -375,26 +377,25 @@ proc _objdir {path} {
 ## Create New Repository
 
 method _do_new {} {
+	global use_ttk NS
 	$w_next conf \
 		-state disabled \
 		-command [cb _do_new2] \
 		-text [mc "Create"]
 
-	frame $w_body
-	label $w_body.h \
-		-font font_uibold \
+	${NS}::frame $w_body
+	${NS}::label $w_body.h \
+		-font font_uibold -anchor center \
 		-text [mc "Create New Repository"]
 	pack $w_body.h -side top -fill x -pady 10
 	pack $w_body -fill x -padx 10
 
-	frame $w_body.where
-	label $w_body.where.l -text [mc "Directory:"]
-	entry $w_body.where.t \
+	${NS}::frame $w_body.where
+	${NS}::label $w_body.where.l -text [mc "Directory:"]
+	${NS}::entry $w_body.where.t \
 		-textvariable @local_path \
-		-borderwidth 1 \
-		-relief sunken \
 		-width 50
-	button $w_body.where.b \
+	${NS}::button $w_body.where.b \
 		-text [mc "Browse"] \
 		-command [cb _new_local_path]
 	set w_localpath $w_body.where.t
@@ -460,63 +461,57 @@ proc _new_ok {p} {
 ## Clone Existing Repository
 
 method _do_clone {} {
+	global use_ttk NS
 	$w_next conf \
 		-state disabled \
 		-command [cb _do_clone2] \
 		-text [mc "Clone"]
 
-	frame $w_body
-	label $w_body.h \
-		-font font_uibold \
+	${NS}::frame $w_body
+	${NS}::label $w_body.h \
+		-font font_uibold -anchor center \
 		-text [mc "Clone Existing Repository"]
 	pack $w_body.h -side top -fill x -pady 10
 	pack $w_body -fill x -padx 10
 
 	set args $w_body.args
-	frame $w_body.args
+	${NS}::frame $w_body.args
 	pack $args -fill both
 
-	label $args.origin_l -text [mc "Source Location:"]
-	entry $args.origin_t \
+	${NS}::label $args.origin_l -text [mc "Source Location:"]
+	${NS}::entry $args.origin_t \
 		-textvariable @origin_url \
-		-borderwidth 1 \
-		-relief sunken \
 		-width 50
-	button $args.origin_b \
+	${NS}::button $args.origin_b \
 		-text [mc "Browse"] \
 		-command [cb _open_origin]
 	grid $args.origin_l $args.origin_t $args.origin_b -sticky ew
 
-	label $args.where_l -text [mc "Target Directory:"]
-	entry $args.where_t \
+	${NS}::label $args.where_l -text [mc "Target Directory:"]
+	${NS}::entry $args.where_t \
 		-textvariable @local_path \
-		-borderwidth 1 \
-		-relief sunken \
 		-width 50
-	button $args.where_b \
+	${NS}::button $args.where_b \
 		-text [mc "Browse"] \
 		-command [cb _new_local_path]
 	grid $args.where_l $args.where_t $args.where_b -sticky ew
 	set w_localpath $args.where_t
 
-	label $args.type_l -text [mc "Clone Type:"]
-	frame $args.type_f
+	${NS}::label $args.type_l -text [mc "Clone Type:"]
+	${NS}::frame $args.type_f
 	set w_types [list]
-	lappend w_types [radiobutton $args.type_f.hardlink \
+	lappend w_types [${NS}::radiobutton $args.type_f.hardlink \
 		-state disabled \
-		-anchor w \
 		-text [mc "Standard (Fast, Semi-Redundant, Hardlinks)"] \
 		-variable @clone_type \
 		-value hardlink]
-	lappend w_types [radiobutton $args.type_f.full \
+	lappend w_types [${NS}::radiobutton $args.type_f.full \
 		-state disabled \
-		-anchor w \
 		-text [mc "Full Copy (Slower, Redundant Backup)"] \
 		-variable @clone_type \
 		-value full]
-	lappend w_types [radiobutton $args.type_f.shared \
+	lappend w_types [${NS}::radiobutton $args.type_f.shared \
 		-state disabled \
-		-anchor w \
 		-text [mc "Shared (Fastest, Not Recommended, No Backup)"] \
 		-variable @clone_type \
 		-value shared]
@@ -1005,26 +1000,25 @@ method _postcheckout_wait {fd_ph} {
 ## Open Existing Repository
 
 method _do_open {} {
+	global NS
 	$w_next conf \
 		-state disabled \
 		-command [cb _do_open2] \
 		-text [mc "Open"]
 
-	frame $w_body
-	label $w_body.h \
-		-font font_uibold \
+	${NS}::frame $w_body
+	${NS}::label $w_body.h \
+		-font font_uibold -anchor center \
 		-text [mc "Open Existing Repository"]
 	pack $w_body.h -side top -fill x -pady 10
 	pack $w_body -fill x -padx 10
 
-	frame $w_body.where
-	label $w_body.where.l -text [mc "Repository:"]
-	entry $w_body.where.t \
+	${NS}::frame $w_body.where
+	${NS}::label $w_body.where.l -text [mc "Repository:"]
+	${NS}::entry $w_body.where.t \
 		-textvariable @local_path \
-		-borderwidth 1 \
-		-relief sunken \
 		-width 50
-	button $w_body.where.b \
+	${NS}::button $w_body.where.b \
 		-text [mc "Browse"] \
 		-command [cb _open_local_path]
 
diff --git a/lib/choose_rev.tcl b/lib/choose_rev.tcl
index c8821c1..c12d5e1 100644
--- a/lib/choose_rev.tcl
+++ b/lib/choose_rev.tcl
@@ -10,7 +10,7 @@ field w_list          ; # list of currently filtered specs
 field w_filter        ; # filter entry for $w_list
 
 field c_expr        {}; # current revision expression
-field filter          ; # current filter string
+field filter        ""; # current filter string
 field revtype     head; # type of revision chosen
 field cur_specs [list]; # list of specs for $revtype
 field spec_head       ; # list of all head specs
@@ -32,7 +32,7 @@ proc new_unmerged {path {title {}}} {
 }
 
 constructor _new {path unmerged_only title} {
-	global current_branch is_detached
+	global current_branch is_detached use_ttk NS
 
 	if {![info exists ::all_remotes]} {
 		load_all_remotes
@@ -41,65 +41,65 @@ constructor _new {path unmerged_only title} {
 	set w $path
 
 	if {$title ne {}} {
-		labelframe $w -text $title
+		${NS}::labelframe $w -text $title
 	} else {
-		frame $w
+		${NS}::frame $w
 	}
 	bind $w <Destroy> [cb _delete %W]
 
 	if {$is_detached} {
-		radiobutton $w.detachedhead_r \
-			-anchor w \
+		${NS}::radiobutton $w.detachedhead_r \
 			-text [mc "This Detached Checkout"] \
 			-value HEAD \
 			-variable @revtype
+		if {!$use_ttk} {$w.detachedhead_r configure -anchor w}
 		grid $w.detachedhead_r -sticky we -padx {0 5} -columnspan 2
 	}
 
-	radiobutton $w.expr_r \
+	${NS}::radiobutton $w.expr_r \
 		-text [mc "Revision Expression:"] \
 		-value expr \
 		-variable @revtype
-	entry $w.expr_t \
-		-borderwidth 1 \
-		-relief sunken \
+	${NS}::entry $w.expr_t \
 		-width 50 \
 		-textvariable @c_expr \
 		-validate key \
 		-validatecommand [cb _validate %d %S]
 	grid $w.expr_r $w.expr_t -sticky we -padx {0 5}
 
-	frame $w.types
-	radiobutton $w.types.head_r \
+	${NS}::frame $w.types
+	${NS}::radiobutton $w.types.head_r \
 		-text [mc "Local Branch"] \
 		-value head \
 		-variable @revtype
 	pack $w.types.head_r -side left
-	radiobutton $w.types.trck_r \
+	${NS}::radiobutton $w.types.trck_r \
 		-text [mc "Tracking Branch"] \
 		-value trck \
 		-variable @revtype
 	pack $w.types.trck_r -side left
-	radiobutton $w.types.tag_r \
+	${NS}::radiobutton $w.types.tag_r \
 		-text [mc "Tag"] \
 		-value tag \
 		-variable @revtype
 	pack $w.types.tag_r -side left
 	set w_filter $w.types.filter
-	entry $w_filter \
-		-borderwidth 1 \
-		-relief sunken \
+	${NS}::entry $w_filter \
 		-width 12 \
 		-textvariable @filter \
 		-validate key \
 		-validatecommand [cb _filter %P]
 	pack $w_filter -side right
-	pack [label $w.types.filter_icon \
+	pack [${NS}::label $w.types.filter_icon \
 		-image ::choose_rev::img_find \
 		] -side right
 	grid $w.types -sticky we -padx {0 5} -columnspan 2
 
-	frame $w.list
+	if {$use_ttk} {
+		ttk::frame $w.list -style SListbox.TFrame -padding 2
+	} else {
+		frame $w.list
+	}
 	set w_list $w.list.l
 	listbox $w_list \
 		-font font_diff \
@@ -109,6 +109,9 @@ constructor _new {path unmerged_only title} {
 		-exportselection false \
 		-xscrollcommand [cb _sb_set $w.list.sbx h] \
 		-yscrollcommand [cb _sb_set $w.list.sby v]
+	if {$use_ttk} {
+		$w_list configure -relief flat -highlightthickness 0 -borderwidth 0
+	}
 	pack $w_list -fill both -expand 1
 	grid $w.list -sticky nswe -padx {20 5} -columnspan 2
 	bind $w_list <Any-Motion>  [cb _show_tooltip @%x,%y]
@@ -235,11 +238,12 @@ constructor _new {path unmerged_only title} {
 }
 
 method none {text} {
+	global NS use_ttk
 	if {![winfo exists $w.none_r]} {
-		radiobutton $w.none_r \
-			-anchor w \
+		${NS}::radiobutton $w.none_r \
 			-value none \
 			-variable @revtype
+		if {!$use_ttk} {$w.none_r configure -anchor w}
 		grid $w.none_r -sticky we -padx {0 5} -columnspan 2
 	}
 	$w.none_r configure -text $text
@@ -425,6 +429,7 @@ method _delete {current} {
 }
 
 method _sb_set {sb orient first last} {
+	global NS
 	set old_focus [focus -lastfor $w]
 
 	if {$first == 0 && $last == 1} {
@@ -440,10 +445,10 @@ method _sb_set {sb orient first last} {
 
 	if {![winfo exists $sb]} {
 		if {$orient eq {h}} {
-			scrollbar $sb -orient h -command [list $w_list xview]
+			${NS}::scrollbar $sb -orient h -command [list $w_list xview]
 			pack $sb -fill x -side bottom -before $w_list
 		} else {
-			scrollbar $sb -orient v -command [list $w_list yview]
+			${NS}::scrollbar $sb -orient v -command [list $w_list yview]
 			pack $sb -fill y -side right -before $w_list
 		}
 		if {$old_focus ne {}} {
diff --git a/lib/class.tcl b/lib/class.tcl
index dc21411..c27b714 100644
--- a/lib/class.tcl
+++ b/lib/class.tcl
@@ -134,6 +134,13 @@ proc delete_this {{t {}}} {
 	if {[namespace exists $t]} {namespace delete $t}
 }
 
+proc make_dialog {t w args} {
+	upvar $t top $w pfx this this
+	global use_ttk
+	uplevel [linsert $args 0 make_toplevel $t $w]
+	pave_toplevel $pfx
+}
+
 proc make_toplevel {t w args} {
 	upvar $t top $w pfx this this
 
diff --git a/lib/console.tcl b/lib/console.tcl
index c112464..1f3248f 100644
--- a/lib/console.tcl
+++ b/lib/console.tcl
@@ -27,20 +27,20 @@ constructor embed {path title} {
 }
 
 method _init {} {
-	global M1B
+	global M1B use_ttk NS
 
 	if {$is_toplevel} {
-		make_toplevel top w -autodelete 0
+		make_dialog top w -autodelete 0
 		wm title $top "[appname] ([reponame]): $t_short"
 	} else {
-		frame $w
+		${NS}::frame $w
 	}
 
 	set console_cr 1.0
 	set w_t $w.m.t
 
-	frame $w.m
-	label $w.m.l1 \
+	${NS}::frame $w.m
+	${NS}::label $w.m.l1 \
 		-textvariable @t_long  \
 		-anchor w \
 		-justify left \
@@ -78,7 +78,7 @@ method _init {} {
 		"
 
 	if {$is_toplevel} {
-		button $w.ok -text [mc "Close"] \
+		${NS}::button $w.ok -text [mc "Close"] \
 			-state disabled \
 			-command [list destroy $w]
 		pack $w.ok -side bottom -anchor e -pady 10 -padx 10
@@ -206,13 +206,14 @@ method done {ok} {
 }
 
 method _sb_set {sb orient first last} {
+	global NS
 	if {![winfo exists $sb]} {
 		if {$first == $last || ($first == 0 && $last == 1)} return
 		if {$orient eq {h}} {
-			scrollbar $sb -orient h -command [list $w_t xview]
+			${NS}::scrollbar $sb -orient h -command [list $w_t xview]
 			pack $sb -fill x -side bottom -before $w_t
 		} else {
-			scrollbar $sb -orient v -command [list $w_t yview]
+			${NS}::scrollbar $sb -orient v -command [list $w_t yview]
 			pack $sb -fill y -side right -before $w_t
 		}
 	}
diff --git a/lib/database.tcl b/lib/database.tcl
index d4e0bed..1f187ed 100644
--- a/lib/database.tcl
+++ b/lib/database.tcl
@@ -2,6 +2,7 @@
 # Copyright (C) 2006, 2007 Shawn Pearce
 
 proc do_stats {} {
+	global use_ttk NS
 	set fd [git_read count-objects -v]
 	while {[gets $fd line] > 0} {
 		if {[regexp {^([^:]+): (\d+)$} $line _ name value]} {
@@ -21,24 +22,22 @@ proc do_stats {} {
 	}
 
 	set w .stats_view
-	toplevel $w
+	Dialog $w
+	wm withdraw $w
 	wm geometry $w "+[winfo rootx .]+[winfo rooty .]"
 
-	label $w.header -text [mc "Database Statistics"]
-	pack $w.header -side top -fill x
-
-	frame $w.buttons -border 1
-	button $w.buttons.close -text [mc Close] \
+	${NS}::frame $w.buttons
+	${NS}::button $w.buttons.close -text [mc Close] \
 		-default active \
 		-command [list destroy $w]
-	button $w.buttons.gc -text [mc "Compress Database"] \
+	${NS}::button $w.buttons.gc -text [mc "Compress Database"] \
 		-default normal \
 		-command "destroy $w;do_gc"
 	pack $w.buttons.close -side right
 	pack $w.buttons.gc -side left
 	pack $w.buttons -side bottom -fill x -pady 10 -padx 10
 
-	frame $w.stat -borderwidth 1 -relief solid
+	${NS}::labelframe $w.stat -text [mc "Database Statistics"]
 	foreach s {
 		{count           {mc "Number of loose objects"}}
 		{size            {mc "Disk space used by loose objects"} { KiB}}
@@ -55,8 +54,8 @@ proc do_stats {} {
 			set value "$value[lindex $s 2]"
 		}
 
-		label $w.stat.l_$name -text "$label:" -anchor w
-		label $w.stat.v_$name -text $value -anchor w
+		${NS}::label $w.stat.l_$name -text "$label:" -anchor w
+		${NS}::label $w.stat.v_$name -text $value -anchor w
 		grid $w.stat.l_$name $w.stat.v_$name -sticky we -padx {0 5}
 	}
 	pack $w.stat -pady 10 -padx 10
@@ -65,6 +64,7 @@ proc do_stats {} {
 	bind $w <Key-Escape> [list destroy $w]
 	bind $w <Key-Return> [list destroy $w]
 	wm title $w [append "[appname] ([reponame]): " [mc "Database Statistics"]]
+	wm deiconify $w
 	tkwait window $w
 }
 
diff --git a/lib/error.tcl b/lib/error.tcl
index 7565015..c0fa69a 100644
--- a/lib/error.tcl
+++ b/lib/error.tcl
@@ -71,11 +71,13 @@ proc ask_popup {msg} {
 }
 
 proc hook_failed_popup {hook msg {is_fatal 1}} {
+	global use_ttk NS
 	set w .hookfail
-	toplevel $w
+	Dialog $w
+	wm withdraw $w
 
-	frame $w.m
-	label $w.m.l1 -text "$hook hook failed:" \
+	${NS}::frame $w.m
+	${NS}::label $w.m.l1 -text "$hook hook failed:" \
 		-anchor w \
 		-justify left \
 		-font font_uibold
@@ -87,10 +89,10 @@ proc hook_failed_popup {hook msg {is_fatal 1}} {
 		-width 80 -height 10 \
 		-font font_diff \
 		-yscrollcommand [list $w.m.sby set]
-	scrollbar $w.m.sby -command [list $w.m.t yview]
+	${NS}::scrollbar $w.m.sby -command [list $w.m.t yview]
 	pack $w.m.l1 -side top -fill x
 	if {$is_fatal} {
-		label $w.m.l2 \
+		${NS}::label $w.m.l2 \
 			-text [mc "You must correct the above errors before committing."] \
 			-anchor w \
 			-justify left \
@@ -104,7 +106,7 @@ proc hook_failed_popup {hook msg {is_fatal 1}} {
 	$w.m.t insert 1.0 $msg
 	$w.m.t conf -state disabled
 
-	button $w.ok -text OK \
+	${NS}::button $w.ok -text OK \
 		-width 15 \
 		-command "destroy $w"
 	pack $w.ok -side bottom -anchor e -pady 10 -padx 10
@@ -112,5 +114,6 @@ proc hook_failed_popup {hook msg {is_fatal 1}} {
 	bind $w <Visibility> "grab $w; focus $w"
 	bind $w <Key-Return> "destroy $w"
 	wm title $w [strcat "[appname] ([reponame]): " [mc "error"]]
+	wm deiconify $w
 	tkwait window $w
 }
diff --git a/lib/index.tcl b/lib/index.tcl
index 0b58bd8..e9db0c4 100644
--- a/lib/index.tcl
+++ b/lib/index.tcl
@@ -8,26 +8,28 @@ proc _delete_indexlock {} {
 }
 
 proc _close_updateindex {fd after} {
+	global use_ttk NS
 	fconfigure $fd -blocking 1
 	if {[catch {close $fd} err]} {
 		set w .indexfried
-		toplevel $w
+		Dialog $w
+		wm withdraw $w
 		wm title $w [strcat "[appname] ([reponame]): " [mc "Index Error"]]
 		wm geometry $w "+[winfo rootx .]+[winfo rooty .]"
 		set s [mc "Updating the Git index failed.  A rescan will be automatically started to resynchronize git-gui."]
 		text $w.msg -yscrollcommand [list $w.vs set] \
 			-width [string length $s] -relief flat \
 			-borderwidth 0 -highlightthickness 0 \
-			-background [$w cget -background]
+			-background [get_bg_color $w]
 		$w.msg tag configure bold -font font_uibold -justify center
-		scrollbar $w.vs -command [list $w.msg yview]
+		${NS}::scrollbar $w.vs -command [list $w.msg yview]
 		$w.msg insert end $s bold \n\n$err {}
 		$w.msg configure -state disabled
 
-		button $w.continue \
+		${NS}::button $w.continue \
 			-text [mc "Continue"] \
 			-command [list destroy $w]
-		button $w.unlock \
+		${NS}::button $w.unlock \
 			-text [mc "Unlock Index"] \
 			-command "destroy $w; _delete_indexlock"
 		grid $w.msg - $w.vs -sticky news
@@ -40,6 +42,7 @@ proc _close_updateindex {fd after} {
 			grab $w
 			focus %W
 		"
+		wm deiconify $w
 		tkwait window $w
 
 		$::main_status stop
diff --git a/lib/merge.tcl b/lib/merge.tcl
index 283e491..5cded23 100644
--- a/lib/merge.tcl
+++ b/lib/merge.tcl
@@ -139,14 +139,14 @@ method _finish {cons ok} {
 
 constructor dialog {} {
 	global current_branch
-	global M1B
+	global M1B use_ttk NS
 
 	if {![_can_merge $this]} {
 		delete_this
 		return
 	}
 
-	make_toplevel top w
+	make_dialog top w
 	wm title $top [append "[appname] ([reponame]): " [mc "Merge"]]
 	if {$top ne {.}} {
 		wm geometry $top "+[winfo rootx .]+[winfo rooty .]"
@@ -154,21 +154,21 @@ constructor dialog {} {
 
 	set _start [cb _start]
 
-	label $w.header \
+	${NS}::label $w.header \
 		-text [mc "Merge Into %s" $current_branch] \
 		-font font_uibold
 	pack $w.header -side top -fill x
 
-	frame $w.buttons
-	button $w.buttons.visualize \
+	${NS}::frame $w.buttons
+	${NS}::button $w.buttons.visualize \
 		-text [mc Visualize] \
 		-command [cb _visualize]
 	pack $w.buttons.visualize -side left
-	button $w.buttons.merge \
+	${NS}::button $w.buttons.merge \
 		-text [mc Merge] \
 		-command $_start
 	pack $w.buttons.merge -side right
-	button $w.buttons.cancel \
+	${NS}::button $w.buttons.cancel \
 		-text [mc "Cancel"] \
 		-command [cb _cancel]
 	pack $w.buttons.cancel -side right -padx 5
diff --git a/lib/option.tcl b/lib/option.tcl
index 1d55b49..d4c5e45 100644
--- a/lib/option.tcl
+++ b/lib/option.tcl
@@ -91,7 +91,7 @@ proc save_config {} {
 proc do_options {} {
 	global repo_config global_config font_descs
 	global repo_config_new global_config_new
-	global ui_comm_spell
+	global ui_comm_spell use_ttk NS
 
 	array unset repo_config_new
 	array unset global_config_new
@@ -110,26 +110,28 @@ proc do_options {} {
 	}
 
 	set w .options_editor
-	toplevel $w
+	Dialog $w
+	wm withdraw $w
+	wm transient $w [winfo parent $w]
 	wm geometry $w "+[winfo rootx .]+[winfo rooty .]"
 
-	frame $w.buttons
-	button $w.buttons.restore -text [mc "Restore Defaults"] \
+	${NS}::frame $w.buttons
+	${NS}::button $w.buttons.restore -text [mc "Restore Defaults"] \
 		-default normal \
 		-command do_restore_defaults
 	pack $w.buttons.restore -side left
-	button $w.buttons.save -text [mc Save] \
+	${NS}::button $w.buttons.save -text [mc Save] \
 		-default active \
 		-command [list do_save_config $w]
 	pack $w.buttons.save -side right
-	button $w.buttons.cancel -text [mc "Cancel"] \
+	${NS}::button $w.buttons.cancel -text [mc "Cancel"] \
 		-default normal \
 		-command [list destroy $w]
 	pack $w.buttons.cancel -side right -padx 5
 	pack $w.buttons -side bottom -fill x -pady 10 -padx 10
 
-	labelframe $w.repo -text [mc "%s Repository" [reponame]]
-	labelframe $w.global -text [mc "Global (All Repositories)"]
+	${NS}::labelframe $w.repo -text [mc "%s Repository" [reponame]]
+	${NS}::labelframe $w.global -text [mc "Global (All Repositories)"]
 	pack $w.repo -side left -fill both -expand 1 -pady 5 -padx 5
 	pack $w.global -side right -fill both -expand 1 -pady 5 -padx 5
 
@@ -161,7 +163,7 @@ proc do_options {} {
 		foreach f {repo global} {
 			switch -glob -- $type {
 			b {
-				checkbutton $w.$f.$optid -text $text \
+				${NS}::checkbutton $w.$f.$optid -text $text \
 					-variable ${f}_config_new($name) \
 					-onvalue true \
 					-offvalue false
@@ -169,10 +171,10 @@ proc do_options {} {
 			}
 			i-* {
 				regexp -- {-(\d+)\.\.(\d+)$} $type _junk min max
-				frame $w.$f.$optid
-				label $w.$f.$optid.l -text "$text:"
+				${NS}::frame $w.$f.$optid
+				${NS}::label $w.$f.$optid.l -text "$text:"
 				pack $w.$f.$optid.l -side left -anchor w -fill x
-				spinbox $w.$f.$optid.v \
+				tspinbox $w.$f.$optid.v \
 					-textvariable ${f}_config_new($name) \
 					-from $min \
 					-to $max \
@@ -184,11 +186,9 @@ proc do_options {} {
 			}
 			c -
 			t {
-				frame $w.$f.$optid
-				label $w.$f.$optid.l -text "$text:"
-				entry $w.$f.$optid.v \
-					-borderwidth 1 \
-					-relief sunken \
+				${NS}::frame $w.$f.$optid
+				${NS}::label $w.$f.$optid.l -text "$text:"
+				${NS}::entry $w.$f.$optid.v \
 					-width 20 \
 					-textvariable ${f}_config_new($name)
 				pack $w.$f.$optid.l -side left -anchor w
@@ -199,7 +199,7 @@ proc do_options {} {
 					menu $w.$f.$optid.m
 					build_encoding_menu $w.$f.$optid.m \
 						[list set ${f}_config_new($name)] 1
-					button $w.$f.$optid.b \
+					${NS}::button $w.$f.$optid.b \
 						-text [mc "Change"] \
 						-command [list popup_btn_menu \
 							$w.$f.$optid.m $w.$f.$optid.b]
@@ -226,11 +226,17 @@ proc do_options {} {
 			set ${f}_config_new(gui.spellingdictionary) $value
 		}
 
-		frame $w.$f.$optid
-		label $w.$f.$optid.l -text [mc "Spelling Dictionary:"]
-		eval tk_optionMenu $w.$f.$optid.v \
-			${f}_config_new(gui.spellingdictionary) \
-			$all_dicts
+		${NS}::frame $w.$f.$optid
+		${NS}::label $w.$f.$optid.l -text [mc "Spelling Dictionary:"]
+		if {$use_ttk} {
+			ttk::combobox $w.$f.$optid.v \
+				-textvariable ${f}_config_new(gui.spellingdictionary) \
+				-values $all_dicts -state readonly
+		} else {
+			eval tk_optionMenu $w.$f.$optid.v \
+				${f}_config_new(gui.spellingdictionary) \
+				$all_dicts
+		}
 		pack $w.$f.$optid.l -side left -anchor w -fill x
 		pack $w.$f.$optid.v -side right -anchor e -padx 5
 		pack $w.$f.$optid -side top -anchor w -fill x
@@ -248,20 +254,20 @@ proc do_options {} {
 		set global_config_new(gui.$font^^size) \
 			[font configure $font -size]
 
-		frame $w.global.$name
-		label $w.global.$name.l -text "$text:"
-		button $w.global.$name.b \
+		${NS}::frame $w.global.$name
+		${NS}::label $w.global.$name.l -text "$text:"
+		${NS}::button $w.global.$name.b \
 			-text [mc "Change Font"] \
 			-command [list \
-				choose_font::pick \
+				tchoosefont \
 				$w \
 				[mc "Choose %s" $text] \
 				global_config_new(gui.$font^^family) \
 				global_config_new(gui.$font^^size) \
 				]
-		label $w.global.$name.f -textvariable global_config_new(gui.$font^^family)
-		label $w.global.$name.s -textvariable global_config_new(gui.$font^^size)
-		label $w.global.$name.pt -text [mc "pt."]
+		${NS}::label $w.global.$name.f -textvariable global_config_new(gui.$font^^family)
+		${NS}::label $w.global.$name.s -textvariable global_config_new(gui.$font^^size)
+		${NS}::label $w.global.$name.pt -text [mc "pt."]
 		pack $w.global.$name.l -side left -anchor w
 		pack $w.global.$name.b -side right -anchor e
 		pack $w.global.$name.pt -side right -anchor w
@@ -280,6 +286,7 @@ proc do_options {} {
 		set t [mc "Options"]
 	}
 	wm title $w "[appname] ([reponame]): $t"
+	wm deiconify $w
 	tkwait window $w
 }
 
diff --git a/lib/remote_add.tcl b/lib/remote_add.tcl
index fb29422..50029d0 100644
--- a/lib/remote_add.tcl
+++ b/lib/remote_add.tcl
@@ -13,45 +13,43 @@ field location     {}; # location of the remote the user has chosen
 field opt_action fetch; # action to do after registering the remote locally
 
 constructor dialog {} {
-	global repo_config
+	global repo_config use_ttk NS
 
-	make_toplevel top w
+	make_dialog top w
+	wm withdraw $top
 	wm title $top [append "[appname] ([reponame]): " [mc "Add Remote"]]
 	if {$top ne {.}} {
 		wm geometry $top "+[winfo rootx .]+[winfo rooty .]"
 	}
 
-	label $w.header -text [mc "Add New Remote"] -font font_uibold
+	${NS}::label $w.header -text [mc "Add New Remote"] \
+		-font font_uibold -anchor center
 	pack $w.header -side top -fill x
 
-	frame $w.buttons
-	button $w.buttons.create -text [mc Add] \
+	${NS}::frame $w.buttons
+	${NS}::button $w.buttons.create -text [mc Add] \
 		-default active \
 		-command [cb _add]
 	pack $w.buttons.create -side right
-	button $w.buttons.cancel -text [mc Cancel] \
+	${NS}::button $w.buttons.cancel -text [mc Cancel] \
 		-command [list destroy $w]
 	pack $w.buttons.cancel -side right -padx 5
 	pack $w.buttons -side bottom -fill x -pady 10 -padx 10
 
-	labelframe $w.desc -text [mc "Remote Details"]
+	${NS}::labelframe $w.desc -text [mc "Remote Details"]
 
-	label $w.desc.name_l -text [mc "Name:"]
+	${NS}::label $w.desc.name_l -text [mc "Name:"]
 	set w_name $w.desc.name_t
-	entry $w_name \
-		-borderwidth 1 \
-		-relief sunken \
+	${NS}::entry $w_name \
 		-width 40 \
 		-textvariable @name \
 		-validate key \
 		-validatecommand [cb _validate_name %d %S]
 	grid $w.desc.name_l $w_name -sticky we -padx {0 5}
 
-	label $w.desc.loc_l -text [mc "Location:"]
+	${NS}::label $w.desc.loc_l -text [mc "Location:"]
 	set w_loc $w.desc.loc_t
-	entry $w_loc \
-		-borderwidth 1 \
-		-relief sunken \
+	${NS}::entry $w_loc \
 		-width 40 \
 		-textvariable @location
 	grid $w.desc.loc_l $w_loc -sticky we -padx {0 5}
@@ -59,21 +57,21 @@ constructor dialog {} {
 	grid columnconfigure $w.desc 1 -weight 1
 	pack $w.desc -anchor nw -fill x -pady 5 -padx 5
 
-	labelframe $w.action -text [mc "Further Action"]
+	${NS}::labelframe $w.action -text [mc "Further Action"]
 
-	radiobutton $w.action.fetch \
+	${NS}::radiobutton $w.action.fetch \
 		-text [mc "Fetch Immediately"] \
 		-value fetch \
 		-variable @opt_action
 	pack $w.action.fetch -anchor nw
 
-	radiobutton $w.action.push \
+	${NS}::radiobutton $w.action.push \
 		-text [mc "Initialize Remote Repository and Push"] \
 		-value push \
 		-variable @opt_action
 	pack $w.action.push -anchor nw
 
-	radiobutton $w.action.none \
+	${NS}::radiobutton $w.action.none \
 		-text [mc "Do Nothing Else Now"] \
 		-value none \
 		-variable @opt_action
@@ -85,6 +83,7 @@ constructor dialog {} {
 	bind $w <Visibility> [cb _visible]
 	bind $w <Key-Escape> [list destroy $w]
 	bind $w <Key-Return> [cb _add]\;break
+	wm deiconify $top
 	tkwait window $w
 }
 
diff --git a/lib/remote_branch_delete.tcl b/lib/remote_branch_delete.tcl
index 2416420..f872a3d 100644
--- a/lib/remote_branch_delete.tcl
+++ b/lib/remote_branch_delete.tcl
@@ -23,34 +23,40 @@ field full_cache
 field cached
 
 constructor dialog {} {
-	global all_remotes M1B
+	global all_remotes M1B use_ttk NS
 
-	make_toplevel top w
+	make_dialog top w
 	wm title $top [append "[appname] ([reponame]): " [mc "Delete Branch Remotely"]]
 	if {$top ne {.}} {
 		wm geometry $top "+[winfo rootx .]+[winfo rooty .]"
 	}
 
-	label $w.header -text [mc "Delete Branch Remotely"] -font font_uibold
+	${NS}::label $w.header -text [mc "Delete Branch Remotely"] \
+		-font font_uibold -anchor center
 	pack $w.header -side top -fill x
 
-	frame $w.buttons
-	button $w.buttons.delete -text [mc Delete] \
+	${NS}::frame $w.buttons
+	${NS}::button $w.buttons.delete -text [mc Delete] \
 		-default active \
 		-command [cb _delete]
 	pack $w.buttons.delete -side right
-	button $w.buttons.cancel -text [mc "Cancel"] \
+	${NS}::button $w.buttons.cancel -text [mc "Cancel"] \
 		-command [list destroy $w]
 	pack $w.buttons.cancel -side right -padx 5
 	pack $w.buttons -side bottom -fill x -pady 10 -padx 10
 
-	labelframe $w.dest -text [mc "From Repository"]
+	${NS}::labelframe $w.dest -text [mc "From Repository"]
 	if {$all_remotes ne {}} {
-		radiobutton $w.dest.remote_r \
+		${NS}::radiobutton $w.dest.remote_r \
 			-text [mc "Remote:"] \
 			-value remote \
 			-variable @urltype
-		eval tk_optionMenu $w.dest.remote_m @remote $all_remotes
+		if {$use_ttk} {
+			ttk::combobox $w.dest.remote_m -textvariable @remote \
+				-values $all_remotes -state readonly
+		} else {
+			eval tk_optionMenu $w.dest.remote_m @remote $all_remotes
+		}
 		grid $w.dest.remote_r $w.dest.remote_m -sticky w
 		if {[lsearch -sorted -exact $all_remotes origin] != -1} {
 			set remote origin
@@ -62,13 +68,11 @@ constructor dialog {} {
 	} else {
 		set urltype url
 	}
-	radiobutton $w.dest.url_r \
+	${NS}::radiobutton $w.dest.url_r \
 		-text [mc "Arbitrary Location:"] \
 		-value url \
 		-variable @urltype
-	entry $w.dest.url_t \
-		-borderwidth 1 \
-		-relief sunken \
+	${NS}::entry $w.dest.url_t \
 		-width 50 \
 		-textvariable @url \
 		-validate key \
@@ -81,33 +85,30 @@ constructor dialog {} {
 	grid columnconfigure $w.dest 1 -weight 1
 	pack $w.dest -anchor nw -fill x -pady 5 -padx 5
 
-	labelframe $w.heads -text [mc "Branches"]
-	listbox $w.heads.l \
+	${NS}::labelframe $w.heads -text [mc "Branches"]
+	slistbox $w.heads.l \
 		-height 10 \
 		-width 70 \
 		-listvariable @head_list \
-		-selectmode extended \
-		-yscrollcommand [list $w.heads.sby set]
-	scrollbar $w.heads.sby -command [list $w.heads.l yview]
+		-selectmode extended
 
-	frame $w.heads.footer
-	label $w.heads.footer.status \
+	${NS}::frame $w.heads.footer
+	${NS}::label $w.heads.footer.status \
 		-textvariable @status \
 		-anchor w \
 		-justify left
-	button $w.heads.footer.rescan \
+	${NS}::button $w.heads.footer.rescan \
 		-text [mc "Rescan"] \
 		-command [cb _rescan]
 	pack $w.heads.footer.status -side left -fill x
 	pack $w.heads.footer.rescan -side right
 
 	pack $w.heads.footer -side bottom -fill x
-	pack $w.heads.sby -side right -fill y
 	pack $w.heads.l -side left -fill both -expand 1
 	pack $w.heads -fill both -expand 1 -pady 5 -padx 5
 
-	labelframe $w.validate -text [mc "Delete Only If"]
-	radiobutton $w.validate.head_r \
+	${NS}::labelframe $w.validate -text [mc "Delete Only If"]
+	${NS}::radiobutton $w.validate.head_r \
 		-text [mc "Merged Into:"] \
 		-value head \
 		-variable @checktype
@@ -115,7 +116,7 @@ constructor dialog {} {
 	trace add variable @head_list write [cb _write_head_list]
 	trace add variable @check_head write [cb _write_check_head]
 	grid $w.validate.head_r $w.validate.head_m -sticky w
-	radiobutton $w.validate.always_r \
+	${NS}::radiobutton $w.validate.always_r \
 		-text [mc "Always (Do not perform merge checks)"] \
 		-value always \
 		-variable @checktype
diff --git a/lib/search.tcl b/lib/search.tcl
index b371e9a..7fdbf87 100644
--- a/lib/search.tcl
+++ b/lib/search.tcl
@@ -14,15 +14,16 @@ field smarktop
 field smarkbot
 
 constructor new {i_w i_text args} {
+	global use_ttk NS
 	set w      $i_w
 	set ctext  $i_text
 
-	frame  $w
-	label  $w.l       -text [mc Find:]
+	${NS}::frame  $w
+	${NS}::label  $w.l       -text [mc Find:]
 	entry  $w.ent -textvariable ${__this}::searchstring -background lightgreen
-	button $w.bn      -text [mc Next] -command [cb find_next]
-	button $w.bp      -text [mc Prev] -command [cb find_prev]
-	checkbutton $w.cs -text [mc Case-Sensitive] \
+	${NS}::button $w.bn      -text [mc Next] -command [cb find_next]
+	${NS}::button $w.bp      -text [mc Prev] -command [cb find_prev]
+	${NS}::checkbutton $w.cs -text [mc Case-Sensitive] \
 		-variable ${__this}::casesensitive -command [cb _incrsearch]
 	pack   $w.l   -side left
 	pack   $w.cs  -side right
diff --git a/lib/sshkey.tcl b/lib/sshkey.tcl
index 82a1a80..5f75bc9 100644
--- a/lib/sshkey.tcl
+++ b/lib/sshkey.tcl
@@ -15,7 +15,7 @@ proc find_ssh_key {} {
 }
 
 proc do_ssh_key {} {
-	global sshkey_title have_tk85 sshkey_fd
+	global sshkey_title have_tk85 sshkey_fd use_ttk NS
 
 	set w .sshkey_dialog
 	if {[winfo exists $w]} {
@@ -23,7 +23,7 @@ proc do_ssh_key {} {
 		return
 	}
 
-	toplevel $w
+	Dialog $w
 	wm transient $w .
 
 	set finfo [find_ssh_key]
@@ -35,9 +35,9 @@ proc do_ssh_key {} {
 		set gen_state   disabled
 	}
 
-	frame $w.header -relief flat
-	label $w.header.lbl -textvariable sshkey_title -anchor w
-	button $w.header.gen -text [mc "Generate Key"] \
+	${NS}::frame $w.header
+	${NS}::label $w.header.lbl -textvariable sshkey_title -anchor w
+	${NS}::button $w.header.gen -text [mc "Generate Key"] \
 		-command [list make_ssh_key $w] -state $gen_state
 	pack $w.header.lbl -side left -expand 1 -fill x
 	pack $w.header.gen -side right
@@ -46,14 +46,16 @@ proc do_ssh_key {} {
 	text $w.contents -width 60 -height 10 -wrap char -relief sunken
 	pack $w.contents -fill both -expand 1
 	if {$have_tk85} {
-		$w.contents configure -inactiveselectbackground darkblue
+		set clr darkblue
+		if {$use_ttk} { set clr [ttk::style lookup . -selectbackground] }
+		$w.contents configure -inactiveselectbackground $clr
 	}
 
-	frame $w.buttons
-	button $w.buttons.close -text [mc Close] \
+	${NS}::frame $w.buttons
+	${NS}::button $w.buttons.close -text [mc Close] \
 		-default active -command [list destroy $w]
 	pack $w.buttons.close -side right
-	button $w.buttons.copy -text [mc "Copy To Clipboard"] \
+	${NS}::button $w.buttons.copy -text [mc "Copy To Clipboard"] \
 		-command [list tk_textCopy $w.contents]
 	pack $w.buttons.copy -side left
 	pack $w.buttons -side bottom -fill x -pady 5 -padx 5
diff --git a/lib/status_bar.tcl b/lib/status_bar.tcl
index 51d4177..5fe3aad 100644
--- a/lib/status_bar.tcl
+++ b/lib/status_bar.tcl
@@ -13,14 +13,16 @@ field units   {}; # unit of progress
 field meter   {}; # current core git progress meter (if active)
 
 constructor new {path} {
+	global use_ttk NS
 	set w $path
 	set w_l $w.l
 	set w_c $w.c
 
-	frame $w \
-		-borderwidth 1 \
-		-relief sunken
-	label $w_l \
+	${NS}::frame $w
+	if {!$use_ttk} {
+		$w configure -borderwidth 1 -relief sunken
+	}
+	${NS}::label $w_l \
 		-textvariable @status \
 		-anchor w \
 		-justify left
@@ -41,8 +43,8 @@ constructor two_line {path} {
 	set w_l $w.l
 	set w_c $w.c
 
-	frame $w
-	label $w_l \
+	${NS}::frame $w
+	${NS}::label $w_l \
 		-textvariable @status \
 		-anchor w \
 		-justify left
diff --git a/lib/themed.tcl b/lib/themed.tcl
new file mode 100644
index 0000000..1da4586
--- /dev/null
+++ b/lib/themed.tcl
@@ -0,0 +1,174 @@
+# Functions for supporting the use of themed Tk widgets in git-gui.
+# Copyright (C) 2009 Pat Thoyts <patthoyts@users.sourceforge.net>
+
+proc InitTheme {} {
+	# Create a color label style (bg can be overridden by widget option)
+	ttk::style layout Color.TLabel {
+		Color.Label.border -sticky news -children {
+			Color.label.fill -sticky news -children {
+				Color.Label.padding -sticky news -children {
+					Color.Label.label -sticky news}}}}
+	eval [linsert [ttk::style configure TLabel] 0 \
+			  ttk::style configure Color.TLabel]
+	ttk::style configure Color.TLabel \
+		-borderwidth 0 -relief flat -padding 2
+	ttk::style map Color.TLabel -background {{} gold}
+	# We also need a padded label.
+	ttk::style configure Padded.TLabel \
+		-padding {5 5} -borderwidth 1 -relief solid
+	# We need a gold frame.
+	ttk::style layout Gold.TFrame {
+		Gold.Frame.border -sticky nswe -children {
+			Gold.Frame.fill -sticky nswe}}
+	ttk::style configure Gold.TFrame -background gold -relief flat
+	# listboxes should have a theme border so embed in ttk::frame
+	ttk::style layout SListbox.TFrame {
+        SListbox.Frame.Entry.field -sticky news -border true -children {
+            SListbox.Frame.padding -sticky news
+        }
+    }
+}
+
+proc gold_frame {w args} {
+	global use_ttk
+	if {$use_ttk} {
+		eval [linsert $args 0 ttk::frame $w -style Gold.TFrame]
+	} else {
+		eval [linsert $args 0 frame $w -background gold]
+	}
+}
+
+proc tlabel {w args} {
+	global use_ttk
+	if {$use_ttk} {
+		set cmd [list ttk::label $w -style Color.TLabel]
+		foreach {k v} $args {
+			switch -glob -- $k {
+				-activebackground {}
+				default { lappend cmd $k $v }
+			}
+		}
+		eval $cmd
+	} else {
+		eval [linsert $args 0 label $w]
+	}
+}
+
+# The padded label gets used in the about class.
+proc paddedlabel {w args} {
+	global use_ttk
+	if {$use_ttk} {
+		eval [linsert $args 0 ttk::label $w -style Padded.TLabel]
+	} else {
+		eval [linsert $args 0 label $w \
+				  -padx 5 -pady 5 \
+				  -justify left \
+				  -anchor w \
+				  -borderwidth 1 \
+				  -relief solid]
+	}
+}
+
+# Create a toplevel for use as a dialog.
+# If available, sets the EWMH dialog hint and if ttk is enabled
+# place a themed frame over the surface.
+proc Dialog {w args} {
+	eval [linsert $args 0 toplevel $w -class Dialog]
+	pave_toplevel $w
+	return $w
+}
+
+# Tk toplevels are not themed - so pave it over with a themed frame to get
+# the base color correct per theme.
+proc pave_toplevel {w} {
+	global use_ttk
+	if {$use_ttk && ![winfo exists $w.!paving]} {
+		set paving [ttk::frame $w.!paving]
+		place $paving -x 0 -y 0 -relwidth 1 -relheight 1
+		lower $paving
+	}
+}
+
+# Create a scrolled listbox with appropriate border for the current theme.
+# On many themes the border for a scrolled listbox needs to go around the
+# listbox and the scrollbar.
+proc slistbox {w args} {
+	global use_ttk NS
+	if {$use_ttk} {
+		set f [ttk::frame $w -style SListbox.TFrame -padding 2]
+	} else {
+		set f [frame $w -relief flat]
+	}
+    if {[catch {
+		if {$use_ttk} {
+			eval [linsert $args 0 listbox $f.list -relief flat \
+					  -highlightthickness 0 -borderwidth 0]
+		} else {
+			eval [linsert $args 0 listbox $f.list]
+		}
+        ${NS}::scrollbar $f.vs -command [list $f.list yview]
+        $f.list configure -yscrollcommand [list $f.vs set]
+        grid $f.list $f.vs -sticky news
+        grid rowconfigure $f 0 -weight 1
+        grid columnconfigure $f 0 -weight 1
+		bind $f.list <<ListboxSelect>> \
+			[list event generate $w <<ListboxSelect>>]
+        interp hide {} $w
+        interp alias {} $w {} $f.list
+    } err]} {
+        destroy $f
+        return -code error $err
+    }
+    return $w
+}
+
+# fetch the background color from a widget.
+proc get_bg_color {w} {
+	global use_ttk
+	if {$use_ttk} {
+		set bg [ttk::style lookup [winfo class $w] -background]
+	} else {
+		set bg [$w cget -background]
+	}
+	return $bg
+}
+
+# ttk::spinbox didn't get added until 8.6
+proc tspinbox {w args} {
+	global use_ttk
+	if {$use_ttk && [llength [info commands ttk::spinbox]] > 0} {
+		eval [linsert $args 0 ttk::spinbox $w]
+	} else {
+		eval [linsert $args 0 spinbox $w]
+	}
+}
+
+# Tk 8.6 provides a standard font selection dialog. This uses the native
+# dialogs on Windows and MacOSX or a standard Tk dialog on X11.
+proc tchoosefont {w title familyvar sizevar} {
+	if {[package vsatisfies [package provide Tk] 8.6]} {
+		upvar #0 $familyvar family
+		upvar #0 $sizevar size
+		tk fontchooser configure -parent $w -title $title \
+			-font [list $family $size] \
+			-command [list on_choosefont $familyvar $sizevar]
+		tk fontchooser show
+	} else {
+		choose_font::pick $w $title $familyvar $sizevar
+	}
+}
+
+# Called when the Tk 8.6 fontchooser selects a font.
+proc on_choosefont {familyvar sizevar font} {
+	upvar #0 $familyvar family
+	upvar #0 $sizevar size
+	set font [font actual $font]
+	set family [dict get $font -family]
+	set size [dict get $font -size]
+}
+
+# Local variables:
+# mode: tcl
+# indent-tabs-mode: t
+# tab-width: 4
+# End:
diff --git a/lib/tools_dlg.tcl b/lib/tools_dlg.tcl
index 5f7f08e..7eeda9d 100644
--- a/lib/tools_dlg.tcl
+++ b/lib/tools_dlg.tcl
@@ -16,53 +16,50 @@ field ask_branch    0; # ask for a revision
 field ask_args      0; # ask for additional args
 
 constructor dialog {} {
-	global repo_config
+	global repo_config use_ttk NS
 
-	make_toplevel top w
+	make_dialog top w
 	wm title $top [append "[appname] ([reponame]): " [mc "Add Tool"]]
 	if {$top ne {.}} {
 		wm geometry $top "+[winfo rootx .]+[winfo rooty .]"
 		wm transient $top .
 	}
 
-	label $w.header -text [mc "Add New Tool Command"] -font font_uibold
+	${NS}::label $w.header -text [mc "Add New Tool Command"] \
+		-font font_uibold -anchor center
 	pack $w.header -side top -fill x
 
-	frame $w.buttons
-	checkbutton $w.buttons.global \
+	${NS}::frame $w.buttons
+	${NS}::checkbutton $w.buttons.global \
 		-text [mc "Add globally"] \
 		-variable @add_global
 	pack $w.buttons.global -side left -padx 5
-	button $w.buttons.create -text [mc Add] \
+	${NS}::button $w.buttons.create -text [mc Add] \
 		-default active \
 		-command [cb _add]
 	pack $w.buttons.create -side right
-	button $w.buttons.cancel -text [mc Cancel] \
+	${NS}::button $w.buttons.cancel -text [mc Cancel] \
 		-command [list destroy $w]
 	pack $w.buttons.cancel -side right -padx 5
 	pack $w.buttons -side bottom -fill x -pady 10 -padx 10
 
-	labelframe $w.desc -text [mc "Tool Details"]
+	${NS}::labelframe $w.desc -text [mc "Tool Details"]
 
-	label $w.desc.name_cmnt -anchor w\
+	${NS}::label $w.desc.name_cmnt -anchor w\
 		-text [mc "Use '/' separators to create a submenu tree:"]
 	grid x $w.desc.name_cmnt -sticky we -padx {0 5} -pady {0 2}
-	label $w.desc.name_l -text [mc "Name:"]
+	${NS}::label $w.desc.name_l -text [mc "Name:"]
 	set w_name $w.desc.name_t
-	entry $w_name \
-		-borderwidth 1 \
-		-relief sunken \
+	${NS}::entry $w_name \
 		-width 40 \
 		-textvariable @name \
 		-validate key \
 		-validatecommand [cb _validate_name %d %S]
 	grid $w.desc.name_l $w_name -sticky we -padx {0 5}
 
-	label $w.desc.cmd_l -text [mc "Command:"]
+	${NS}::label $w.desc.cmd_l -text [mc "Command:"]
 	set w_cmd $w.desc.cmd_t
-	entry $w_cmd \
-		-borderwidth 1 \
-		-relief sunken \
+	${NS}::entry $w_cmd \
 		-width 40 \
 		-textvariable @command
 	grid $w.desc.cmd_l $w_cmd -sticky we -padx {0 5} -pady {0 3}
@@ -70,30 +67,30 @@ constructor dialog {} {
 	grid columnconfigure $w.desc 1 -weight 1
 	pack $w.desc -anchor nw -fill x -pady 5 -padx 5
 
-	checkbutton $w.confirm \
+	${NS}::checkbutton $w.confirm \
 		-text [mc "Show a dialog before running"] \
 		-variable @confirm -command [cb _check_enable_dlg]
 
-	labelframe $w.dlg -labelwidget $w.confirm
+	${NS}::labelframe $w.dlg -labelwidget $w.confirm
 
-	checkbutton $w.dlg.askbranch \
+	${NS}::checkbutton $w.dlg.askbranch \
 		-text [mc "Ask the user to select a revision (sets \$REVISION)"] \
 		-variable @ask_branch -state disabled
 	pack $w.dlg.askbranch -anchor w -padx 15
 
-	checkbutton $w.dlg.askargs \
+	${NS}::checkbutton $w.dlg.askargs \
 		-text [mc "Ask the user for additional arguments (sets \$ARGS)"] \
 		-variable @ask_args -state disabled
 	pack $w.dlg.askargs -anchor w -padx 15
 
 	pack $w.dlg -anchor nw -fill x -pady {0 8} -padx 5
 
-	checkbutton $w.noconsole \
+	${NS}::checkbutton $w.noconsole \
 		-text [mc "Don't show the command output window"] \
 		-variable @no_console
 	pack $w.noconsole -anchor w -padx 5
 
-	checkbutton $w.needsfile \
+	${NS}::checkbutton $w.needsfile \
 		-text [mc "Run only if a diff is selected (\$FILENAME not empty)"] \
 		-variable @needs_file
 	pack $w.needsfile -anchor w -padx 5
@@ -182,40 +179,38 @@ field w              ; # widget path
 field w_names        ; # name list
 
 constructor dialog {} {
-	global repo_config global_config system_config
+	global repo_config global_config system_config use_ttk NS
 
 	load_config 1
 
-	make_toplevel top w
+	make_dialog top w
 	wm title $top [append "[appname] ([reponame]): " [mc "Remove Tool"]]
 	if {$top ne {.}} {
 		wm geometry $top "+[winfo rootx .]+[winfo rooty .]"
 		wm transient $top .
 	}
 
-	label $w.header -text [mc "Remove Tool Commands"] -font font_uibold
+	${NS}::label $w.header -text [mc "Remove Tool Commands"] \
+		-font font_uibold -anchor center
 	pack $w.header -side top -fill x
 
-	frame $w.buttons
-	button $w.buttons.create -text [mc Remove] \
+	${NS}::frame $w.buttons
+	${NS}::button $w.buttons.create -text [mc Remove] \
 		-default active \
 		-command [cb _remove]
 	pack $w.buttons.create -side right
-	button $w.buttons.cancel -text [mc Cancel] \
+	${NS}::button $w.buttons.cancel -text [mc Cancel] \
 		-command [list destroy $w]
 	pack $w.buttons.cancel -side right -padx 5
 	pack $w.buttons -side bottom -fill x -pady 10 -padx 10
 
-	frame $w.list
+	${NS}::frame $w.list
 	set w_names $w.list.l
-	listbox $w_names \
+	slistbox $w_names \
 		-height 10 \
 		-width 30 \
 		-selectmode extended \
-		-exportselection false \
-		-yscrollcommand [list $w.list.sby set]
-	scrollbar $w.list.sby -command [list $w.list.l yview]
-	pack $w.list.sby -side right -fill y
+		-exportselection false
 	pack $w.list.l -side left -fill both -expand 1
 	pack $w.list -fill both -expand 1 -pady 5 -padx 5
 
@@ -232,7 +227,7 @@ constructor dialog {} {
 	}
 
 	if {$local_cnt > 0} {
-		label $w.colorlbl -foreground blue \
+		${NS}::label $w.colorlbl -foreground blue \
 			-text [mc "(Blue denotes repository-local tools)"]
 		pack $w.colorlbl -fill x -pady 5 -padx 5
 	}
@@ -277,14 +272,14 @@ field is_ok         0; # ok to start
 field argstr       {}; # arguments
 
 constructor dialog {fullname} {
-	global M1B
+	global M1B use_ttk NS
 
 	set title [get_config "guitool.$fullname.title"]
 	if {$title eq {}} {
 		regsub {/} $fullname { / } title
 	}
 
-	make_toplevel top w -autodelete 0
+	make_dialog top w -autodelete 0
 	wm title $top [append "[appname] ([reponame]): " $title]
 	if {$top ne {.}} {
 		wm geometry $top "+[winfo rootx .]+[winfo rooty .]"
@@ -297,7 +292,7 @@ constructor dialog {fullname} {
 		set prompt [mc "Run Command: %s" $command]
 	}
 
-	label $w.header -text $prompt -font font_uibold
+	${NS}::label $w.header -text $prompt -font font_uibold -anchor center
 	pack $w.header -side top -fill x
 
 	set argprompt [get_config "guitool.$fullname.argprompt"]
@@ -311,12 +306,10 @@ constructor dialog {fullname} {
 			set argprompt [mc "Arguments"]
 		}
 
-		labelframe $w.arg -text $argprompt
+		${NS}::labelframe $w.arg -text $argprompt
 
 		set w_args $w.arg.txt
-		entry $w_args \
-			-borderwidth 1 \
-			-relief sunken \
+		${NS}::entry $w_args \
 			-width 40 \
 			-textvariable @argstr
 		pack $w_args -padx 5 -pady 5 -fill both
@@ -337,18 +330,18 @@ constructor dialog {fullname} {
 		pack $w.rev -anchor nw -fill both -expand 1 -pady 5 -padx 5
 	}
 
-	frame $w.buttons
+	${NS}::frame $w.buttons
 	if {$is_ask_revs} {
-		button $w.buttons.visualize \
+		${NS}::button $w.buttons.visualize \
 			-text [mc Visualize] \
 			-command [cb _visualize]
 		pack $w.buttons.visualize -side left
 	}
-	button $w.buttons.ok \
+	${NS}::button $w.buttons.ok \
 		-text [mc OK] \
 		-command [cb _start]
 	pack $w.buttons.ok -side right
-	button $w.buttons.cancel \
+	${NS}::button $w.buttons.cancel \
 		-text [mc "Cancel"] \
 		-command [cb _cancel]
 	pack $w.buttons.cancel -side right -padx 5
diff --git a/lib/transport.tcl b/lib/transport.tcl
index b18d9c7..60e3a64 100644
--- a/lib/transport.tcl
+++ b/lib/transport.tcl
@@ -91,50 +91,55 @@ trace add variable push_remote write \
 proc do_push_anywhere {} {
 	global all_remotes current_branch
 	global push_urltype push_remote push_url push_thin push_tags
-	global push_force
+	global push_force use_ttk NS
 
 	set w .push_setup
 	toplevel $w
+	wm withdraw $w
 	wm geometry $w "+[winfo rootx .]+[winfo rooty .]"
+	pave_toplevel $w
 
-	label $w.header -text [mc "Push Branches"] -font font_uibold
+	${NS}::label $w.header -text [mc "Push Branches"] \
+		-font font_uibold -anchor center
 	pack $w.header -side top -fill x
 
-	frame $w.buttons
-	button $w.buttons.create -text [mc Push] \
+	${NS}::frame $w.buttons
+	${NS}::button $w.buttons.create -text [mc Push] \
 		-default active \
 		-command [list start_push_anywhere_action $w]
 	pack $w.buttons.create -side right
-	button $w.buttons.cancel -text [mc "Cancel"] \
+	${NS}::button $w.buttons.cancel -text [mc "Cancel"] \
 		-default normal \
 		-command [list destroy $w]
 	pack $w.buttons.cancel -side right -padx 5
 	pack $w.buttons -side bottom -fill x -pady 10 -padx 10
 
-	labelframe $w.source -text [mc "Source Branches"]
-	listbox $w.source.l \
+	${NS}::labelframe $w.source -text [mc "Source Branches"]
+	slistbox $w.source.l \
 		-height 10 \
 		-width 70 \
-		-selectmode extended \
-		-yscrollcommand [list $w.source.sby set]
+		-selectmode extended
 	foreach h [load_all_heads] {
 		$w.source.l insert end $h
 		if {$h eq $current_branch} {
 			$w.source.l select set end
 		}
 	}
-	scrollbar $w.source.sby -command [list $w.source.l yview]
-	pack $w.source.sby -side right -fill y
 	pack $w.source.l -side left -fill both -expand 1
 	pack $w.source -fill both -expand 1 -pady 5 -padx 5
 
-	labelframe $w.dest -text [mc "Destination Repository"]
+	${NS}::labelframe $w.dest -text [mc "Destination Repository"]
 	if {$all_remotes ne {}} {
-		radiobutton $w.dest.remote_r \
+		${NS}::radiobutton $w.dest.remote_r \
 			-text [mc "Remote:"] \
 			-value remote \
 			-variable push_urltype
-		eval tk_optionMenu $w.dest.remote_m push_remote $all_remotes
+		if {$use_ttk} {
+			ttk::combobox $w.dest.remote_m -textvariable push_remote \
+				-values $all_remotes
+		} else {
+			eval tk_optionMenu $w.dest.remote_m push_remote $all_remotes
+		}
 		grid $w.dest.remote_r $w.dest.remote_m -sticky w
 		if {[lsearch -sorted -exact $all_remotes origin] != -1} {
 			set push_remote origin
@@ -145,13 +150,11 @@ proc do_push_anywhere {} {
 	} else {
 		set push_urltype url
 	}
-	radiobutton $w.dest.url_r \
+	${NS}::radiobutton $w.dest.url_r \
 		-text [mc "Arbitrary Location:"] \
 		-value url \
 		-variable push_urltype
-	entry $w.dest.url_t \
-		-borderwidth 1 \
-		-relief sunken \
+	${NS}::entry $w.dest.url_t \
 		-width 50 \
 		-textvariable push_url \
 		-validate key \
@@ -166,16 +169,16 @@ proc do_push_anywhere {} {
 	grid columnconfigure $w.dest 1 -weight 1
 	pack $w.dest -anchor nw -fill x -pady 5 -padx 5
 
-	labelframe $w.options -text [mc "Transfer Options"]
-	checkbutton $w.options.force \
+	${NS}::labelframe $w.options -text [mc "Transfer Options"]
+	${NS}::checkbutton $w.options.force \
 		-text [mc "Force overwrite existing branch (may discard changes)"] \
 		-variable push_force
 	grid $w.options.force -columnspan 2 -sticky w
-	checkbutton $w.options.thin \
+	${NS}::checkbutton $w.options.thin \
 		-text [mc "Use thin pack (for slow network connections)"] \
 		-variable push_thin
 	grid $w.options.thin -columnspan 2 -sticky w
-	checkbutton $w.options.tags \
+	${NS}::checkbutton $w.options.tags \
 		-text [mc "Include tags"] \
 		-variable push_tags
 	grid $w.options.tags -columnspan 2 -sticky w
@@ -191,5 +194,6 @@ proc do_push_anywhere {} {
 	bind $w <Key-Escape> "destroy $w"
 	bind $w <Key-Return> [list start_push_anywhere_action $w]
 	wm title $w [append "[appname] ([reponame]): " [mc "Push"]]
+	wm deiconify $w
 	tkwait window $w
 }
-- 
1.6.6

^ permalink raw reply related

* Re: [PATCH] t5541-http-push: make grep expression check for one line  only
From: Tay Ray Chuan @ 2010-01-26  1:33 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <7v1vhe3wdz.fsf@alter.siamese.dyndns.org>

Hi,

On Tue, Jan 26, 2010 at 1:52 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Tay Ray Chuan <rctay89@gmail.com> writes:
>>
>>> Simplify the grep expressions in the non-fast-forward tests to check
>>> only for the first line of the non-fast-forward warning - having that
>>> line should be enough assurance that the full warning is printed.
>>>
>>> In the first place, grep can't deal with expressions for multiple
>>> lines.
>>
>> This shows that nobody has ever run this test since January 8th, not even
>> the original author?
>>
>> Hmmm.
>
> Actually, if you grep with a pattern with multiple lines, it is equivalent
> to giving each of these lines as a separate pattern from the command line.
> So it is understandable that the tests passed.  They were checking if
> these match, but it doesn't check (and grep is not designed to) if the
> first pattern matched the first line, the second to second, etc.

that was why the tests passed then and passed now - I just happened to
run them, and I noticed grep reported success, yet only matched 2 of
the 3 lines. Then I remembered recently there had been a rewording of
the fast-forward warning was reworded (c0eb604 "push: spell 'Note
about fast-forwards'")...

> So I'd say something like...
>
>    Don't feed multiple-line pattern to grep and expect them to match with
>    lines in order.
>
>    Simplify the grep expressions in the non-fast-forward tests to check
>    only for the first line of the non-fast-forward warning - having that
>    line should be enough assurance that the full warning is printed.
>
>    Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
>
> instead.

No problem.

-- 
Cheers,
Ray Chuan

^ permalink raw reply

* Re: [PATCH v4] Threaded grep
From: Junio C Hamano @ 2010-01-26  1:20 UTC (permalink / raw)
  To: Fredrik Kuivinen; +Cc: git, Linus Torvalds, Johannes Sixt
In-Reply-To: <20100125225139.GA3048@fredrik-laptop>

Fredrik Kuivinen <frekui@gmail.com> writes:

> The patch has been rebased on top of next.
>
> Additional changes since v3:
>
> * Fix some issues with Git's native pthreads implementation on
>   Windows (pthread_cond_broadcast is still used though).
> * Fix style issues.
> * When greping in a tree, allocate memory for the data buffer as
>   late as possible.
> * Return void from grep_sha1_async and grep_file_async instead of
>   always returning 0.

Thanks; I've fixed up a few old-style declaration header and queued the
result to 'pu'.

^ permalink raw reply

* Re: [PATCHv3 0/3] Fix patch detection for thunderbird saved mails
From: Junio C Hamano @ 2010-01-26  1:19 UTC (permalink / raw)
  To: Stephen Boyd; +Cc: git, Eric Blake
In-Reply-To: <1264466039-22431-1-git-send-email-bebarino@gmail.com>

Looks sensible; thanks.

^ permalink raw reply

* Re: What's cooking in git.git (Jan 2010, #08; Sun, 24)
From: Jakub Narebski @ 2010-01-26  0:59 UTC (permalink / raw)
  To: J.H.; +Cc: Petr Baudis, Junio C Hamano, git, J.H.
In-Reply-To: <4B5E357A.50607@kernel.org>

On Mon, 25 Jan 2010 at 16:21:51 -0800 (PST), J.H. wrote:
> >
> > I am waiting for response from (I guess busy) J.H.; I can do the re-roll
> > if he is too busy to work on it.
> 
> I'm juggling about 4 things in the air right now, with gitweb being one 
> of them, and two of them involving external entities to kernel.org.  
> It's good news all told, just requires a fair amount of my time 
> (currently) and babysitting.  I've been digging through the patches 
> you've got right now.  I should really just pull them in from your git 
> tree again.

Note however that my series (gitweb/cache-kernel branch) is based on
gitweb-ml-v2, not on newer gitweb-ml-v5.

> After reading through some of the discussions today I've had some more 
> ideas on the caching stuff, they are written up on my whiteboard and I 
> might be able to get to shortly, but I've got a rather large wall 
> looming ahead of me that's going to chew up a *LOT* (read at least a 
> month, if not two) of my time, so I'm a bit under the gun to try and get 
> this as far as I can before I hit that wall or this is going to fall by 
> the wayside again till I've got time again.

I think the best way of utilizing your time would be for you to take
active part in discussion, especially in what git.kernel.org needs and why.

I can clean up the miscellaneous improvements parts myself, and re-roll
caching patches.

> > > For the main caching patch, it seems like good idea to take Jakub's
> > > split-up series instead, let's see what is J.H.'s opinion on the series?
> > >     
> >
> > Let me at least make them into proper patches, with commit messages and 
> > configureability at least of the original caching patch by J.H.
> >
> > Also the question whether to create 'print -> print $out' patch, or to
> > manipulate *STDOUT instead must be solved, I think, before applying
> > those patches... well, at least beyond 'pu'.
> >
> > I am waiting for promised J.H. comments, when he will have time for it...

-- 
Jakub Narebski
Poland

^ permalink raw reply

* [PATCHv3 3/3] am: fix patch format detection for Thunderbird "Save As" emails
From: Stephen Boyd @ 2010-01-26  0:33 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Eric Blake
In-Reply-To: <1264466039-22431-1-git-send-email-bebarino@gmail.com>

The patch detection wants to inspect all the headers of a rfc2822 message
and ensure that they look like header fields. The headers are always
separated from the message body with a blank line. When Thunderbird saves
the message the blank line separating the headers from the body includes a
CR. The patch detection is failing because a CRLF doesn't match /^$/. Fix
this by allowing a CR to exist on the separating line.

Signed-off-by: Stephen Boyd <bebarino@gmail.com>
---

I gave up trying to make it portable and now just use append_cr() from
test-lib. Hopefully this is a much better and simpler approach.

 git-am.sh     |    3 ++-
 t/t4150-am.sh |   15 +++++++++++++++
 2 files changed, 17 insertions(+), 1 deletions(-)

diff --git a/git-am.sh b/git-am.sh
index 2f46fda..c8b9cbb 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -205,7 +205,8 @@ check_patch_format () {
 			# discarding the indented remainder of folded lines,
 			# and see if it looks like that they all begin with the
 			# header field names...
-			sed -n -e '/^$/q' -e '/^[ 	]/d' -e p "$1" |
+			tr -d '\015' <"$1" |
+			sed -n -e '/^$/q' -e '/^[ 	]/d' -e p |
 			sane_egrep -v '^[!-9;-~]+:' >/dev/null ||
 			patch_format=mbox
 		fi
diff --git a/t/t4150-am.sh b/t/t4150-am.sh
index 8296605..a034918 100755
--- a/t/t4150-am.sh
+++ b/t/t4150-am.sh
@@ -83,6 +83,12 @@ test_expect_success setup '
 		echo "X-Fake-Field: Line Three" &&
 		git format-patch --stdout first | sed -e "1d"
 	} > patch1.eml &&
+	{
+		echo "X-Fake-Field: Line One" &&
+		echo "X-Fake-Field: Line Two" &&
+		echo "X-Fake-Field: Line Three" &&
+		git format-patch --stdout first | sed -e "1d"
+	} | append_cr >patch1-crlf.eml &&
 	sed -n -e "3,\$p" msg >file &&
 	git add file &&
 	test_tick &&
@@ -123,6 +129,15 @@ test_expect_success 'am applies patch e-mail not in a mbox' '
 	test "$(git rev-parse second^)" = "$(git rev-parse HEAD^)"
 '
 
+test_expect_success 'am applies patch e-mail not in a mbox with CRLF' '
+	git checkout first &&
+	git am patch1-crlf.eml &&
+	! test -d .git/rebase-apply &&
+	test -z "$(git diff second)" &&
+	test "$(git rev-parse second)" = "$(git rev-parse HEAD)" &&
+	test "$(git rev-parse second^)" = "$(git rev-parse HEAD^)"
+'
+
 GIT_AUTHOR_NAME="Another Thor"
 GIT_AUTHOR_EMAIL="a.thor@example.com"
 GIT_COMMITTER_NAME="Co M Miter"
-- 
1.7.0.rc0

^ permalink raw reply related

* [PATCHv3 2/3] t0022: replace non-portable literal CR
From: Stephen Boyd @ 2010-01-26  0:33 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Eric Blake
In-Reply-To: <1264466039-22431-1-git-send-email-bebarino@gmail.com>

We shouldn't have literal CR's in tests as they aren't portable.

Signed-off-by: Stephen Boyd <bebarino@gmail.com>
---

Hopefully this isn't botched over the wire.

 t/t0022-crlf-rename.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/t/t0022-crlf-rename.sh b/t/t0022-crlf-rename.sh
index f1e1d48..7af3fbc 100755
--- a/t/t0022-crlf-rename.sh
+++ b/t/t0022-crlf-rename.sh
@@ -12,7 +12,7 @@ test_expect_success setup '
 	test_tick &&
 	git commit -m Initial &&
 
-	sed -e "s/\$/
/" "$TEST_DIRECTORY"/t0022-crlf-rename.sh >elpmas &&
+	append_cr <"$TEST_DIRECTORY"/t0022-crlf-rename.sh >elpmas &&
 	git add elpmas &&
 	rm -f sample &&
 
-- 
1.7.0.rc0

^ permalink raw reply related

* [PATCHv3 0/3] Fix patch detection for thunderbird saved mails
From: Stephen Boyd @ 2010-01-26  0:33 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Eric Blake

The original patch for this is a bit old (~1 month) but I'd like to see it
make it in. The first two patches are cleanup before the original patch in
the third spot. Now the test in patch 3 uses append_cr() instead of a hand
crafted version.

Stephen Boyd (3):
  tests: consolidate CR removal/addition functions
  t0022: replace non-portable literal CR
  am: fix patch format detection for Thunderbird "Save As" emails

 git-am.sh                     |    3 +-
 t/t0020-crlf.sh               |   55 +++++++++++++++-------------------------
 t/t0022-crlf-rename.sh        |    2 +-
 t/t3400-rebase.sh             |    4 ---
 t/t4012-diff-binary.sh        |    4 ---
 t/t4150-am.sh                 |   15 +++++++++++
 t/t6033-merge-crlf.sh         |    8 ------
 t/t9401-git-cvsserver-crlf.sh |    8 ------
 t/test-lib.sh                 |   16 ++++++++++++
 9 files changed, 55 insertions(+), 60 deletions(-)

^ permalink raw reply

* [PATCHv3 1/3] tests: consolidate CR removal/addition functions
From: Stephen Boyd @ 2010-01-26  0:33 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Eric Blake
In-Reply-To: <1264466039-22431-1-git-send-email-bebarino@gmail.com>

append_cr(), remove_cr(), q_to_nul() and q_to_cr() are defined in multiple
tests. Consolidate them into test-lib.sh so we can stop redefining them.
The use of remove_cr() in t0020 to test for a CR is replaced with a new
function has_cr() to accurately reflect what is intended (the output of
remove_cr() was being thrown away).

Signed-off-by: Stephen Boyd <bebarino@gmail.com>
---
 t/t0020-crlf.sh               |   55 +++++++++++++++-------------------------
 t/t3400-rebase.sh             |    4 ---
 t/t4012-diff-binary.sh        |    4 ---
 t/t6033-merge-crlf.sh         |    8 ------
 t/t9401-git-cvsserver-crlf.sh |    8 ------
 t/test-lib.sh                 |   16 ++++++++++++
 6 files changed, 37 insertions(+), 58 deletions(-)

diff --git a/t/t0020-crlf.sh b/t/t0020-crlf.sh
index 4e72b53..c3e7e32 100755
--- a/t/t0020-crlf.sh
+++ b/t/t0020-crlf.sh
@@ -4,21 +4,8 @@ test_description='CRLF conversion'
 
 . ./test-lib.sh
 
-q_to_nul () {
-	perl -pe 'y/Q/\000/'
-}
-
-q_to_cr () {
-	tr Q '\015'
-}
-
-append_cr () {
-	sed -e 's/$/Q/' | tr Q '\015'
-}
-
-remove_cr () {
-	tr '\015' Q <"$1" | grep Q >/dev/null &&
-	tr '\015' Q <"$1" | sed -ne 's/Q$//p'
+has_cr() {
+	tr '\015' Q <"$1" | grep Q >/dev/null
 }
 
 test_expect_success setup '
@@ -156,7 +143,7 @@ test_expect_success 'checkout with autocrlf=true' '
 
 	for f in one dir/two
 	do
-		remove_cr "$f" >tmp && mv -f tmp $f &&
+		remove_cr <"$f" >tmp && mv -f tmp $f &&
 		git update-index -- $f || {
 			echo "Eh? $f"
 			false
@@ -180,7 +167,7 @@ test_expect_success 'checkout with autocrlf=input' '
 
 	for f in one dir/two
 	do
-		if remove_cr "$f" >/dev/null
+		if has_cr "$f"
 		then
 			echo "Eh? $f"
 			false
@@ -245,7 +232,7 @@ test_expect_success 'apply patch (autocrlf=true)' '
 	git read-tree --reset -u HEAD &&
 
 	git apply patch.file &&
-	test "$patched" = "`remove_cr one | git hash-object --stdin`" || {
+	test "$patched" = "`remove_cr <one | git hash-object --stdin`" || {
 		echo "Eh?  apply without index"
 		false
 	}
@@ -272,7 +259,7 @@ test_expect_success 'apply patch --index (autocrlf=true)' '
 
 	git apply --index patch.file &&
 	test "$patched" = `git rev-parse :one` &&
-	test "$patched" = "`remove_cr one | git hash-object --stdin`" || {
+	test "$patched" = "`remove_cr <one | git hash-object --stdin`" || {
 		echo "Eh?  apply with --index"
 		false
 	}
@@ -285,7 +272,7 @@ test_expect_success '.gitattributes says two is binary' '
 	git config core.autocrlf true &&
 	git read-tree --reset -u HEAD &&
 
-	if remove_cr dir/two >/dev/null
+	if has_cr dir/two
 	then
 		echo "Huh?"
 		false
@@ -293,7 +280,7 @@ test_expect_success '.gitattributes says two is binary' '
 		: happy
 	fi &&
 
-	if remove_cr one >/dev/null
+	if has_cr one
 	then
 		: happy
 	else
@@ -301,7 +288,7 @@ test_expect_success '.gitattributes says two is binary' '
 		false
 	fi &&
 
-	if remove_cr three >/dev/null
+	if has_cr three
 	then
 		echo "Huh?"
 		false
@@ -316,7 +303,7 @@ test_expect_success '.gitattributes says two is input' '
 	echo "two crlf=input" >.gitattributes &&
 	git read-tree --reset -u HEAD &&
 
-	if remove_cr dir/two >/dev/null
+	if has_cr dir/two
 	then
 		echo "Huh?"
 		false
@@ -331,7 +318,7 @@ test_expect_success '.gitattributes says two and three are text' '
 	echo "t* crlf" >.gitattributes &&
 	git read-tree --reset -u HEAD &&
 
-	if remove_cr dir/two >/dev/null
+	if has_cr dir/two
 	then
 		: happy
 	else
@@ -339,7 +326,7 @@ test_expect_success '.gitattributes says two and three are text' '
 		false
 	fi &&
 
-	if remove_cr three >/dev/null
+	if has_cr three
 	then
 		: happy
 	else
@@ -357,14 +344,14 @@ test_expect_success 'in-tree .gitattributes (1)' '
 	rm -rf tmp one dir .gitattributes patch.file three &&
 	git read-tree --reset -u HEAD &&
 
-	if remove_cr one >/dev/null
+	if has_cr one
 	then
 		echo "Eh? one should not have CRLF"
 		false
 	else
 		: happy
 	fi &&
-	remove_cr three >/dev/null || {
+	has_cr three || {
 		echo "Eh? three should still have CRLF"
 		false
 	}
@@ -376,14 +363,14 @@ test_expect_success 'in-tree .gitattributes (2)' '
 	git read-tree --reset HEAD &&
 	git checkout-index -f -q -u -a &&
 
-	if remove_cr one >/dev/null
+	if has_cr one
 	then
 		echo "Eh? one should not have CRLF"
 		false
 	else
 		: happy
 	fi &&
-	remove_cr three >/dev/null || {
+	has_cr three || {
 		echo "Eh? three should still have CRLF"
 		false
 	}
@@ -396,14 +383,14 @@ test_expect_success 'in-tree .gitattributes (3)' '
 	git checkout-index -u .gitattributes &&
 	git checkout-index -u one dir/two three &&
 
-	if remove_cr one >/dev/null
+	if has_cr one
 	then
 		echo "Eh? one should not have CRLF"
 		false
 	else
 		: happy
 	fi &&
-	remove_cr three >/dev/null || {
+	has_cr three || {
 		echo "Eh? three should still have CRLF"
 		false
 	}
@@ -416,14 +403,14 @@ test_expect_success 'in-tree .gitattributes (4)' '
 	git checkout-index -u one dir/two three &&
 	git checkout-index -u .gitattributes &&
 
-	if remove_cr one >/dev/null
+	if has_cr one
 	then
 		echo "Eh? one should not have CRLF"
 		false
 	else
 		: happy
 	fi &&
-	remove_cr three >/dev/null || {
+	has_cr three || {
 		echo "Eh? three should still have CRLF"
 		false
 	}
@@ -456,7 +443,7 @@ test_expect_success 'checkout when deleting .gitattributes' '
 
 	git checkout master~1 &&
 	git checkout master &&
-	remove_cr .file2 >/dev/null
+	has_cr .file2
 
 '
 
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index 4e6a44b..4314ad2 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -134,10 +134,6 @@ test_expect_success 'rebase -q is quiet' '
      test ! -s output.out
 '
 
-q_to_cr () {
-	tr Q '\015'
-}
-
 test_expect_success 'Rebase a commit that sprinkles CRs in' '
 	(
 		echo "One"
diff --git a/t/t4012-diff-binary.sh b/t/t4012-diff-binary.sh
index f64aa48..bc46563 100755
--- a/t/t4012-diff-binary.sh
+++ b/t/t4012-diff-binary.sh
@@ -77,10 +77,6 @@ test_expect_success 'apply binary patch' \
 	 tree1=`git write-tree` &&
 	 test "$tree1" = "$tree0"'
 
-q_to_nul() {
-	perl -pe 'y/Q/\000/'
-}
-
 nul_to_q() {
 	perl -pe 'y/\000/Q/'
 }
diff --git a/t/t6033-merge-crlf.sh b/t/t6033-merge-crlf.sh
index 75d9602..e8d65ee 100755
--- a/t/t6033-merge-crlf.sh
+++ b/t/t6033-merge-crlf.sh
@@ -1,13 +1,5 @@
 #!/bin/sh
 
-append_cr () {
-	sed -e 's/$/Q/' | tr Q '\015'
-}
-
-remove_cr () {
-	tr '\015' Q | sed -e 's/Q$//'
-}
-
 test_description='merge conflict in crlf repo
 
 		b---M
diff --git a/t/t9401-git-cvsserver-crlf.sh b/t/t9401-git-cvsserver-crlf.sh
index 40637d6..ed7b513 100755
--- a/t/t9401-git-cvsserver-crlf.sh
+++ b/t/t9401-git-cvsserver-crlf.sh
@@ -11,14 +11,6 @@ repository using cvs CLI client via git-cvsserver server'
 
 . ./test-lib.sh
 
-q_to_nul () {
-    perl -pe 'y/Q/\000/'
-}
-
-q_to_cr () {
-    tr Q '\015'
-}
-
 marked_as () {
     foundEntry="$(grep "^/$2/" "$1/CVS/Entries")"
     if [ x"$foundEntry" = x"" ] ; then
diff --git a/t/test-lib.sh b/t/test-lib.sh
index baa4093..afd3053 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -230,6 +230,22 @@ test_decode_color () {
 		-e 's/.\[m/<RESET>/g'
 }
 
+q_to_nul () {
+	perl -pe 'y/Q/\000/'
+}
+
+q_to_cr () {
+	tr Q '\015'
+}
+
+append_cr () {
+	sed -e 's/$/Q/' | tr Q '\015'
+}
+
+remove_cr () {
+	tr '\015' Q | sed -e 's/Q$//'
+}
+
 test_tick () {
 	if test -z "${test_tick+set}"
 	then
-- 
1.7.0.rc0

^ permalink raw reply related

* Re: What's cooking in git.git (Jan 2010, #08; Sun, 24)
From: J.H. @ 2010-01-26  0:21 UTC (permalink / raw)
  To: Jakub Narebski; +Cc: Petr Baudis, Junio C Hamano, git, J.H.
In-Reply-To: <201001260107.25796.jnareb@gmail.com>


>
> I am waiting for response from (I guess busy) J.H.; I can do the re-roll
> if he is too busy to work on it.
>
>   

I'm juggling about 4 things in the air right now, with gitweb being one 
of them, and two of them involving external entities to kernel.org.  
It's good news all told, just requires a fair amount of my time 
(currently) and babysitting.  I've been digging through the patches 
you've got right now.  I should really just pull them in from your git 
tree again.

After reading through some of the discussions today I've had some more 
ideas on the caching stuff, they are written up on my whiteboard and I 
might be able to get to shortly, but I've got a rather large wall 
looming ahead of me that's going to chew up a *LOT* (read at least a 
month, if not two) of my time, so I'm a bit under the gun to try and get 
this as far as I can before I hit that wall or this is going to fall by 
the wayside again till I've got time again.

>> For the main caching patch, it seems like good idea to take Jakub's
>> split-up series instead, let's see what is J.H.'s opinion on the series?
>>     
>
> Let me at least make them into proper patches, with commit messages and 
> configureability at least of the original caching patch by J.H.
>
> Also the question whether to create 'print -> print $out' patch, or to
> manipulate *STDOUT instead must be solved, I think, before applying
> those patches... well, at least beyond 'pu'.
>
> I am waiting for promised J.H. comments, when he will have time for it...
>   

^ permalink raw reply

* Re: What's cooking in git.git (Jan 2010, #08; Sun, 24)
From: Jakub Narebski @ 2010-01-26  0:07 UTC (permalink / raw)
  To: Petr Baudis; +Cc: Junio C Hamano, git, J.H., John 'Warthog9' Hawley
In-Reply-To: <20100125231241.GA4159@machine.or.cz>

On Tue, Jan 26, 2010 at 00:12 +0100, Petr Baudis wrote:
> On Mon, Jan 25, 2010 at 02:43:01PM -0800, Jakub Narebski wrote:

> > You are right that actual caching support is in flux.  The discussion
> > continues: it is very good that we have the voice from Pasky, too.
> 
> Unfortunately, I can't give much time to it, or actual contributions.
> I think it would be great to apply at least the miscellanous-changes
> part to (i) move this further (ii) give jh some positive feedback. :)
> There are ugly minor things, but they can be patched up later. I think
> it's healthier for new gitweb stuff to develop more in-tree, even
> if that means few angry users annoyed about less-than-perfect UI stuff,
> rather than polish the diamond in infinite discussions; worse is better!
> The only thing we should be worried about is avoiding introducing new
> bad URL interfaces since we should keep backwards compatibility there.

I think that while some patches from the miscellaneous-changes part are
good now (and very nice to have, especially load checking and 
gitweb/Makefile), there needs to be one last, final reroll of those
patches.

I am waiting for response from (I guess busy) J.H.; I can do the re-roll
if he is too busy to work on it.

> For the main caching patch, it seems like good idea to take Jakub's
> split-up series instead, let's see what is J.H.'s opinion on the series?

Let me at least make them into proper patches, with commit messages and 
configureability at least of the original caching patch by J.H.

Also the question whether to create 'print -> print $out' patch, or to
manipulate *STDOUT instead must be solved, I think, before applying
those patches... well, at least beyond 'pu'.

I am waiting for promised J.H. comments, when he will have time for it...
-- 
Jakub Narebski
Poland

^ permalink raw reply

* Re: [PATCH v4] Threaded grep
From: Linus Torvalds @ 2010-01-25 23:59 UTC (permalink / raw)
  To: Fredrik Kuivinen; +Cc: Git Mailing List, Junio C Hamano, Johannes Sixt
In-Reply-To: <20100125225139.GA3048@fredrik-laptop>



On Mon, 25 Jan 2010, Fredrik Kuivinen wrote:
> 
> The results below are best of five runs in the Linux repository (on a
> box with two cores).
> 
> git grep qwerty

Before:

	real	0m0.531s
	user	0m0.412s
	sys	0m0.112s

After:

	real	0m0.151s
	user	0m0.720s
	sys	0m0.272s


> $ /usr/bin/time git grep void

Before:

	real	0m1.144s
	user	0m0.988s
	sys	0m0.148s

After:
	real	0m0.290s
	user	0m1.732s
	sys	0m0.232s

So it's helping a lot (~3.5x and ~3.9x) on this 4-core HT setup. 

I don't seem to ever get more than a 4x speedup, so my guess is that HT 
simply isn't able to do much of anything with this load. 

The profile for the threaded case says:

    51.73%      git  libc-2.11.1.so                 [.] re_search_internal
    11.47%      git  [kernel]                       [k] copy_user_generic_string
     2.90%      git  libc-2.11.1.so                 [.] __strlen_sse2
     2.66%      git  [kernel]                       [k] link_path_walk
     2.55%      git  [kernel]                       [k] intel_pmu_enable_all
     2.40%      git  [kernel]                       [k] __d_lookup
     1.71%      git  libc-2.11.1.so                 [.] __GI___libc_malloc
     1.55%      git  [kernel]                       [k] _raw_spin_lock
     1.43%      git  [kernel]                       [k] sys_futex
     1.30%      git  libc-2.11.1.so                 [.] __cfree
     1.28%      git  [kernel]                       [k] intel_pmu_disable_all
     1.25%      git  libc-2.11.1.so                 [.] __GI_memchr
     1.14%      git  libc-2.11.1.so                 [.] _int_malloc
     1.02%      git  [kernel]                       [k] effective_load

and the only thing that makes me go "eh?" there is the strlen(). Why is 
that so hot?  But locking doesn't seem to be the biggest issue, and in 
general I think this is all pretty good. The 'effective_load' thing is the 
scheduler, so there's certainly some context switching going on, probably 
still due to excessive synchronization, but it's equally clear that that 
is certainly not a dominant factor.

One potentially interesting data point is that if I make NR_THREADS be 16, 
performance goes down, and I get more locking overhead. So NR_THREADS of 8 
works well on this machine.

So ack from me. The patch looks reasonably clean too, at least for 
something as complex as a multi-threaded grep.

One worry is, of course, whether all regex() implementations are 
thread-safe. Maybe there are broken libraries that have hidden global 
state in them?

			Linus

^ permalink raw reply

* Re: What's cooking in git.git (Jan 2010, #08; Sun, 24)
From: Junio C Hamano @ 2010-01-25 23:47 UTC (permalink / raw)
  To: Petr Baudis
  Cc: Jakub Narebski, Junio C Hamano, git, J.H.,
	John 'Warthog9' Hawley
In-Reply-To: <20100125231241.GA4159@machine.or.cz>

Petr Baudis <pasky@suse.cz> writes:

> ... I think
> it's healthier for new gitweb stuff to develop more in-tree, even
> if that means few angry users annoyed about less-than-perfect UI stuff,
> rather than polish the diamond in infinite discussions; worse is better!
> The only thing we should be worried about is avoiding introducing new
> bad URL interfaces since we should keep backwards compatibility there.
> ...
> For the main caching patch, it seems like good idea to take Jakub's
> split-up series instead, let's see what is J.H.'s opinion on the series?

Sounds reasonable.

^ permalink raw reply

* [PATCH] grep: expose "status-only" feature via -q
From: Junio C Hamano @ 2010-01-25 23:37 UTC (permalink / raw)
  To: git

Teach "git grep" a new "-q" option to report the presense of a match via
its exit status without showing any output, similar to how "grep -q"
works.  Internally "grep" engine already knew this "status-only" mode of
operation because it needed to grep inside log message to filter commits
when called from the "git log" machinery, and this patch only exposes it
to the command line tool.

A somewhat unfair benchmark in the Linux kernel directory shows a dramatic
improvement:

    (with patch)
    $ time ../git.git/git grep -q linux HEAD ; echo $?

    real    0m0.030s
    user    0m0.004s
    sys     0m0.004s
    0

    (without patch)
    $ time git grep linux HEAD >/dev/null; echo $?

    real    0m4.432s
    user    0m4.272s
    sys     0m0.076s
    0

This is "somewhat unfair" because I knew a file with such a string comes
very early in the tree traversal (namely, ".gitignore").

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-grep.c |   16 ++++++++++++++--
 1 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/builtin-grep.c b/builtin-grep.c
index da854fa..64cdfef 100644
--- a/builtin-grep.c
+++ b/builtin-grep.c
@@ -236,6 +236,8 @@ static int grep_cache(struct grep_opt *opt, const char **paths, int cached)
 				 !strcmp(ce->name, active_cache[nr]->name));
 			nr--; /* compensate for loop control */
 		}
+		if (hit && opt->status_only)
+			break;
 	}
 	free_grep_patterns(opt);
 	return hit;
@@ -293,6 +295,8 @@ static int grep_tree(struct grep_opt *opt, const char **paths,
 			hit |= grep_tree(opt, paths, &sub, tree_name, down);
 			free(data);
 		}
+		if (hit && opt->status_only)
+			break;
 	}
 	strbuf_release(&pathbuf);
 	return hit;
@@ -329,8 +333,11 @@ static int grep_directory(struct grep_opt *opt, const char **paths)
 	setup_standard_excludes(&dir);
 
 	fill_directory(&dir, paths);
-	for (i = 0; i < dir.nr; i++)
+	for (i = 0; i < dir.nr; i++) {
 		hit |= grep_file(opt, dir.entries[i]->name);
+		if (hit && opt->status_only)
+			break;
+	}
 	free_grep_patterns(opt);
 	return hit;
 }
@@ -505,6 +512,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 		{ OPTION_CALLBACK, ')', NULL, &opt, NULL, "",
 		  PARSE_OPT_NOARG | PARSE_OPT_NONEG | PARSE_OPT_NODASH,
 		  close_callback },
+		OPT_BOOLEAN('q', "quick", &opt.status_only,
+			    "indicate hit with exit status without output"),
 		OPT_BOOLEAN(0, "all-match", &opt.all_match,
 			"show only matches from files that match all patterns"),
 		OPT_GROUP(""),
@@ -628,8 +637,11 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	for (i = 0; i < list.nr; i++) {
 		struct object *real_obj;
 		real_obj = deref_tag(list.objects[i].item, NULL, 0);
-		if (grep_object(&opt, paths, real_obj, list.objects[i].name))
+		if (grep_object(&opt, paths, real_obj, list.objects[i].name)) {
 			hit = 1;
+			if (opt.status_only)
+				break;
+		}
 	}
 	free_grep_patterns(&opt);
 	return !hit;

^ permalink raw reply related

* Re: What's cooking in git.git (Jan 2010, #08; Sun, 24)
From: Petr Baudis @ 2010-01-25 23:12 UTC (permalink / raw)
  To: Jakub Narebski; +Cc: Junio C Hamano, git, J.H., John 'Warthog9' Hawley
In-Reply-To: <m3eildbydx.fsf@localhost.localdomain>

On Mon, Jan 25, 2010 at 02:43:01PM -0800, Jakub Narebski wrote:
> You are right that actual caching support is in flux.  The discussion
> continues: it is very good that we have the voice from Pasky, too.

Unfortunately, I can't give much time to it, or actual contributions.
I think it would be great to apply at least the miscellanous-changes
part to (i) move this further (ii) give jh some positive feedback. :)
There are ugly minor things, but they can be patched up later. I think
it's healthier for new gitweb stuff to develop more in-tree, even
if that means few angry users annoyed about less-than-perfect UI stuff,
rather than polish the diamond in infinite discussions; worse is better!
The only thing we should be worried about is avoiding introducing new
bad URL interfaces since we should keep backwards compatibility there.

But Jakub's opinion is definitely much more important, he gives gitweb
much more time than me.

For the main caching patch, it seems like good idea to take Jakub's
split-up series instead, let's see what is J.H.'s opinion on the series?

-- 
				Petr "Pasky" Baudis
If you can't see the value in jet powered ants you should turn in
your nerd card. -- Dunbal (464142)

^ permalink raw reply

* [PATCH v4] Threaded grep
From: Fredrik Kuivinen @ 2010-01-25 22:51 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Linus Torvalds, Johannes Sixt

Make git grep use threads when it is available.

The results below are best of five runs in the Linux repository (on a
box with two cores).

With the patch:

git grep qwerty
1.58user 0.55system 0:01.16elapsed 183%CPU (0avgtext+0avgdata 0maxresident)k
0inputs+800outputs (0major+5774minor)pagefaults 0swaps

Without:

git grep qwerty
1.59user 0.43system 0:02.02elapsed 100%CPU (0avgtext+0avgdata 0maxresident)k
0inputs+800outputs (0major+3716minor)pagefaults 0swaps


And with a pattern with quite a few matches:

With the patch:

$ /usr/bin/time git grep void
5.61user 0.56system 0:03.44elapsed 179%CPU (0avgtext+0avgdata 0maxresident)k
0inputs+800outputs (0major+5587minor)pagefaults 0swaps

Without:

$ /usr/bin/time git grep void
5.36user 0.51system 0:05.87elapsed 100%CPU (0avgtext+0avgdata 0maxresident)k
0inputs+800outputs (0major+3693minor)pagefaults 0swaps

In either case we gain about 40% by the threading.

Signed-off-by: Fredrik Kuivinen <frekui@gmail.com>
---

The patch has been rebased on top of next.

Additional changes since v3:

* Fix some issues with Git's native pthreads implementation on
  Windows (pthread_cond_broadcast is still used though).
* Fix style issues.
* When greping in a tree, allocate memory for the data buffer as
  late as possible.
* Return void from grep_sha1_async and grep_file_async instead of
  always returning 0.

 builtin-grep.c |  394 ++++++++++++++++++++++++++++++++++++++++++++++++++++----
 grep.c         |  106 +++++++++++++--
 grep.h         |    6 +
 3 files changed, 457 insertions(+), 49 deletions(-)

diff --git a/builtin-grep.c b/builtin-grep.c
index da854fa..252cb0b 100644
--- a/builtin-grep.c
+++ b/builtin-grep.c
@@ -16,11 +16,270 @@
 #include "quote.h"
 #include "dir.h"
 
+#ifndef NO_PTHREADS
+#include "thread-utils.h"
+#include <pthread.h>
+#endif
+
 static char const * const grep_usage[] = {
 	"git grep [options] [-e] <pattern> [<rev>...] [[--] path...]",
 	NULL
 };
 
+static int use_threads = 1;
+
+#ifndef NO_PTHREADS
+#define THREADS 8
+static pthread_t threads[THREADS];
+
+static void *load_sha1(const unsigned char *sha1, unsigned long *size,
+		       const char *name);
+static void *load_file(const char *filename, size_t *sz);
+
+enum work_type {WORK_SHA1, WORK_FILE};
+
+/* We use one producer thread and THREADS consumer
+ * threads. The producer adds struct work_items to 'todo' and the
+ * consumers pick work items from the same array.
+ */
+struct work_item
+{
+	enum work_type type;
+	char *name;
+
+	/* if type == WORK_SHA1, then 'identifier' is a SHA1,
+	 * otherwise type == WORK_FILE, and 'identifier' is a NUL
+	 * terminated filename.
+	 */
+	void *identifier;
+	char done;
+	struct strbuf out;
+};
+
+/* In the range [todo_done, todo_start) in 'todo' we have work_items
+ * that have been or are processed by a consumer thread. We haven't
+ * written the result for these to stdout yet.
+ *
+ * The work_items in [todo_start, todo_end) are waiting to be picked
+ * up by a consumer thread.
+ *
+ * The ranges are modulo TODO_SIZE.
+ */
+#define TODO_SIZE 128
+static struct work_item todo[TODO_SIZE];
+static int todo_start;
+static int todo_end;
+static int todo_done;
+
+/* Has all work items been added? */
+static int all_work_added;
+
+/* This lock protects all the variables above. */
+static pthread_mutex_t grep_lock;
+
+/* Used to serialize calls to read_sha1_file. */
+static pthread_mutex_t read_sha1_lock;
+
+/* Signalled when a new work_item is added to todo. */
+static pthread_cond_t cond_add;
+
+/* Signalled when the result from one work_item is written to
+ * stdout.
+ */
+static pthread_cond_t cond_write;
+
+/* Signalled when we are finished with everything. */
+static pthread_cond_t cond_result;
+
+static void add_work(enum work_type type, char *name, void *id)
+{
+	pthread_mutex_lock(&grep_lock);
+
+	while ((todo_end+1) % ARRAY_SIZE(todo) == todo_done) {
+		pthread_cond_wait(&cond_write, &grep_lock);
+	}
+
+	todo[todo_end].type = type;
+	todo[todo_end].name = name;
+	todo[todo_end].identifier = id;
+	todo[todo_end].done = 0;
+	strbuf_reset(&todo[todo_end].out);
+	todo_end = (todo_end + 1) % ARRAY_SIZE(todo);
+
+	pthread_cond_signal(&cond_add);
+	pthread_mutex_unlock(&grep_lock);
+}
+
+static struct work_item *get_work()
+{
+	struct work_item *ret;
+
+	pthread_mutex_lock(&grep_lock);
+	while (todo_start == todo_end && !all_work_added) {
+		pthread_cond_wait(&cond_add, &grep_lock);
+	}
+
+	if (todo_start == todo_end && all_work_added) {
+		ret = NULL;
+	} else {
+		ret = &todo[todo_start];
+		todo_start = (todo_start + 1) % ARRAY_SIZE(todo);
+	}
+	pthread_mutex_unlock(&grep_lock);
+	return ret;
+}
+
+static void grep_sha1_async(struct grep_opt *opt, char *name,
+			    const unsigned char *sha1)
+{
+	unsigned char *s;
+	s = xmalloc(20);
+	memcpy(s, sha1, 20);
+	add_work(WORK_SHA1, name, s);
+}
+
+static void grep_file_async(struct grep_opt *opt, char *name,
+			    const char *filename)
+{
+	add_work(WORK_FILE, name, xstrdup(filename));
+}
+
+static void work_done(struct work_item *w)
+{
+	int old_done;
+
+	pthread_mutex_lock(&grep_lock);
+	w->done = 1;
+	old_done = todo_done;
+	for(; todo[todo_done].done && todo_done != todo_start;
+	    todo_done = (todo_done+1) % ARRAY_SIZE(todo)) {
+		w = &todo[todo_done];
+		write_or_die(1, w->out.buf, w->out.len);
+		free(w->name);
+		free(w->identifier);
+	}
+
+	if (old_done != todo_done)
+		pthread_cond_signal(&cond_write);
+
+	if (all_work_added && todo_done == todo_end)
+		pthread_cond_signal(&cond_result);
+
+	pthread_mutex_unlock(&grep_lock);
+}
+
+static void *run(void *arg)
+{
+	int hit = 0;
+	struct grep_opt *opt = arg;
+
+	while (1) {
+		struct work_item *w = get_work();
+		if (!w)
+			break;
+
+		opt->output_priv = w;
+		if (w->type == WORK_SHA1) {
+			unsigned long sz;
+			void* data;
+
+			pthread_mutex_lock(&read_sha1_lock);
+			data = load_sha1(w->identifier, &sz, w->name);
+			pthread_mutex_unlock(&read_sha1_lock);
+
+			if (data) {
+				hit |= grep_buffer(opt, w->name, data, sz);
+				free(data);
+			}
+		} else if (w->type == WORK_FILE) {
+			size_t sz;
+			void* data = load_file(w->identifier, &sz);
+			if (data) {
+				hit |= grep_buffer(opt, w->name, data, sz);
+				free(data);
+			}
+		} else {
+			assert(0);
+		}
+
+		work_done(w);
+	}
+
+	return (void*) (intptr_t) hit;
+}
+
+static void strbuf_out(struct grep_opt *opt, const void *buf, size_t size)
+{
+	struct work_item *w = opt->output_priv;
+	strbuf_add(&w->out, buf, size);
+}
+
+static void start_threads(struct grep_opt *opt)
+{
+	int i;
+
+	pthread_mutex_init(&grep_lock, NULL);
+	pthread_mutex_init(&read_sha1_lock, NULL);
+	pthread_cond_init(&cond_add, NULL);
+	pthread_cond_init(&cond_write, NULL);
+	pthread_cond_init(&cond_result, NULL);
+
+	for (i = 0; i < ARRAY_SIZE(todo); i++) {
+		strbuf_init(&todo[i].out, 0);
+	}
+
+	for (i = 0; i < ARRAY_SIZE(threads); i++) {
+		int err;
+		struct grep_opt *o = grep_opt_dup(opt);
+		o->output = strbuf_out;
+		compile_grep_patterns(o);
+		err = pthread_create(&threads[i], NULL, run, o);
+
+		if (err)
+			die("grep: failed to create thread: %s",
+			    strerror(err));
+	}
+}
+
+static int wait_all()
+{
+	int hit = 0;
+	int i;
+
+	pthread_mutex_lock(&grep_lock);
+	all_work_added = 1;
+
+	/* Wait until all work is done. */
+	while (todo_done != todo_end)
+		pthread_cond_wait(&cond_result, &grep_lock);
+
+	/* Wake up all the consumer threads so they can see that there
+	 * is no more work to do.
+	 */
+	pthread_cond_broadcast(&cond_add);
+	pthread_mutex_unlock(&grep_lock);
+
+	for (i = 0; i < ARRAY_SIZE(threads); i++) {
+		void *h;
+		pthread_join(threads[i], &h);
+		hit |= (int) (intptr_t) h;
+	}
+
+	pthread_mutex_destroy(&grep_lock);
+	pthread_mutex_destroy(&read_sha1_lock);
+	pthread_cond_destroy(&cond_add);
+	pthread_cond_destroy(&cond_write);
+	pthread_cond_destroy(&cond_result);
+
+	return hit;
+}
+#else /* !NO_PTHREADS */
+static int wait_all()
+{
+	return 0;
+}
+#endif
+
 static int grep_config(const char *var, const char *value, void *cb)
 {
 	struct grep_opt *opt = cb;
@@ -144,37 +403,60 @@ static int pathspec_matches(const char **paths, const char *name, int max_depth)
 	return 0;
 }
 
-static int grep_sha1(struct grep_opt *opt, const unsigned char *sha1, const char *name, int tree_name_len)
+static void *load_sha1(const unsigned char *sha1, unsigned long *size,
+		       const char *name)
 {
-	unsigned long size;
-	char *data;
 	enum object_type type;
-	int hit;
-	struct strbuf pathbuf = STRBUF_INIT;
+	char *data = read_sha1_file(sha1, &type, size);
 
-	data = read_sha1_file(sha1, &type, &size);
-	if (!data) {
+	if (!data)
 		error("'%s': unable to read %s", name, sha1_to_hex(sha1));
-		return 0;
-	}
+
+	return data;
+}
+
+static int grep_sha1(struct grep_opt *opt, const unsigned char *sha1,
+		     const char *filename, int tree_name_len)
+{
+	struct strbuf pathbuf = STRBUF_INIT;
+	char *name;
+
 	if (opt->relative && opt->prefix_length) {
-		quote_path_relative(name + tree_name_len, -1, &pathbuf, opt->prefix);
-		strbuf_insert(&pathbuf, 0, name, tree_name_len);
-		name = pathbuf.buf;
+		quote_path_relative(filename + tree_name_len, -1, &pathbuf,
+				    opt->prefix);
+		strbuf_insert(&pathbuf, 0, filename, tree_name_len);
+	} else {
+		strbuf_addstr(&pathbuf, filename);
+	}
+
+	name = strbuf_detach(&pathbuf, NULL);
+
+#ifndef NO_PTHREADS
+	if (use_threads) {
+		grep_sha1_async(opt, name, sha1);
+		return 0;
+	} else
+#endif
+	{
+		int hit;
+		unsigned long sz;
+		void *data = load_sha1(sha1, &sz, name);
+		if (!data)
+			hit = 0;
+		else
+			hit = grep_buffer(opt, name, data, sz);
+
+		free(data);
+		free(name);
+		return hit;
 	}
-	hit = grep_buffer(opt, name, data, size);
-	strbuf_release(&pathbuf);
-	free(data);
-	return hit;
 }
 
-static int grep_file(struct grep_opt *opt, const char *filename)
+static void *load_file(const char *filename, size_t *sz)
 {
 	struct stat st;
-	int i;
 	char *data;
-	size_t sz;
-	struct strbuf buf = STRBUF_INIT;
+	int i;
 
 	if (lstat(filename, &st) < 0) {
 	err_ret:
@@ -184,25 +466,52 @@ static int grep_file(struct grep_opt *opt, const char *filename)
 	}
 	if (!S_ISREG(st.st_mode))
 		return 0;
-	sz = xsize_t(st.st_size);
+	*sz = xsize_t(st.st_size);
 	i = open(filename, O_RDONLY);
 	if (i < 0)
 		goto err_ret;
-	data = xmalloc(sz + 1);
-	if (st.st_size != read_in_full(i, data, sz)) {
+	data = xmalloc(*sz + 1);
+	if (st.st_size != read_in_full(i, data, *sz)) {
 		error("'%s': short read %s", filename, strerror(errno));
 		close(i);
 		free(data);
 		return 0;
 	}
 	close(i);
-	data[sz] = 0;
+	data[*sz] = 0;
+	return data;
+}
+
+static int grep_file(struct grep_opt *opt, const char *filename)
+{
+	struct strbuf buf = STRBUF_INIT;
+	char *name;
+
 	if (opt->relative && opt->prefix_length)
-		filename = quote_path_relative(filename, -1, &buf, opt->prefix);
-	i = grep_buffer(opt, filename, data, sz);
-	strbuf_release(&buf);
-	free(data);
-	return i;
+		quote_path_relative(filename, -1, &buf, opt->prefix);
+	else
+		strbuf_addstr(&buf, filename);
+	name = strbuf_detach(&buf, NULL);
+
+#ifndef NO_PTHREADS
+	if (use_threads) {
+		grep_file_async(opt, name, filename);
+		return 0;
+	} else
+#endif
+	{
+		int hit;
+		size_t sz;
+		void *data = load_file(filename, &sz);
+		if (!data)
+			hit = 0;
+		else
+			hit = grep_buffer(opt, name, data, sz);
+
+		free(data);
+		free(name);
+		return hit;
+	}
 }
 
 static int grep_cache(struct grep_opt *opt, const char **paths, int cached)
@@ -572,6 +881,17 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 		opt.regflags |= REG_ICASE;
 	if ((opt.regflags != REG_NEWLINE) && opt.fixed)
 		die("cannot mix --fixed-strings and regexp");
+
+#ifndef NO_PTHREADS
+	if (online_cpus() == 1 || !grep_threads_ok(&opt))
+		use_threads = 0;
+
+	if (use_threads)
+		start_threads(&opt);
+#else
+	use_threads = 0;
+#endif
+
 	compile_grep_patterns(&opt);
 
 	/* Check revs and then paths */
@@ -609,17 +929,26 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	}
 
 	if (!use_index) {
+		int hit;
 		if (cached)
 			die("--cached cannot be used with --no-index.");
 		if (list.nr)
 			die("--no-index cannot be used with revs.");
-		return !grep_directory(&opt, paths);
+		hit = grep_directory(&opt, paths);
+		if (use_threads)
+			hit |= wait_all();
+		return !hit;
 	}
 
 	if (!list.nr) {
+		int hit;
 		if (!cached)
 			setup_work_tree();
-		return !grep_cache(&opt, paths, cached);
+
+		hit = grep_cache(&opt, paths, cached);
+		if (use_threads)
+			hit |= wait_all();
+		return !hit;
 	}
 
 	if (cached)
@@ -631,6 +960,9 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 		if (grep_object(&opt, paths, real_obj, list.objects[i].name))
 			hit = 1;
 	}
+
+	if (use_threads)
+		hit |= wait_all();
 	free_grep_patterns(&opt);
 	return !hit;
 }
diff --git a/grep.c b/grep.c
index 8e1f7de..d281a02 100644
--- a/grep.c
+++ b/grep.c
@@ -29,6 +29,28 @@ void append_grep_pattern(struct grep_opt *opt, const char *pat,
 	p->next = NULL;
 }
 
+struct grep_opt *grep_opt_dup(const struct grep_opt *opt)
+{
+	struct grep_pat *pat;
+	struct grep_opt *ret = xmalloc(sizeof(struct grep_opt));
+	*ret = *opt;
+
+	ret->pattern_list = NULL;
+	ret->pattern_tail = &ret->pattern_list;
+
+	for(pat = opt->pattern_list; pat != NULL; pat = pat->next)
+	{
+		if(pat->token == GREP_PATTERN_HEAD)
+			append_header_grep_pattern(ret, pat->field,
+						   pat->pattern);
+		else
+			append_grep_pattern(ret, pat->pattern, pat->origin,
+					    pat->no, pat->token);
+	}
+
+	return ret;
+}
+
 static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)
 {
 	int err;
@@ -253,7 +275,8 @@ static int word_char(char ch)
 
 static void show_name(struct grep_opt *opt, const char *name)
 {
-	printf("%s%c", name, opt->null_following_name ? '\0' : '\n');
+	opt->output(opt, name, strlen(name));
+	opt->output(opt, opt->null_following_name ? "\0" : "\n", 1);
 }
 
 
@@ -490,24 +513,32 @@ static void show_line(struct grep_opt *opt, char *bol, char *eol,
 		      const char *name, unsigned lno, char sign)
 {
 	int rest = eol - bol;
+	char sign_str[1];
 
+	sign_str[0] = sign;
 	if (opt->pre_context || opt->post_context) {
 		if (opt->last_shown == 0) {
 			if (opt->show_hunk_mark)
-				fputs("--\n", stdout);
+				opt->output(opt, "--\n", 3);
 			else
 				opt->show_hunk_mark = 1;
 		} else if (lno > opt->last_shown + 1)
-			fputs("--\n", stdout);
+			opt->output(opt, "--\n", 3);
 	}
 	opt->last_shown = lno;
 
 	if (opt->null_following_name)
-		sign = '\0';
-	if (opt->pathname)
-		printf("%s%c", name, sign);
-	if (opt->linenum)
-		printf("%d%c", lno, sign);
+		sign_str[0] = '\0';
+	if (opt->pathname) {
+		opt->output(opt, name, strlen(name));
+		opt->output(opt, sign_str, 1);
+	}
+	if (opt->linenum) {
+		char buf[32];
+		snprintf(buf, sizeof(buf), "%d", lno);
+		opt->output(opt, buf, strlen(buf));
+		opt->output(opt, sign_str, 1);
+	}
 	if (opt->color) {
 		regmatch_t match;
 		enum grep_context ctx = GREP_CONTEXT_BODY;
@@ -518,18 +549,22 @@ static void show_line(struct grep_opt *opt, char *bol, char *eol,
 		while (next_match(opt, bol, eol, ctx, &match, eflags)) {
 			if (match.rm_so == match.rm_eo)
 				break;
-			printf("%.*s%s%.*s%s",
-			       (int)match.rm_so, bol,
-			       opt->color_match,
-			       (int)(match.rm_eo - match.rm_so), bol + match.rm_so,
-			       GIT_COLOR_RESET);
+
+			opt->output(opt, bol, match.rm_so);
+			opt->output(opt, opt->color_match,
+				    strlen(opt->color_match));
+			opt->output(opt, bol + match.rm_so,
+				    (int)(match.rm_eo - match.rm_so));
+			opt->output(opt, GIT_COLOR_RESET,
+				    strlen(GIT_COLOR_RESET));
 			bol += match.rm_eo;
 			rest -= match.rm_eo;
 			eflags = REG_NOTBOL;
 		}
 		*eol = ch;
 	}
-	printf("%.*s\n", rest, bol);
+	opt->output(opt, bol, rest);
+	opt->output(opt, "\n", 1);
 }
 
 static int match_funcname(struct grep_opt *opt, char *bol, char *eol)
@@ -667,6 +702,32 @@ static int look_ahead(struct grep_opt *opt,
 	return 0;
 }
 
+int grep_threads_ok(const struct grep_opt *opt)
+{
+	/* If this condition is true, then we may use the attribute
+	 * machinery in grep_buffer_1. The attribute code is not
+	 * thread safe, so we disable the use of threads.
+	 */
+	if (opt->funcname && !opt->unmatch_name_only && !opt->status_only &&
+	    !opt->name_only)
+		return 0;
+
+	/* If we are showing hunk marks, we should not do it for the
+	 * first match. The synchronization problem we get for this
+	 * constraint is not yet solved, so we disable threading in
+	 * this case.
+	 */
+	if (opt->pre_context || opt->post_context)
+		return 0;
+
+	return 1;
+}
+
+static void std_output(struct grep_opt *opt, const void *buf, size_t size)
+{
+	fwrite(buf, size, 1, stdout);
+}
+
 static int grep_buffer_1(struct grep_opt *opt, const char *name,
 			 char *buf, unsigned long size, int collect_hits)
 {
@@ -682,6 +743,9 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name,
 
 	opt->last_shown = 0;
 
+	if (!opt->output)
+		opt->output = std_output;
+
 	if (buffer_is_binary(buf, size)) {
 		switch (opt->binary) {
 		case GREP_BINARY_DEFAULT:
@@ -754,7 +818,9 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name,
 			if (opt->status_only)
 				return 1;
 			if (binary_match_only) {
-				printf("Binary file %s matches\n", name);
+				opt->output(opt, "Binary file ", 12);
+				opt->output(opt, name, strlen(name));
+				opt->output(opt, " matches\n", 9);
 				return 1;
 			}
 			if (opt->name_only) {
@@ -810,9 +876,13 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name,
 	 * which feels mostly useless but sometimes useful.  Maybe
 	 * make it another option?  For now suppress them.
 	 */
-	if (opt->count && count)
-		printf("%s%c%u\n", name,
-		       opt->null_following_name ? '\0' : ':', count);
+	if (opt->count && count) {
+		char buf[32];
+		opt->output(opt, name, strlen(name));
+		snprintf(buf, sizeof(buf), "%c%u\n",
+			 opt->null_following_name ? '\0' : ':', count);
+		opt->output(opt, buf, strlen(buf));
+	}
 	return !!last_hit;
 }
 
diff --git a/grep.h b/grep.h
index 0c61b00..9703087 100644
--- a/grep.h
+++ b/grep.h
@@ -91,6 +91,9 @@ struct grep_opt {
 	unsigned last_shown;
 	int show_hunk_mark;
 	void *priv;
+
+	void (*output)(struct grep_opt *opt, const void *data, size_t size);
+	void *output_priv;
 };
 
 extern void append_grep_pattern(struct grep_opt *opt, const char *pat, const char *origin, int no, enum grep_pat_token t);
@@ -99,4 +102,7 @@ extern void compile_grep_patterns(struct grep_opt *opt);
 extern void free_grep_patterns(struct grep_opt *opt);
 extern int grep_buffer(struct grep_opt *opt, const char *name, char *buf, unsigned long size);
 
+extern struct grep_opt *grep_opt_dup(const struct grep_opt *opt);
+extern int grep_threads_ok(const struct grep_opt *opt);
+
 #endif

^ permalink raw reply related

* Re: [PATCH v3] Threaded grep
From: Fredrik Kuivinen @ 2010-01-25 22:47 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Linus Torvalds, Git Mailing List, Johannes Sixt
In-Reply-To: <4c8ef71001181612l72ec73ecmae709fbb475752b0@mail.gmail.com>

On Tue, Jan 19, 2010 at 01:12, Fredrik Kuivinen <frekui@gmail.com> wrote:
> On Mon, Jan 18, 2010 at 23:10, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> I am wondering if this can somehow be made into a change with alot
>> smaller inpact, in spirit similar to "preloading".  The higher level
>> loop may walk the input (be it cache, tree, or directory), issuing one
>> grep_file() or grep_sha1() at a time just like the existing code, but
>> the thread-aware code adds "priming" phase that (1) populates a "work
>> queue" with a very small memory footprint, and that (2) starts more
>> than one underlying grep on different files and blobs (up to the
>> number of threads you are allowed, like your "#define THREADS 8"), and
>> that (3) upon completion of one "work queue" item, the work queue item
>> is marked with its result.
>>
>> Then grep_file() and grep_sha1() _may_ notice that the work queue item
>> hasn't completed, and would wait in such a case, or just emits the
>> output produced earlier (could be much earlier) by the worker bee.
>>
>> The low-memory-footprint work queue could be implemented as a lazy
>> one, and may be very different depending on how the input is created.
>> If we are grepping in the index, it could be a small array of <array
>> index in active_cache[], done-bit, result-bit, result-strbuf> with a
>> single integer that is a pointer into the index to indicate up to
>> which index entry the work queue has been populated.
>
> I will have to think about this a bit more. It's getting late here.

I will be sending v4 of the patch in a couple of minutes, but I want
to comment on this first.

Sure, it is probably possible to structure the code as you suggest.
However, I am not so sure that it will be a significantly smaller
change. I find the approach taken in the patch to be quite nice as the
threaded and non-threaded cases are fairly similar. There is a block
of code which deals with the threading, but that part is mostly
self-contained. In v4 I have not changed the overall approach to the
problem.

- Fredrik

^ permalink raw reply

* Re: What's cooking in git.git (Jan 2010, #08; Sun, 24)
From: Jakub Narebski @ 2010-01-25 22:43 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, J.H., John 'Warthog9' Hawley, Petr Baudis
In-Reply-To: <7vfx5u6bn9.fsf@alter.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> writes:


> * jh/gitweb-cached (2010-01-13) 9 commits

>  - gitweb: File based caching layer (from git.kernel.org)
>  - gitweb: Convert output to using indirect file handle

>  - gitweb: cleanup error message produced by undefined $site_header
>  - gitweb: add a get function to compliment print_sort_th
>  - gitweb: add a get function to compliment print_local_time
>  - gitweb: Makefile improvements
>  - gitweb: Add option to force version match
>  - gitweb: change die_error to take "extra" argument for extended die information
>  - gitweb: Load checking
> 
> I know there is a series to split the later ones into smaller chunks that
> are being discussed on the list, but they don't appear here.  I'd prefer
> to pick the series up after all the dust from the discussion settles.

Well, this series actually consist of two parts: miscellaneous gitweb
improvements and actual gitweb output caching.  The first part is more
or less ready; in particular the following patches are I think ready
for inclusion:
 - gitweb: Makefile improvements
 - gitweb: change die_error to take "extra" argument for extended die information
 - gitweb: Load checking
(Although die_error improvements is used by 'force version match',
which is not ready yet).


The following patches needs minimal fixups or extensions.  In
 - gitweb: add a get function to compliment print_sort_th
 - gitweb: add a get function to compliment print_local_time
IMVHO the new (unused) get_* functions should really be named format_*
to follow gitweb convention for naming subroutines.  And of course
that would probably require accompanying change to the commit message.

The patch
 - gitweb: cleanup error message produced by undefined $site_header
solves its problem... but not fully.  There are other variables
holding file names that could, in theory, be undefined.  Also the
commit message should perhaps mention that is defensive coding against
errors in gitweb config file, as in normal situation those variables
would be set to empty string, but not undefined.


I am not sure about the
 - gitweb: Add option to force version match
patch.  It is not that useful if the feature is turned off by default;
on the other hand if the feature is turned on by default the error
message needs to be changed... and be a bit more complicated in the
case when there are no config files (see my reply in thread).


You are right that actual caching support is in flux.  The discussion
continues: it is very good that we have the voice from Pasky, too.

-- 
Jakub Narebski
Poland
ShadeHawk on #git

^ permalink raw reply

* Re: [PATCH not-for-mainline] Implement git-vcs-p4
From: Sverre Rabbelier @ 2010-01-25 22:28 UTC (permalink / raw)
  To: Daniel Barkalow; +Cc: git, Tor Arvid Lund
In-Reply-To: <alpine.LNX.2.00.1001251708530.14365@iabervon.org>

Heya,

On Mon, Jan 25, 2010 at 23:26, Daniel Barkalow <barkalow@iabervon.org> wrote:

> That's the right thing to do. However, you might notice that there's no
> code around to actually do it (or anything else, presently).

Check, I reckon I can look at how the 'import' command is implemented,
and turn the logic around (tie fast-export's stdout to the helpers
stdin or such).

> I've been working primarily on non-git-related stuff lately, and that's
> been keeping me pretty busy.

I know the problem :).

> I can definitely review and discuss design
> issues, but I'm not sure I'll manage writing anything any time soon.

Ok, fair enough, hopefully I'll have some code to show soon :).

-- 
Cheers,

Sverre Rabbelier

^ permalink raw reply

* Re: [PATCH not-for-mainline] Implement git-vcs-p4
From: Daniel Barkalow @ 2010-01-25 22:26 UTC (permalink / raw)
  To: Sverre Rabbelier; +Cc: git, Tor Arvid Lund
In-Reply-To: <fabb9a1e1001251353q3739a5efq38606ca0b63ce10@mail.gmail.com>

On Mon, 25 Jan 2010, Sverre Rabbelier wrote:

> Heya,
> 
> On Mon, Jan 25, 2010 at 22:35, Daniel Barkalow <barkalow@iabervon.org> wrote:
> > The push support is also currently based on a transport helper
> > export design that isn't upstream and I don't like any more; a better
> > design is probably to have the core send an "export" command and then a
> > gfi stream, but I haven't worked on this.
> 
> Ah, that was actually what I _thought_ the export command did, and how
> I was/am going to implement it for git-remote-hg.

That's the right thing to do. However, you might notice that there's no 
code around to actually do it (or anything else, presently).

> Do you think you'll have time to work on that anytime soon? My git 
> budget should go up enough to do some serious work after this weekend, 
> so if you have time we could work on moulding the 'export' feature into 
> something more generically useful. If not, I'll do the work anyway, and 
> hope you'll have time to review it at some point :).

I've been working primarily on non-git-related stuff lately, and that's 
been keeping me pretty busy. I can definitely review and discuss design 
issues, but I'm not sure I'll manage writing anything any time soon.

	-Daniel
*This .sig left intentionally blank*

^ permalink raw reply

* Re: [PATCH not-for-mainline] Implement git-vcs-p4
From: Sverre Rabbelier @ 2010-01-25 21:53 UTC (permalink / raw)
  To: Daniel Barkalow; +Cc: git, Tor Arvid Lund
In-Reply-To: <alpine.LNX.2.00.1001251628431.14365@iabervon.org>

Heya,

On Mon, Jan 25, 2010 at 22:35, Daniel Barkalow <barkalow@iabervon.org> wrote:
> The push support is also currently based on a transport helper
> export design that isn't upstream and I don't like any more; a better
> design is probably to have the core send an "export" command and then a
> gfi stream, but I haven't worked on this.

Ah, that was actually what I _thought_ the export command did, and how
I was/am going to implement it for git-remote-hg. Do you think you'll
have time to work on that anytime soon? My git budget should go up
enough to do some serious work after this weekend, so if you have time
we could work on moulding the 'export' feature into something more
generically useful. If not, I'll do the work anyway, and hope you'll
have time to review it at some point :).

-- 
Cheers,

Sverre Rabbelier

^ permalink raw reply

* Re: [PATCH 8/9] gitweb: Convert output to using indirect file handle
From: Jakub Narebski @ 2010-01-25 21:48 UTC (permalink / raw)
  To: J.H.; +Cc: Petr Baudis, John 'Warthog9' Hawley, git
In-Reply-To: <4B5E03AD.1020404@eaglescrag.net>

On Mon, 25 Jan 2010, J.H. wrote:

> > So there is a bit of rule of preservation of difficulty at work.  Either
> > we have large patch adding explicit filehandle to all print statements
> > 'print <sth> -> print $out <sth>' but simple code, or have smaller patch
> > but complicated *STDOUT manipulation, or have small patch but rely on 
> > non-core CPAN modules present.
> 
> I think depending in non-core CPAN modules is a really bad idea, and
> will cause some concern and consternation with mainline distributions,
> besides making it more complicated for users to get this up and running
> quickly and easily.  

Note: the modules in question are Capture::Tiny or PerlIO::Util.
Capture::Tiny is available as perl-Capture-Tiny RPM module for Fedora Core
up from FC11 and in DAG repository, at least. 


First, installing non-core CPAN modules locally for user is very simple
thanks to local::lib.  After setup it is simply 'cpan -i PerlIO::Util',
and PerlIO::Util gets installed locally, by default inside ~/perl5.

Second, we can go the route that Git.pm did, namely include sources for
extra modules (private-Error.pm in case of Git.pm) and install it if 
needed.  With proper build system (Module::Install, Module::Build) it is
even simpler with inc or inc::latest.  And there is always PAR[1] if you
want to have everything in one file.

[1]: http://par.perl.org

>                     While I agree there are other ways of handling this 
> I think just adding the filehandle to the print statements provides the
> easiest cross section of usability and functionality for everything
> involved.

Well, manipulating *STDOUT shouldn't be, I think, much harder, although
I guess it would need more care (so that e.g. "Generating..." page gets
written to real STDOUT).

-- 
Jakub Narebski
Poland

^ permalink raw reply

* Re: [PATCH] Handle UNC paths everywhere
From: Robin Rosenberg @ 2010-01-25 21:42 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: git, Junio C Hamano
In-Reply-To: <201001252107.45745.j6t@kdbg.org>

måndagen den 25 januari 2010 21.07.44 skrev  Johannes Sixt:
> On Montag, 25. Januar 2010, Robin Rosenberg wrote:
> > In Windows paths beginning with // are knows as UNC paths. They are
> > absolute paths, usually referring to a shared resource on a server.
> >
> > Examples of legal UNC paths
> >
> > 	\\hub\repos\repo
> > 	\\?\unc\hub\repos
> > 	\\?\d:\repo
> 
> I agree that that the problem that you are addressing needs a solution.
> 
> However, the solution is not a whole-sale replacement of
> have_dos_drive_prefix() by a function that is only a tiny bit fancier.
> Accompanying changes are needed, and perhaps more code locations need
>  change.

I was hoping to get help in identifying these and perhaps more cases to test
than then ones I thought of at first.

> > @@ -648,7 +648,7 @@ int safe_create_leading_directories_const(const char
> > *path);
> >  char *enter_repo(char *path, int strict);
> >  static inline int is_absolute_path(const char *path)
> >  {
> > -	return path[0] == '/' || has_dos_drive_prefix(path);
> > +	return path[0] == '/' || has_win32_abs_prefix(path);
> 
> Perhaps we need is_dir_sep(path[0]) here? But since I have not observed any
> breakage in connection with this code, I think that all callers feed only
> normalized paths (i.e. with forward slash). (Note that our getcwd()
probably true.

> implementation converts backslashes to forward slashes.) This means that a
> full-fledged check is not needed.
ack.

> > @@ -5,7 +5,7 @@ char *gitbasename (char *path)
> >  {
> >  	const char *base;
> >  	/* Skip over the disk name in MSDOS pathnames. */
> > -	if (has_dos_drive_prefix(path))
> > +	if (has_win32_abs_prefix(path))
> >  		path += 2;
> 
> This change is unnecessary; it really is only to skip an initial driver
> prefix. If you want to support \\?\X: style paths, more work is needed here
> so that you do not return X: or ? as the basename.

late night hacks aren't always good.

> > +#define has_win32_abs_prefix(path) \
> 
> Do we really have to name everything "win32" when it is about Windows?
hmm

> > @@ -535,7 +535,7 @@ struct child_process *git_connect(int fd[2], const
> > char *url_orig,
> >  		end = host;
> >
> >  	path = strchr(end, c);
> > -	if (path && !has_dos_drive_prefix(end)) {
> > +	if (path && !has_win32_abs_prefix(end)) {
> 
> This change is wrong because the check is really only about the drive
>  prefix: It checks that we do not mistake c:/foo as a ssh connection to
>  host c, path /foo. Yes, it does mean that on Windows we cannot have
>  remotes to hosts whose name consists only of a single letter using the rcp
>  notation (you must say ssh://c/foo if you mean it).
right.

> > @@ -409,7 +409,7 @@ int normalize_path_copy(char *dst, const char *src)
> >  {
> >  	char *dst0;
> >
> > -	if (has_dos_drive_prefix(src)) {
> > +	if (has_win32_abs_prefix(src)) {
> >  		*dst++ = *src++;
> >  		*dst++ = *src++;
> >  	}
> 
> Is skipping just two characters for \\ or \\?\whatever paths the right
>  thing?
I shouldn't skip anything. I wasn't converting the first two \'s to //.

> > @@ -342,7 +342,7 @@ const char *setup_git_directory_gently(int
> > *nongit_ok) die_errno("Unable to read current working directory");
> >
> >  	ceil_offset = longest_ancestor_length(cwd, env_ceiling_dirs);
> > -	if (ceil_offset < 0 && has_dos_drive_prefix(cwd))
> > +	if (ceil_offset < 0 && has_win32_abs_prefix(cwd))
> >  		ceil_offset = 1;
> 
> I doubt that this is correct. The purpose of this check is that "c:/" is
>  the last directory that is checked (on Unix it would be "/") when path
>  components are stripped from cwd. For UNC paths this must be adjusted
>  depending on how you want to support \\server\share and \\?\c:\paths: You
>  do not want to check whether \\server\.git or \\.git or \\?\.git are git
>  directories.

\\server\.git seems valid. Probably not a good idea, but who am I to judge?

> 
> > --- a/transport.c
> > +++ b/transport.c
> > @@ -797,7 +797,7 @@ static int is_local(const char *url)
> >  	const char *colon = strchr(url, ':');
> >  	const char *slash = strchr(url, '/');
> >  	return !colon || (slash && slash < colon) ||
> > -		has_dos_drive_prefix(url);
> > +		has_win32_abs_prefix(url);
> 
> This check is again to not mistake c:/foo as rcp style connection. No
>  change needed.
> 
> As I said, changes to other parts are perhaps also needed, most
>  prominently, make_relative_path() that prompted this patch. What about
> make_absolute_path() and make_non_relative_path()?

Thanks for the feedback. 

-- robin

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox