Re: [PATCH v3 4/4] git gui: allow for a long recentrepo list

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 17, 2015, 23:29 UTC
Message-ID: <xmqqbn9obqne.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1450392007-600-1-git-send-email-philipoakley@iee.org>

Philip Oakley <philipoakley@iee.org> writes:

Show 49 quoted lines

> The gui.recentrepo list may be longer than the maxrecent setting.
> Use the actual length determined earlier, now saved as $lenrecent.
>
> In an ideal world, the git gui would limit the number of entries
> to the maxrecent setting, however the recentrepo config list may
> have been extended outside of the gui, or the maxrecent setting changed
> to a reduced value. Further, when testing the gui's recentrepo
> logic it is useful to show these extra, but valid, entries.
>
> The list length will be trimmed to $maxrecent after the user selects
> a repo to open.
>
> Signed-off-by: Philip Oakley <philipoakley@iee.org>
> ---
> V2:
> word usage corrected.
> Eric's comments $gmane/282432
> V3: 
> Updated list length measure following Junio's comments $gmane/282669
>
> Replaces the previous V2 Patch 4/4.
>
> ---
>  git-gui/lib/choose_repository.tcl | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/git-gui/lib/choose_repository.tcl b/git-gui/lib/choose_repository.tcl
> index ad7a888..b4cc7dd 100644
> --- a/git-gui/lib/choose_repository.tcl
> +++ b/git-gui/lib/choose_repository.tcl
> @@ -134,7 +134,8 @@ constructor pick {} {
>  	$opts conf -state disabled
>  
>  	set sorted_recent [_get_recentrepos]
> -	if {[llength $sorted_recent] > 0} {
> +	set lenrecent [llength $sorted_recent]
> +	if {$lenrecent > 0} {
>  		if {$m_repo ne {}} {
>  			$m_repo add separator
>  			$m_repo add command \
> @@ -153,7 +154,7 @@ constructor pick {} {
>  			-background [get_bg_color $w_body.recentlabel] \
>  			-wrap none \
>  			-width 50 \
> -			-height $maxrecent
> +			-height $lenrecent
>  		$w_recentlist tag conf link \
>  			-foreground blue \
>  			-underline 1

Natural questions that come to mind after seeing the above are like these:

 - Does maxrecent still get used after this change?

 - Does the remainder of the code get upset if -height is set to too
   small (lenrecent might turn out to be 1, for example)?

And grepping for maxrecent shows there is _append_recentrepos that does this:

proc _append_recentrepos {path} {
	set path [file normalize $path]
	set recent [get_config gui.recentrepo]

	...
	if {[set maxrecent [get_config gui.maxrecentrepo]] eq {}} {
		set maxrecent 10
	}

	while {[llength $recent] > $maxrecent} {
		_unset_recentrepo [lindex $recent 0]
		set recent [lrange $recent 1 end]
	}
}

The last while loop looks to me like truncating the overlong recent list down to maxrecent. Perhaps a similar logic needs to be at the end of _get_recentrepos?