git gui: fail on dual entries in recent repo list

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

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(-)

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(-)

On Monday, December 14, 2015, Philip Oakley <philipoakley@iee.org> wrote:

s/multiply/multiple/

s/thoses/those/

s/present the/present, then/

From: "Eric Sunshine" <sunshine@sunshineco.com>

The original report has that mistake. I'll quote the thread title.

OK

OK [my first glance thought the change was to the 'prevents the' at the end...]

--

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

Philip 

On Wed, Dec 16, 2015 at 6:41 PM, Philip Oakley <philipoakley@iee.org> wrote:

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.

_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(-)

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(-)

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(-)

On Mon, Dec 14, 2015 at 10:09 AM, Philip Oakley <philipoakley@iee.org> wrote:

s/outwith/without/

From: "Eric Sunshine" <sunshine@sunshineco.com>

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.