Re: [PATCH] gitk: Alter the ordering for the "Tags and heads" view

From: Paul Mackerras <paulus@samba.org>
Date: Aug 13, 2015, 00:43 UTC
Message-ID: <20150813004302.GB6686@fergus.ozlabs.ibm.com>
In-Reply-To: <1433243470-93392-2-git-send-email-rappazzo@gmail.com>

On Tue, Jun 02, 2015 at 07:11:10AM -0400, Michael Rappazzo wrote:

Show 6 quoted lines

> In the "Tags and heads" view, the list of refs is globally sorted.
> The list of local refs (heads) is separated by the remote refs.  This
> change re-orders the view toi be: local refs, remote refs tracked by
> local refs, remote refs, tags, and then other refs
> 
> Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>

Sorry it's taken me so long to get around to reviewing this. I have a couple of comments:

Show 25 quoted lines

> ---
>  gitk-git/gitk | 48 ++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 42 insertions(+), 6 deletions(-)
> 
> diff --git a/gitk-git/gitk b/gitk-git/gitk
> index 9a2daf3..431a6a1 100755
> --- a/gitk-git/gitk
> +++ b/gitk-git/gitk
> @@ -9879,35 +9879,71 @@ proc refill_reflist {} {
>      global curview
>  
>      if {![info exists showrefstop] || ![winfo exists $showrefstop]} return
> -    set refs {}
> +    set localrefs {}
> +    set remoterefs {}
> +    set locally_tracked_remote_refs {}
> +    set tagrefs {}
> +    set otherrefs {}
>      foreach n [array names headids] {
> -	if {[string match $reflistfilter $n]} {
> +	if {![string match "remotes/*" $n] && [string match $reflistfilter $n]} {
> +	    if {[commitinview $headids($n) $curview]} {
> +		lappend localrefs [list $n H]
> +		catch {set remote_name [exec git config --get branch.$n.remote]}
> +		if {$remote_name ne ""} {

First off, if the git config command fails for any reason and returns an error status, the set command won't get done and $remote_name will either be undefined or will have whatever value it had before. If it is undefined then the if statement is going to throw an error. I don't think that is what you meant to happen. This same problem will occur for other variables such as $remote_ref and $exists.

Secondly, I'm not very happy about doing all these external git commands every time we run refill_reflist. Couldn't we cache which remote each local branch is tracking? We would then throw away and reload the cache in rereadrefs. Most executions of refill_reflist would then not need to do any external git commands at all.

Paul.