{"thread":{"id":"40983","subject":"git gui: fail on dual entries in recent repo list","startedAt":"2015-12-13T09:31:29Z","lastAt":"2015-12-17T07:45:14Z","messageCount":11,"participants":["Philip Oakley","Eric Sunshine"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"274344","messageId":"3453668A49C94C2AA39911FC594AE151@PhilipOakley","threadId":"40983","inReplyTo":null,"subject":"git gui: fail on dual entries in recent repo list","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":null,"receivedAt":"2015-12-13T09:31:29Z","isPatch":false,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"A user, Alexey A, reported\n(https://groups.google.com/forum/#!topic/git-users/msev4KsQGFc, Warning:\ngui.recentrepo has multiply values while executing) on the Git User list\nthat the Git Gui start-up could barf if there were duplicate entries in the\nuser's recent repo list within their --global .gitconfig, but found no fix \non\nthe internet.\n\nAlexey had posted a screenshot of the script error, which, with a little\ndigging, got down to the _unset_recentrepo procedure in the\nchoose_repository.tcl file where the line\nhttps://github.com/git/git/blob/master/git-gui/lib/choose_repository.tcl#L250\n\n git config --global --unset gui.recentrepo \"^$p\\$\"\n\nshould read\n\n git config --global --unset-all gui.recentrepo \"^$p\\$\"\n\nfor the case where duplicate entries are present.\n\nThis fix resolved Alexey's problem.\n\nSo far, so good.\n\n\nUnfortunately this doesn't explain how duplicate entries are created, and if\nthe fix will affect other code, which I need some help on.\n\nThe 'proc _append_recentrepos {path}' code\nhttps://github.com/git/git/blob/master/git-gui/lib/choose_repository.tcl#L254\nhas some code to avoid both duplicate entries and to limit the number of\nentries in the recent repo list.\n\nA web search to understand the code got me to the lreplace function\n(http://wiki.tcl.tk/1485 #Performance: Modifying a List In-Place), which\nsuggest that the de-dup may (randomly) fail in some cases. In addition it\nlooks like the code may fail if there are multiple entries returned from the\nearlier search of the recentrepo list, whereby the following lreplace would\nhave more args in the arg list (4 rather than two) and so confusing it,\nthough my ignorance of tcl precedes me here.\n\nEven if all that tcl works fine, my proposed update will still have issues\nfor that code, because the append_recentrepos de-dup code keeps its own list\nof the recent repos, and thinks that it removed entries one at a time, yet\nthe change can remove multiple entries if duplicates are present, completely\nconfusing the counting of entries in the local copy '$recent'.\n\n\nSo, have I understood the code correctly? Are more fixes needed for the\nappend_recentrepos code to refactor the re-ordering of the recent repos list\nand limiting the list to 10 entries once the --unset-all fix is added?\n--\n\nPhilip\n"},{"id":"274375","messageId":"1450105743-2432-1-git-send-email-philipoakley@iee.org","threadId":"40983","inReplyTo":"3453668A49C94C2AA39911FC594AE151@PhilipOakley","subject":"[PATCH v1 0/4] Fix git-gui when recentrepo list has duplicates","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":"2015-12-14T15:08:59Z","receivedAt":"2015-12-14T15:08:59Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"This is the patch series which follows from a user report of not being\nable to start the git-gui when it contained duplicate entries.\n\nThe git gui design assumes that there will never be duplicate entries\nin the recent repo list, and attempts to keep it that way.\n\nFor reasons unknown (other applications or tcl bugs?) there are cases\nwhere the global .gitconfig does contain duplicate entries in the\ngui.recentrepo config var. contrary to expectation.\n\nThe patch series fixes the root of the issue first, then patches the\nlogic for the two functional usages of _unset_recentrepo.\n\nFinally, the displayable recentrepos list region is expanded to allow\nextended recentrepo lists to at least be shown, before the git-gui\nwrangles then back down to the allowed maxrecent size.\n\nPhilip Oakley (4):\n git-gui: remove duplicate entries from .gitconfig's gui.recentrepo\n git gui: cope with duplicates in _get_recentrepo\n git gui: de-dup selected repo from recentrepo history\n git gui: allow for a long recentrepo list\n\n git-gui/lib/choose_repository.tcl | 13 ++++++-------\n 1 file changed, 6 insertions(+), 7 deletions(-)\n\n-- \n2.5.2.windows.2\n"},{"id":"274377","messageId":"1450105743-2432-2-git-send-email-philipoakley@iee.org","threadId":"40983","inReplyTo":"1450105743-2432-1-git-send-email-philipoakley@iee.org","subject":"[PATCH v1 1/4] git-gui: remove duplicate entries from .gitconfig's gui.recentrepo","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":"2015-12-14T15:09:00Z","receivedAt":"2015-12-14T15:09:00Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"The git gui's recent repo list may become contaminated with duplicate\nentries. The git gui would barf when attempting to remove one entry.\nRemove them all - there is no option within 'git config' to selectively\nremove one of the entries.\n\nThis issue was reported on the 'Git User' list\n(https://groups.google.com/forum/#!topic/git-users/msev4KsQGFc,\nWarning: gui.recentrepo has multiply values while executing).\n\nOn startup the gui checks that entries in the recentrepo list are still\nvalid repos and deletes thoses that are not. If duplicate entries are\npresent the 'git config --unset' will barf and this prevents the gui\nfrom starting.\n\nSubsequent patches fix other parts of recentrepo logic used for syncing\ninternal lists with the external .gitconfig.\n\nReported-by: Alexey Astakhov \nSigned-off-by: Philip Oakley \n---\n git-gui/lib/choose_repository.tcl | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/git-gui/lib/choose_repository.tcl b/git-gui/lib/choose_repository.tcl\nindex 75d1da8..133ca0a 100644\n--- a/git-gui/lib/choose_repository.tcl\n+++ b/git-gui/lib/choose_repository.tcl\n@@ -247,7 +247,7 @@ proc _get_recentrepos {} {\n \n proc _unset_recentrepo {p} {\n \tregsub -all -- {([()\\[\\]{}\\.^$+*?\\\\])} $p {\\\\\\1} p\n-\tgit config --global --unset gui.recentrepo \"^$p\\$\"\n+\tgit config --global --unset-all gui.recentrepo \"^$p\\$\"\n \tload_config 1\n }\n \n-- \n2.5.2.windows.2\n"},{"id":"274376","messageId":"1450105743-2432-3-git-send-email-philipoakley@iee.org","threadId":"40983","inReplyTo":"1450105743-2432-1-git-send-email-philipoakley@iee.org","subject":"[PATCH v1 2/4] git gui: cope with duplicates in _get_recentrepo","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":"2015-12-14T15:09:01Z","receivedAt":"2015-12-14T15:09:01Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"_get_recentrepo will fail if duplicate invalid entries are present\nin the recentrepo config list. The previous commit fixed the\n'git config' limitations in _unset_recentrepo by unsetting all config\nentries, however this code would fail on the second attempt to unset it.\n\nRefactor the code to pre-sort and de-duplicate the recentrepo list to\navoid a potential second unset attempt.\n\nSigned-off-by: Philip Oakley \n---\n git-gui/lib/choose_repository.tcl | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/git-gui/lib/choose_repository.tcl b/git-gui/lib/choose_repository.tcl\nindex 133ca0a..aa87bcc 100644\n--- a/git-gui/lib/choose_repository.tcl\n+++ b/git-gui/lib/choose_repository.tcl\n@@ -235,14 +235,14 @@ method _invoke_next {} {\n \n proc _get_recentrepos {} {\n \tset recent [list]\n-\tforeach p [get_config gui.recentrepo] {\n+\tforeach p [lsort -unique [get_config gui.recentrepo]] {\n \t\tif {[_is_git [file join $p .git]]} {\n \t\t\tlappend recent $p\n \t\t} else {\n \t\t\t_unset_recentrepo $p\n \t\t}\n \t}\n-\treturn [lsort $recent]\n+\treturn $recent\n }\n \n proc _unset_recentrepo {p} {\n-- \n2.5.2.windows.2\n"},{"id":"274374","messageId":"1450105743-2432-4-git-send-email-philipoakley@iee.org","threadId":"40983","inReplyTo":"1450105743-2432-1-git-send-email-philipoakley@iee.org","subject":"[PATCH v1 3/4] git gui: de-dup selected repo from recentrepo history","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":"2015-12-14T15:09:02Z","receivedAt":"2015-12-14T15:09:02Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"When the gui/user selects a repo for display, that repo is brought to\nthe end of the recentrepo config list. The logic can fail if there are\nduplicate old entries for the repo (you cannot unset a single config\nentry when duplicates are present).\n\nSimilarly, the maxrecentrepo logic could fail if older duplicate entries\nare present.\n\nThe first commit of this series ({this}~2) fixed the config unsetting\nissue. Rather than manipulating a local copy of the $recent list (one\ncannot know how many entries were removed), simply re-read it.\n\nSigned-off-by: Philip Oakley \n---\n git-gui/lib/choose_repository.tcl | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/git-gui/lib/choose_repository.tcl b/git-gui/lib/choose_repository.tcl\nindex aa87bcc..ad7a888 100644\n--- a/git-gui/lib/choose_repository.tcl\n+++ b/git-gui/lib/choose_repository.tcl\n@@ -262,12 +262,11 @@ proc _append_recentrepos {path} {\n \tset i [lsearch $recent $path]\n \tif {$i >= 0} {\n \t\t_unset_recentrepo $path\n-\t\tset recent [lreplace $recent $i $i]\n \t}\n \n-\tlappend recent $path\n \tgit config --global --add gui.recentrepo $path\n \tload_config 1\n+\tset recent [get_config gui.recentrepo]\n \n \tif {[set maxrecent [get_config gui.maxrecentrepo]] eq {}} {\n \t\tset maxrecent 10\n@@ -275,7 +274,7 @@ proc _append_recentrepos {path} {\n \n \twhile {[llength $recent] > $maxrecent} {\n \t\t_unset_recentrepo [lindex $recent 0]\n-\t\tset recent [lrange $recent 1 end]\n+\t\tset recent [get_config gui.recentrepo]\n \t}\n }\n \n-- \n2.5.2.windows.2\n"},{"id":"274378","messageId":"1450105743-2432-5-git-send-email-philipoakley@iee.org","threadId":"40983","inReplyTo":"1450105743-2432-1-git-send-email-philipoakley@iee.org","subject":"[PATCH v1 4/4] git gui: allow for a long recentrepo list","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":"2015-12-14T15:09:03Z","receivedAt":"2015-12-14T15:09:03Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"The gui.recentrepo list may be longer than the maxrecent setting.\nAllow extra space to show any extra entries.\n\nIn an ideal world, the git gui would limit the number of entries\nto the maxrecent setting, however the recentrepo config list may\nhave been extended outwith the gui, or the maxrecent setting changed\nto a reduced value. Further, when testing the gui's recentrepo\nlogic it is useful to show these extra, but valid, entries.\n\nSigned-off-by: Philip Oakley \n---\n git-gui/lib/choose_repository.tcl | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/git-gui/lib/choose_repository.tcl b/git-gui/lib/choose_repository.tcl\nindex ad7a888..a08ed4f 100644\n--- a/git-gui/lib/choose_repository.tcl\n+++ b/git-gui/lib/choose_repository.tcl\n@@ -153,7 +153,7 @@ constructor pick {} {\n \t\t\t-background [get_bg_color $w_body.recentlabel] \\\n \t\t\t-wrap none \\\n \t\t\t-width 50 \\\n-\t\t\t-height $maxrecent\n+\t\t\t-height [expr {$maxrecent + 5}]\n \t\t$w_recentlist tag conf link \\\n \t\t\t-foreground blue \\\n \t\t\t-underline 1\n-- \n2.5.2.windows.2\n"},{"id":"274445","messageId":"CAPig+cQ8=WKwew5GNqyAPFuYJDgM+ae6rxaJQg=jhrxn61rNbQ@mail.gmail.com","threadId":"40983","inReplyTo":"1450105743-2432-2-git-send-email-philipoakley@iee.org","subject":"Re: [PATCH v1 1/4] git-gui: remove duplicate entries from .gitconfig's gui.recentrepo","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-12-14T22:31:22Z","receivedAt":"2015-12-14T22:31:22Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Monday, December 14, 2015, Philip Oakley wrote:\n> The git gui's recent repo list may become contaminated with duplicate\n> entries. The git gui would barf when attempting to remove one entry.\n> Remove them all - there is no option within 'git config' to selectively\n> remove one of the entries.\n>\n> This issue was reported on the 'Git User' list\n> (https://groups.google.com/forum/#!topic/git-users/msev4KsQGFc,\n> Warning: gui.recentrepo has multiply values while executing).\n\ns/multiply/multiple/\n\n> On startup the gui checks that entries in the recentrepo list are still\n> valid repos and deletes thoses that are not. If duplicate entries are\n\ns/thoses/those/\n\n> present the 'git config --unset' will barf and this prevents the gui\n\ns/present the/present, then/\n\n> from starting.\n>\n> Subsequent patches fix other parts of recentrepo logic used for syncing\n> internal lists with the external .gitconfig.\n>\n> Reported-by: Alexey Astakhov \n> Signed-off-by: Philip Oakley \n> ---\n> diff --git a/git-gui/lib/choose_repository.tcl b/git-gui/lib/choose_repository.tcl\n> index 75d1da8..133ca0a 100644\n> --- a/git-gui/lib/choose_repository.tcl\n> +++ b/git-gui/lib/choose_repository.tcl\n> @@ -247,7 +247,7 @@ proc _get_recentrepos {} {\n>\n> proc _unset_recentrepo {p} {\n> regsub -all -- {([()\\[\\]{}\\.^$+*?\\\\])} $p {\\\\\\1} p\n> - git config --global --unset gui.recentrepo \"^$p\\$\"\n> + git config --global --unset-all gui.recentrepo \"^$p\\$\"\n> load_config 1\n> }\n>\n> --\n> 2.5.2.windows.2\n"},{"id":"274447","messageId":"CAPig+cSE41rHyq8hNcEL+qJaZrSwp0uc=Y6gPiRcj_T1EgQ4cg@mail.gmail.com","threadId":"40983","inReplyTo":"1450105743-2432-5-git-send-email-philipoakley@iee.org","subject":"Re: [PATCH v1 4/4] git gui: allow for a long recentrepo list","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-12-14T22:36:06Z","receivedAt":"2015-12-14T22:36:06Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Dec 14, 2015 at 10:09 AM, Philip Oakley wrote:\n> The gui.recentrepo list may be longer than the maxrecent setting.\n> Allow extra space to show any extra entries.\n>\n> In an ideal world, the git gui would limit the number of entries\n> to the maxrecent setting, however the recentrepo config list may\n> have been extended outwith the gui, or the maxrecent setting changed\n\ns/outwith/without/\n\n> to a reduced value. Further, when testing the gui's recentrepo\n> logic it is useful to show these extra, but valid, entries.\n>\n> Signed-off-by: Philip Oakley \n> ---\n> diff --git a/git-gui/lib/choose_repository.tcl b/git-gui/lib/choose_repository.tcl\n> index ad7a888..a08ed4f 100644\n> --- a/git-gui/lib/choose_repository.tcl\n> +++ b/git-gui/lib/choose_repository.tcl\n> @@ -153,7 +153,7 @@ constructor pick {} {\n> -background [get_bg_color $w_body.recentlabel] \\\n> -wrap none \\\n> -width 50 \\\n> - -height $maxrecent\n> + -height [expr {$maxrecent + 5}]\n> $w_recentlist tag conf link \\\n> -foreground blue \\\n> -underline 1\n> --\n> 2.5.2.windows.2\n"},{"id":"274469","messageId":"71B00739215A4176BA65000AF1CBBBD4@PhilipOakley","threadId":"40983","inReplyTo":"CAPig+cSE41rHyq8hNcEL+qJaZrSwp0uc=Y6gPiRcj_T1EgQ4cg@mail.gmail.com","subject":"Re: [PATCH v1 4/4] git gui: allow for a long recentrepo list","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":null,"receivedAt":"2015-12-14T23:57:37Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Eric Sunshine\" \n> On Mon, Dec 14, 2015 at 10:09 AM, Philip Oakley \n> wrote:\n>> The gui.recentrepo list may be longer than the maxrecent setting.\n>> Allow extra space to show any extra entries.\n>>\n>> In an ideal world, the git gui would limit the number of entries\n>> to the maxrecent setting, however the recentrepo config list may\n>> have been extended outwith the gui, or the maxrecent setting changed\n>\n> s/outwith/without/\n\nAh, it's a Scottish english word, which better 'translates' as as outside, \nrather than being a commutation ;-)\n\nI'll see if there are any other comments before a re-roll.\n\n>\n>> to a reduced value. Further, when testing the gui's recentrepo\n>> logic it is useful to show these extra, but valid, entries.\n>>\n>> Signed-off-by: Philip Oakley \n>> ---\n>> diff --git a/git-gui/lib/choose_repository.tcl \n>> b/git-gui/lib/choose_repository.tcl\n>> index ad7a888..a08ed4f 100644\n>> --- a/git-gui/lib/choose_repository.tcl\n>> +++ b/git-gui/lib/choose_repository.tcl\n>> @@ -153,7 +153,7 @@ constructor pick {} {\n>> -background [get_bg_color $w_body.recentlabel] \\\n>> -wrap none \\\n>> -width 50 \\\n>> - -height $maxrecent\n>> + -height [expr {$maxrecent + 5}]\n>> $w_recentlist tag conf link \\\n>> -foreground blue \\\n>> -underline 1\n>> --\n>> 2.5.2.windows.2\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n> \n"},{"id":"274634","messageId":"B283141DAA2647EF9917F27A817CC7FA@PhilipOakley","threadId":"40983","inReplyTo":"CAPig+cQ8=WKwew5GNqyAPFuYJDgM+ae6rxaJQg=jhrxn61rNbQ@mail.gmail.com","subject":"Re: [PATCH v1 1/4] git-gui: remove duplicate entries from .gitconfig's gui.recentrepo","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":null,"receivedAt":"2015-12-16T22:46:23Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Eric Sunshine\" \n> On Monday, December 14, 2015, Philip Oakley wrote:\n>> The git gui's recent repo list may become contaminated with duplicate\n>> entries. The git gui would barf when attempting to remove one entry.\n>> Remove them all - there is no option within 'git config' to selectively\n>> remove one of the entries.\n>>\n>> This issue was reported on the 'Git User' list\n>> (https://groups.google.com/forum/#!topic/git-users/msev4KsQGFc,\n>> Warning: gui.recentrepo has multiply values while executing).\n>\n> s/multiply/multiple/\n\nThe original report has that mistake. I'll quote the thread title.\n>\n>> On startup the gui checks that entries in the recentrepo list are still\n>> valid repos and deletes thoses that are not. If duplicate entries are\n>\n> s/thoses/those/\nOK\n\n>\n>> present the 'git config --unset' will barf and this prevents the gui\n>\n> s/present the/present, then/\n\nOK\n[my first glance thought the change was to the 'prevents the' at the end...]\n>\n>> from starting.\n>>\n>> Subsequent patches fix other parts of recentrepo logic used for syncing\n>> internal lists with the external .gitconfig.\n>>\n>> Reported-by: Alexey Astakhov \n>> Signed-off-by: Philip Oakley \n>> ---\n>> diff --git a/git-gui/lib/choose_repository.tcl \n>> b/git-gui/lib/choose_repository.tcl\n>> index 75d1da8..133ca0a 100644\n>> --- a/git-gui/lib/choose_repository.tcl\n>> +++ b/git-gui/lib/choose_repository.tcl\n>> @@ -247,7 +247,7 @@ proc _get_recentrepos {} {\n>>\n>> proc _unset_recentrepo {p} {\n>> regsub -all -- {([()\\[\\]{}\\.^$+*?\\\\])} $p {\\\\\\1} p\n>> - git config --global --unset gui.recentrepo \"^$p\\$\"\n>> + git config --global --unset-all gui.recentrepo \"^$p\\$\"\n>> load_config 1\n>> }\n--\n\nWill re-roll to give Pat the opportunity to include in any 2.7 update.\n\nPhilip \n"},{"id":"274651","messageId":"CAPig+cQd2f7zHJAwS29-zuL3xvC2thu9pvU-i1GXVTm_3E6rAQ@mail.gmail.com","threadId":"40983","inReplyTo":"B283141DAA2647EF9917F27A817CC7FA@PhilipOakley","subject":"Re: [PATCH v1 1/4] git-gui: remove duplicate entries from .gitconfig's gui.recentrepo","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-12-17T07:45:14Z","receivedAt":"2015-12-17T07:45:14Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Dec 16, 2015 at 6:41 PM, Philip Oakley wrote:\n> From: \"Eric Sunshine\" \n>> On Monday, December 14, 2015, Philip Oakley wrote:\n>>> The git gui's recent repo list may become contaminated with duplicate\n>>> entries. The git gui would barf when attempting to remove one entry.\n>>> Remove them all - there is no option within 'git config' to selectively\n>>> remove one of the entries.\n>>>\n>>> This issue was reported on the 'Git User' list\n>>> (https://groups.google.com/forum/#!topic/git-users/msev4KsQGFc,\n>>> Warning: gui.recentrepo has multiply values while executing).\n>>\n>> s/multiply/multiple/\n>\n> The original report has that mistake. I'll quote the thread title.\n\nAh, I had checked the screenshot included with the original report and\nsaw that it was correctly spelled, thus assumed that the typo was\nyours. I didn't pay much attention to the title in the original\nreport, which indeed already contains this misspelling.\n"}]}