Re: [PATCH v2 1/2] gitk: alter the ordering for the "Tags and heads" view

From: Paul Mackerras <paulus@ozlabs.org>
Date: Mar 19, 2016, 06:53 UTC
Message-ID: <20160319065347.GC27126@fergus.ozlabs.ibm.com>
In-Reply-To: <1458327097-92126-2-git-send-email-rappazzo@gmail.com>

On Fri, Mar 18, 2016 at 02:51:36PM -0400, Michael Rappazzo wrote:

> In the "Tags and heads" view, the list of refs is globally sorted.
> Because of this, the list of local refs (heads) can be interrupted by the
> list of remote refs.  This change re-orders the view to be: local refs,
> remote refs tracked by local refs, remote refs, tags, and then other refs.

This seems like a nice idea, and could lead on to having a section that can be opened and closed for each of these categories. However, I have some comments on the implementation:

Show 29 quoted lines

> @@ -9933,35 +9933,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 refs [list $n H]
> +		lappend localrefs [list $n H]
> +		catch {set remote_name [exec git config --get branch.$n.remote]}
> +		if {$remote_name ne ""} {
> +		    catch {set remote_ref [exec git config --get branch.$n.merge]}
> +		    set remote_ref [string map {"refs/heads/" ""} $remote_ref]
> +		    set locally_tracked_remote_ref "remotes/$remote_name/$remote_ref"
> +		    set upstream_exists ""
> +		    catch {set upstream_exists [exec git rev-parse --verify $locally_tracked_remote_ref]}
> +		    if {$upstream_exists ne ""} {
> +			if {[lsearch $locally_tracked_remote_refs [list $locally_tracked_remote_ref H]] < 0} {
> +			    lappend locally_tracked_remote_refs [list $locally_tracked_remote_ref H]
> +			}
> +		    }
> +		}

I'm worried about the number of external git invocations we're potentially doing here, and how long that would take when you have a lot of heads. Can we cache the results perhaps? Or is there a way to do a single git command and get a list of tracking branches with their remotes etc.?

Also, the default for lsearch is glob-style matching. It's unlikely that ref names would have any of *?[\ in them, but AFAIK it's not impossible. You probably want -exact in there.

Further, the kind of thing you are using lsearch for can often be done more efficiently using an array (which becomes essentially a hash table underneath).

Paul.