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

From: Shawn O. Pearce <spearce@spearce.org>
Date: Oct 7, 2007, 23:30 UTC
Message-ID: <20071007233023.GE2137@spearce.org>
In-Reply-To: <11917925011987-git-send-email-prohaska@zib.de>

Steffen Prohaska <prohaska@zib.de> wrote:

Show 11 quoted lines

> 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.

Show 10 quoted lines

> 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.

Show 11 quoted lines

> +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.