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

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

[Philip Oakley]

Way back in December 2015 I made a couple of attempts to patch up
the git-gui's recentrepo list in the face of duplicate entries in
the .gitconfig

The series end at http://public-inbox.org/git/9731888BD4C348F5BFC82FA96D978034@PhilipOakley/

A similar problem was reported recently on the Git for Windows list
https://github.com/git-for-windows/git/issues/1014 were a full
recentrepo list also stopped the gui working.

This series applies to Pat Thoyt's upstream Github repo as a merge
ready Pull Request https://github.com/patthoyts/git-gui/pull/10.
Hence if applied here it should be into the sub-tree git/git-gui.

I believe I've covered the points raised by Junio in the last review
and allowed for cases where the recentrepo list is now longer than the
maximum (which can be configured, both up and down).

I've cc'd just the cover letter to those involved previously, rather
than the series to avoid spamming.

There are various other low hanging fruit that the eager could look at
which are detailed at the end of the GfW/issues/104 referenced above.

Philip Oakley (4):
  git-gui: remove duplicate entries from .gitconfig's gui.recentrepo
  git gui: cope with duplicates in _get_recentrepo
  git gui: de-dup selected repo from recentrepo history
  git gui: allow for a long recentrepo list

 lib/choose_repository.tcl | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

-- 
2.9.0.windows.1.323.g0305acf
---
cc: Junio C Hamano <gitster@pobox.com>
cc: Pat Thoyts <patthoyts@users.sourceforge.net>,
cc: Eric Sunshine <sunshine@sunshineco.com>,
cc: Alexey Astakhov <asstv7@gmail.com>
cc: Johannes Schindelin <johannes.schindelin@gmx.de>

[PATCH v3 1/4] git-gui: remove duplicate entries from .gitconfig's gui.recentrepo

[Philip Oakley]

The git gui's recent repo list may become contaminated with duplicate
entries. The git gui would barf when attempting to remove one entry.
Remove them all - there is no option within 'git config' to selectively
remove one of the entries.

This issue was reported on the 'Git User' list
(https://groups.google.com/forum/#!topic/git-users/msev4KsQGFc,
Warning: gui.recentrepo has multiply values while executing).

And also by zosrothko as a Git-for-Windows issue
https://github.com/git-for-windows/git/issues/1014.

On startup the gui checks that entries in the recentrepo list are still
valid repos and deletes thoses that are not. If duplicate entries are
present the 'git config --unset' will barf and this prevents the gui
from starting.

Subsequent patches fix other parts of recentrepo logic used for syncing
internal lists with the external .gitconfig.

Reported-by: Alexey Astakhov <asstv7@gmail.com>
Signed-off-by: Philip Oakley <philipoakley@iee.org>
---
 lib/choose_repository.tcl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/choose_repository.tcl b/lib/choose_repository.tcl
index 75d1da8..133ca0a 100644
--- a/lib/choose_repository.tcl
+++ b/lib/choose_repository.tcl
@@ -247,7 +247,7 @@ proc _get_recentrepos {} {
 
 proc _unset_recentrepo {p} {
 	regsub -all -- {([()\[\]{}\.^$+*?\\])} $p {\\\1} p
-	git config --global --unset gui.recentrepo "^$p\$"
+	git config --global --unset-all gui.recentrepo "^$p\$"
 	load_config 1
 }
 
-- 
2.9.0.windows.1.323.g0305acf

[PATCH v3 2/4] git gui: cope with duplicates in _get_recentrepo

[Philip Oakley]

_get_recentrepo will fail if duplicate invalid entries are present
in the recentrepo config list. The previous commit fixed the
'git config' limitations in _unset_recentrepo by unsetting all config
entries, however this code would fail on the second attempt to unset it.

Refactor the code to pre-sort and de-duplicate the recentrepo list to
avoid a potential second unset attempt.

Signed-off-by: Philip Oakley <philipoakley@iee.org>
---
 lib/choose_repository.tcl | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/choose_repository.tcl b/lib/choose_repository.tcl
index 133ca0a..aa87bcc 100644
--- a/lib/choose_repository.tcl
+++ b/lib/choose_repository.tcl
@@ -235,14 +235,14 @@ method _invoke_next {} {
 
 proc _get_recentrepos {} {
 	set recent [list]
-	foreach p [get_config gui.recentrepo] {
+	foreach p [lsort -unique [get_config gui.recentrepo]] {
 		if {[_is_git [file join $p .git]]} {
 			lappend recent $p
 		} else {
 			_unset_recentrepo $p
 		}
 	}
-	return [lsort $recent]
+	return $recent
 }
 
 proc _unset_recentrepo {p} {
-- 
2.9.0.windows.1.323.g0305acf

[PATCH v3 3/4] git gui: de-dup selected repo from recentrepo history

[Philip Oakley]

When the gui/user selects a repo for display, that repo is brought to
the end of the recentrepo config list. The logic can fail if there are
duplicate old entries for the repo (you cannot unset a single config
entry when duplicates are present).

Similarly, the maxrecentrepo logic could fail if older duplicate entries
are present.

The first commit of this series ({this}~2) fixed the config unsetting
issue. Rather than manipulating a local copy of the $recent list (one
cannot know how many entries were removed), simply re-read it.

We must also catch the error when the attempt to remove the second copy
from the re-read list is performed.

Signed-off-by: Philip Oakley <philipoakley@iee.org>
---
 lib/choose_repository.tcl | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/lib/choose_repository.tcl b/lib/choose_repository.tcl
index aa87bcc..f39636f 100644
--- a/lib/choose_repository.tcl
+++ b/lib/choose_repository.tcl
@@ -247,7 +247,7 @@ proc _get_recentrepos {} {
 
 proc _unset_recentrepo {p} {
 	regsub -all -- {([()\[\]{}\.^$+*?\\])} $p {\\\1} p
-	git config --global --unset-all gui.recentrepo "^$p\$"
+	catch {git config --global --unset-all gui.recentrepo "^$p\$"}
 	load_config 1
 }
 
@@ -262,12 +262,11 @@ proc _append_recentrepos {path} {
 	set i [lsearch $recent $path]
 	if {$i >= 0} {
 		_unset_recentrepo $path
-		set recent [lreplace $recent $i $i]
 	}
 
-	lappend recent $path
 	git config --global --add gui.recentrepo $path
 	load_config 1
+	set recent [get_config gui.recentrepo]
 
 	if {[set maxrecent [get_config gui.maxrecentrepo]] eq {}} {
 		set maxrecent 10
@@ -275,7 +274,7 @@ proc _append_recentrepos {path} {
 
 	while {[llength $recent] > $maxrecent} {
 		_unset_recentrepo [lindex $recent 0]
-		set recent [lrange $recent 1 end]
+		set recent [get_config gui.recentrepo]
 	}
 }
 
-- 
2.9.0.windows.1.323.g0305acf

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

[Philip Oakley]

The gui.recentrepo list may be longer than the maxrecent setting.
Allow extra space to show any extra entries.

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

Signed-off-by: Philip Oakley <philipoakley@iee.org>
---
 lib/choose_repository.tcl | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/lib/choose_repository.tcl b/lib/choose_repository.tcl
index f39636f..80f5a59 100644
--- a/lib/choose_repository.tcl
+++ b/lib/choose_repository.tcl
@@ -142,6 +142,10 @@ constructor pick {} {
 				-label [mc "Recent Repositories"]
 		}
 
+	if {[set lenrecent [llength $sorted_recent]] < $maxrecent} {
+		set lenrecent $maxrecent
+	}
+
 		${NS}::label $w_body.space
 		${NS}::label $w_body.recentlabel \
 			-anchor w \
@@ -153,7 +157,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
-- 
2.9.0.windows.1.323.g0305acf

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

[Philip Oakley]

Ping

Any comments anyone?
(https://public-inbox.org/git/20170122195301.1784-1-philipoakley@iee.org/)

I understand that the Git-for-Windows team is planning to include this in 
their next release, so additional eyes are welcomed.

Philip

From: "Philip Oakley" <philipoakley@iee.org> date: Sunday, January 22, 2017 
7:52 PM
> Way back in December 2015 I made a couple of attempts to patch up
> the git-gui's recentrepo list in the face of duplicate entries in
> the .gitconfig
>
> The series end at 
> http://public-inbox.org/git/9731888BD4C348F5BFC82FA96D978034@PhilipOakley/
>
> A similar problem was reported recently on the Git for Windows list
> https://github.com/git-for-windows/git/issues/1014 were a full
> recentrepo list also stopped the gui working.
>
> This series applies to Pat Thoyt's upstream Github repo as a merge
> ready Pull Request https://github.com/patthoyts/git-gui/pull/10.
> Hence if applied here it should be into the sub-tree git/git-gui.
>
> I believe I've covered the points raised by Junio in the last review
> and allowed for cases where the recentrepo list is now longer than the
> maximum (which can be configured, both up and down).
>
> I've cc'd just the cover letter to those involved previously, rather
> than the series to avoid spamming.
>
> There are various other low hanging fruit that the eager could look at
> which are detailed at the end of the GfW/issues/104 referenced above.
>
> Philip Oakley (4):
>  git-gui: remove duplicate entries from .gitconfig's gui.recentrepo
>  git gui: cope with duplicates in _get_recentrepo
>  git gui: de-dup selected repo from recentrepo history
>  git gui: allow for a long recentrepo list
>
> lib/choose_repository.tcl | 17 ++++++++++-------
> 1 file changed, 10 insertions(+), 7 deletions(-)
>
> -- 
> 2.9.0.windows.1.323.g0305acf
> ---
> cc: Junio C Hamano <gitster@pobox.com>
> cc: Pat Thoyts <patthoyts@users.sourceforge.net>,
> cc: Eric Sunshine <sunshine@sunshineco.com>,
> cc: Alexey Astakhov <asstv7@gmail.com>
> cc: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> 

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

[Philip Oakley]

From: "Philip Oakley" <philipoakley@iee.org>
> From: "Junio C Hamano" <gitster@pobox.com>, December 17, 2015 7:32 PM
>> Philip Oakley <philipoakley@iee.org> writes:
>>
>>> The gui.recentrepo list may be longer than the maxrecent setting.
>>> Allow extra space to show any extra entries.
>>>
>>> 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.
>>
>> I do not have a strong objection either way, but the magic number 5
>> smells like an indication that this is solving a wrong problem or
>> solving a problem in a wrong approach.  What happens to the entries
>> beyond $maxrecent-th in the recent list with the current code?  The
>> list effectively truncated and not shown?  Wouldn't the same thing
>> happen to the entries beyond ($maxrecent+5)-th in the recent list
>> with your patch?
>
> True, it was a magic number selected as being a moderate overspill 
> allowance.
> I tried 3 and 5 as possible values, and 5 didn't look ungainly, even on my 
> small netbook, and covered all my test hacking cases.
>
> The issue was that the prior code assumed that the fault could never 
> happen and limited the display, which is sorted, to the max value, so in a 
> fault condition the most recent repo could fall off the bottom.
>
> As a bit of 'monkey patching' I wasn't sure how the dialog creation could 
> know in advance how long the list would be, so I simply added on a small 
> margin.
>
>>
>> Truncating the list at $maxrecent-th to match the display
>> (i.e. declare that "$maxrecent" is really the max number of entries
>> we would keep track of), or allowing the UI to scroll so that
>> entries beyond $maxrecent-th one can be shown, perhaps?
>
> This would certainly be a reasonable approach, but I don't know enough of 
> the tcl to know how to make it happen. If anyone has suggestions...

I think I see the way to do it. V3 Patch 4/4 to follow.
>
>>
>>>
>>> Signed-off-by: Philip Oakley <philipoakley@iee.org>
>>> ---
>>> word usage corrected.
>>> Eric's comments $gmane/282432
>>> ---
>>>  git-gui/lib/choose_repository.tcl | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/git-gui/lib/choose_repository.tcl 
>>> b/git-gui/lib/choose_repository.tcl
>>> index ad7a888..a08ed4f 100644
>>> --- a/git-gui/lib/choose_repository.tcl
>>> +++ b/git-gui/lib/choose_repository.tcl
>>> @@ -153,7 +153,7 @@ constructor pick {} {
>>>  -background [get_bg_color $w_body.recentlabel] \
>>>  -wrap none \
>>>  -width 50 \
>>> - -height $maxrecent
>>> + -height [expr {$maxrecent + 5}]
>>>  $w_recentlist tag conf link \
>>>  -foreground blue \
>>>  -underline 1
>> --
>> To unsubscribe from this list: send the line "unsubscribe git" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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

[Philip Oakley]

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
-- 
2.6.4.windows.1

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

[Junio C Hamano]

Philip Oakley <philipoakley@iee.org> writes:

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

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

[Philip Oakley]

From: "Junio C Hamano" <gitster@pobox.com>
> Philip Oakley <philipoakley@iee.org> writes:
>
>> 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?

Yes, essentially as you describe....

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

No it's OK, the code has at least checked it is positive..

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

It does three things:
- removes the current (selected) repo from the list of past repos (and 
removes any duplicates)
- places the current repo at the end
- trims the list to back to max repos.

Nominally, it can't do anything about non-selected valid repos which may be 
listed in the config twice because git config won't selectively remove just 
one of two identical key/values, however in Patch 3 I do remove both older & 
newer entries and re-read the config (some may consider that a small 
regression).

It's not easy working around the contrary assumptions of the gui.recentrepo 
list (once only) and the reality of the .gitconfig (duplicates possible) and 
'git config's --unset/--unset-all limitation.

Exactly how those extra entries get there is an open question, but it does 
happen (so say's google, as well as the user report, I've seen it myself ;-)

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

Yes

>   Perhaps a similar logic needs to be at the
> end of _get_recentrepos?

The logic here is to show what is in the config to the user, *before* we 
trim the list, hence we wait until the user has selected a repo to open and 
do the purging in _append_recentrepos.

As an aside, there's also a long standing 'feature' that if one works both 
on and off network, then network repo paths get trimmed as soon as the user 
starts the gui while off-net (or the network goes down;-) because those 
recent repo paths become 'invalid' and are purged, but let's not go there.

--
Philip