git gui: fail on dual entries in recent repo list

git gui: fail on dual entries in recent repo list

[Philip Oakley]

A user, Alexey A, reported
(https://groups.google.com/forum/#!topic/git-users/msev4KsQGFc, Warning:
gui.recentrepo has multiply values while executing) on the Git User list
that the Git Gui start-up could barf if there were duplicate entries in the
user's recent repo list within their --global .gitconfig, but found no fix 
on
the internet.

Alexey had posted a screenshot of the script error, which, with a little
digging, got down to the _unset_recentrepo procedure in the
choose_repository.tcl file where the line
https://github.com/git/git/blob/master/git-gui/lib/choose_repository.tcl#L250

 git config --global --unset gui.recentrepo "^$p\$"

should read

 git config --global --unset-all gui.recentrepo "^$p\$"

for the case where duplicate entries are present.

This fix resolved Alexey's problem.

So far, so good.


Unfortunately this doesn't explain how duplicate entries are created, and if
the fix will affect other code, which I need some help on.

The 'proc _append_recentrepos {path}' code
https://github.com/git/git/blob/master/git-gui/lib/choose_repository.tcl#L254
has some code to avoid both duplicate entries and to limit the number of
entries in the recent repo list.

A web search to understand the code got me to the lreplace function
(http://wiki.tcl.tk/1485 #Performance: Modifying a List In-Place), which
suggest that the de-dup may (randomly) fail in some cases. In addition it
looks like the code may fail if there are multiple entries returned from the
earlier search of the recentrepo list, whereby the following lreplace would
have more args in the arg list (4 rather than two) and so confusing it,
though my ignorance of tcl precedes me here.

Even if all that tcl works fine, my proposed update will still have issues
for that code, because the append_recentrepos de-dup code keeps its own list
of the recent repos, and thinks that it removed entries one at a time, yet
the change can remove multiple entries if duplicates are present, completely
confusing the counting of entries in the local copy '$recent'.


So, have I understood the code correctly? Are more fixes needed for the
append_recentrepos code to refactor the re-ordering of the recent repos list
and limiting the list to 10 entries once the --unset-all fix is added?
--

Philip

[PATCH v1 0/4] Fix git-gui when recentrepo list has duplicates

[Philip Oakley]

This is the patch series which follows from a user report of not being
able to start the git-gui when it contained duplicate entries.

The git gui design assumes that there will never be duplicate entries
in the recent repo list, and attempts to keep it that way.

For reasons unknown (other applications or tcl bugs?) there are cases
where the global .gitconfig does contain duplicate entries in the
gui.recentrepo config var. contrary to expectation.

The patch series fixes the root of the issue first, then patches the
logic for the two functional usages of _unset_recentrepo.

Finally, the displayable recentrepos list region is expanded to allow
extended recentrepo lists to at least be shown, before the git-gui
wrangles then back down to the allowed maxrecent size.

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

 git-gui/lib/choose_repository.tcl | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

-- 
2.5.2.windows.2

[PATCH v1 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).

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>
---
 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 75d1da8..133ca0a 100644
--- a/git-gui/lib/choose_repository.tcl
+++ b/git-gui/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.5.2.windows.2

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

[Eric Sunshine]

On Monday, December 14, 2015, Philip Oakley <philipoakley@iee.org> wrote:
> 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).

s/multiply/multiple/

> 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

s/thoses/those/

> present the 'git config --unset' will barf and this prevents the gui

s/present the/present, then/

> 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>
> ---
> diff --git a/git-gui/lib/choose_repository.tcl b/git-gui/lib/choose_repository.tcl
> index 75d1da8..133ca0a 100644
> --- a/git-gui/lib/choose_repository.tcl
> +++ b/git-gui/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.5.2.windows.2

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

[Philip Oakley]

From: "Eric Sunshine" <sunshine@sunshineco.com>
> On Monday, December 14, 2015, Philip Oakley <philipoakley@iee.org> wrote:
>> 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).
>
> s/multiply/multiple/

The original report has that mistake. I'll quote the thread title.
>
>> 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
>
> s/thoses/those/
OK

>
>> present the 'git config --unset' will barf and this prevents the gui
>
> s/present the/present, then/

OK
[my first glance thought the change was to the 'prevents the' at the end...]
>
>> 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>
>> ---
>> diff --git a/git-gui/lib/choose_repository.tcl 
>> b/git-gui/lib/choose_repository.tcl
>> index 75d1da8..133ca0a 100644
>> --- a/git-gui/lib/choose_repository.tcl
>> +++ b/git-gui/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
>>  }
--

Will re-roll to give Pat the opportunity to include in any 2.7 update.

Philip 

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

[Eric Sunshine]

On Wed, Dec 16, 2015 at 6:41 PM, Philip Oakley <philipoakley@iee.org> wrote:
> From: "Eric Sunshine" <sunshine@sunshineco.com>
>> On Monday, December 14, 2015, Philip Oakley <philipoakley@iee.org> wrote:
>>> 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).
>>
>> s/multiply/multiple/
>
> The original report has that mistake. I'll quote the thread title.

Ah, I had checked the screenshot included with the original report and
saw that it was correctly spelled, thus assumed that the typo was
yours. I didn't pay much attention to the title in the original
report, which indeed already contains this misspelling.

[PATCH v1 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>
---
 git-gui/lib/choose_repository.tcl | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-gui/lib/choose_repository.tcl b/git-gui/lib/choose_repository.tcl
index 133ca0a..aa87bcc 100644
--- a/git-gui/lib/choose_repository.tcl
+++ b/git-gui/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.5.2.windows.2

[PATCH v1 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.

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

diff --git a/git-gui/lib/choose_repository.tcl b/git-gui/lib/choose_repository.tcl
index aa87bcc..ad7a888 100644
--- a/git-gui/lib/choose_repository.tcl
+++ b/git-gui/lib/choose_repository.tcl
@@ -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.5.2.windows.2

[PATCH v1 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>
---
 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
-- 
2.5.2.windows.2

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

[Eric Sunshine]

On Mon, Dec 14, 2015 at 10:09 AM, Philip Oakley <philipoakley@iee.org> wrote:
> 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

s/outwith/without/

> 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>
> ---
> 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
> --
> 2.5.2.windows.2

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

[Philip Oakley]

From: "Eric Sunshine" <sunshine@sunshineco.com>
> On Mon, Dec 14, 2015 at 10:09 AM, Philip Oakley <philipoakley@iee.org> 
> wrote:
>> 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
>
> s/outwith/without/

Ah, it's a Scottish english word, which better 'translates' as as outside, 
rather than being a commutation ;-)

I'll see if there are any other comments before a re-roll.

>
>> 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>
>> ---
>> 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
>> --
>> 2.5.2.windows.2
> --
> 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
>