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

[Paul Mackerras <paulus@samba.org>]

On Tue, Jun 02, 2015 at 07:11:10AM -0400, Michael Rappazzo wrote:
> 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:

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