Re: [PATCH] git-gui: offer a list of recent repositories on startup

["Shawn O. Pearce" <spearce@spearce.org>]

Steffen Prohaska <prohaska@zib.de> wrote:
> If git-gui is started outside a work tree the repository
> chooser will offer a list of recently opend repositories.
> Clicking on an entry directly opens the repository.
> 
> The list of recently opened repositories is stored in the
> config as gui.recentrepos. If the list grows beyond 10
> entries it will be truncated.
> 
> Note, only repositories that are opened through the
> repository chooser will get added to the recent list.
> Repositories opened from the shell will not yet be added.

I think that all makes a lot of sense.  Three comments below about
this patch in particular.
 
> I'd suggest to reduce the number of clicks needed to open or
> clone an existing directory that is not in the list of
> recent repositories. First choosing from the radiobuttons
> and then clicking next is one click to much. There are no
> options to combine. Choosing from the list should
> immediately trigger the action.
> 
> We could either put 'Create/Clone/Open New Repository' into
> the Repository menu and only present the recent repository
> list. Or we could offer push buttons for the other actions.

I agree entirely.  That "Next" button is stupid stupid stupid.
What was I smoking that day?  :-)

I'm concerned about putting them into the Repository menu only
as then the main window is competely void and users are sort of
wondering what they should do next.  I think we should actually
do both.  Put them into the menu and as push buttons on the window.

> +	label $w_body.space
> +	label $w_body.recentlabel \
> +		-anchor w \
> +		-text "Select Recent Repository:"

This string needs to be i18n'd with [mc ...].

> +	listbox $w_body.recentlist \

Please make a field in this class called say "w_recentlist"
so you can use that field name instead of $w_body.recentlist.
This simplifies the code if we ever have to change the actual path
that the widget resides at, such as to alter the layout.

> +proc _append_recentrepos {path} {
> +	set recent [get_config gui.recentrepos]
> +	if {[lsearch $recent $path] < 0} {
> +		lappend recent $path
> +	}
> +	if {[llength $recent] > 10} {
> +		set recent [lrange $recent 1 end]
> +	}
> +	regsub -all "\[{}\]" $recent {"} recent
> +	git config --global gui.recentrepos $recent
> +}

Why treat this as a Tcl list in a single value?  Why not make it
a true multi-value configuration entry in ~/.gitconfig, like how
remote.$name.fetch is a multi-value entry?  Does Windows allow
you to put " in a path name?  Because the above regex will make
a list of paths that contains " in one of the entries invalid.

I think you also want to have this function return back immediately
if [lsearch $recent $path] >= 0 as then you don't invoke git-config
to perform a no-op change in the configuration file.  As you well
know forking on Windows is a major cost.  We shouldn't run git-config
just because the user opened a recent repository.

-- 
Shawn.