{"thread":{"id":"41026","subject":"[PATCH v2 3/4] git gui: de-dup selected repo from recentrepo history","startedAt":"2015-12-16T23:58:03Z","lastAt":"2015-12-17T23:35:25Z","messageCount":11,"participants":["Philip Oakley","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":4},"messages":[{"id":"274637","messageId":"1450310287-4936-1-git-send-email-philipoakley@iee.org","threadId":"41026","inReplyTo":null,"subject":"[PATCH v2 0/4] Fix git-gui when recentrepo list has duplicates","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":"2015-12-16T23:58:03Z","receivedAt":"2015-12-16T23:58:03Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"git-gui won't start if either there are duplicate invalid entries or\nthe user selects a duplicated entry from the gui.recentrepo list. Such\nduplicates have been found in the wild.\n\nV2 corrects the spellings, grammar and quoting errors in the commit\nmessages as noted by Eric Sunshine ($gmane/282430 & $gmane/282432)\n\nAlso patch 1/4 title has been shortened\n\nThis re-roll will hopefully give Pat an opportunity to consider including\nthe fix into the git-gui for Git V2.7\n\nV1 series at $gmane/282360\n\n\nPhilip Oakley (4):\n git-gui: de-dup entries from 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.6.4.windows.1\n"},{"id":"274638","messageId":"1450310287-4936-2-git-send-email-philipoakley@iee.org","threadId":"41026","inReplyTo":"1450310287-4936-1-git-send-email-philipoakley@iee.org","subject":"[PATCH v2 1/4] git-gui: de-dup entries from gui.recentrepo","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":"2015-12-16T23:58:04Z","receivedAt":"2015-12-16T23:58:04Z","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,\n\"Warning: 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 those that are not. If duplicate entries are\npresent, then '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---\ntightened the patch title\nuser report thread title now quoted\nspelling corrected\nfixed grammar (comma)\nEric's comments $gmane/282430\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.6.4.windows.1\n"},{"id":"274640","messageId":"1450310287-4936-3-git-send-email-philipoakley@iee.org","threadId":"41026","inReplyTo":"1450310287-4936-1-git-send-email-philipoakley@iee.org","subject":"[PATCH v2 2/4] git gui: cope with duplicates in _get_recentrepo","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":"2015-12-16T23:58:05Z","receivedAt":"2015-12-16T23:58:05Z","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.6.4.windows.1\n"},{"id":"274636","messageId":"1450310287-4936-4-git-send-email-philipoakley@iee.org","threadId":"41026","inReplyTo":"1450310287-4936-1-git-send-email-philipoakley@iee.org","subject":"[PATCH v2 3/4] git gui: de-dup selected repo from recentrepo history","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":"2015-12-16T23:58:06Z","receivedAt":"2015-12-16T23:58:06Z","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.6.4.windows.1\n"},{"id":"274639","messageId":"1450310287-4936-5-git-send-email-philipoakley@iee.org","threadId":"41026","inReplyTo":"1450310287-4936-1-git-send-email-philipoakley@iee.org","subject":"[PATCH v2 4/4] git gui: allow for a long recentrepo list","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":"2015-12-16T23:58:07Z","receivedAt":"2015-12-16T23:58:07Z","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 outside of 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---\nword usage corrected.\nEric's comments $gmane/282432\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.6.4.windows.1\n"},{"id":"274684","messageId":"xmqqsi30c1lk.fsf@gitster.mtv.corp.google.com","threadId":"41026","inReplyTo":"1450310287-4936-5-git-send-email-philipoakley@iee.org","subject":"Re: [PATCH v2 4/4] git gui: allow for a long recentrepo list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-17T19:32:55Z","receivedAt":"2015-12-17T19:32:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Philip Oakley writes:\n\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 outside of the gui, or the maxrecent setting changed\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\nI do not have a strong objection either way, but the magic number 5\nsmells like an indication that this is solving a wrong problem or\nsolving a problem in a wrong approach. What happens to the entries\nbeyond $maxrecent-th in the recent list with the current code? The\nlist effectively truncated and not shown? Wouldn't the same thing\nhappen to the entries beyond ($maxrecent+5)-th in the recent list\nwith your patch?\n\nTruncating the list at $maxrecent-th to match the display\n(i.e. declare that \"$maxrecent\" is really the max number of entries\nwe would keep track of), or allowing the UI to scroll so that\nentries beyond $maxrecent-th one can be shown, perhaps?\n\n>\n> Signed-off-by: Philip Oakley \n> ---\n> word usage corrected.\n> Eric's comments $gmane/282432\n> ---\n> git-gui/lib/choose_repository.tcl | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\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> \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"},{"id":"274685","messageId":"1CAD7B4439DF4AE19EE8CD56CB47F399@PhilipOakley","threadId":"41026","inReplyTo":"xmqqsi30c1lk.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 4/4] git gui: allow for a long recentrepo list","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":null,"receivedAt":"2015-12-17T19:32:55Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Junio C Hamano\" , December 17, 2015 7:32 PM\n> Philip Oakley writes:\n>\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 outside of the gui, or the maxrecent setting changed\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> I do not have a strong objection either way, but the magic number 5\n> smells like an indication that this is solving a wrong problem or\n> solving a problem in a wrong approach. What happens to the entries\n> beyond $maxrecent-th in the recent list with the current code? The\n> list effectively truncated and not shown? Wouldn't the same thing\n> happen to the entries beyond ($maxrecent+5)-th in the recent list\n> with your patch?\n\nTrue, it was a magic number selected as being a moderate overspill \nallowance.\nI tried 3 and 5 as possible values, and 5 didn't look ungainly, even on my \nsmall netbook, and covered all my test hacking cases.\n\nThe issue was that the prior code assumed that the fault could never happen \nand limited the display, which is sorted, to the max value, so in a fault \ncondition the most recent repo could fall off the bottom.\n\nAs a bit of 'monkey patching' I wasn't sure how the dialog creation could \nknow in advance how long the list would be, so I simply added on a small \nmargin.\n\n>\n> Truncating the list at $maxrecent-th to match the display\n> (i.e. declare that \"$maxrecent\" is really the max number of entries\n> we would keep track of), or allowing the UI to scroll so that\n> entries beyond $maxrecent-th one can be shown, perhaps?\n\nThis would certainly be a reasonable approach, but I don't know enough of \nthe tcl to know how to make it happen. If anyone has suggestions...\n\n>\n>>\n>> Signed-off-by: Philip Oakley \n>> ---\n>> word usage corrected.\n>> Eric's comments $gmane/282432\n>> ---\n>> git-gui/lib/choose_repository.tcl | 2 +-\n>> 1 file changed, 1 insertion(+), 1 deletion(-)\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> 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":"274696","messageId":"2F62ABA2EE734F5F926C9E102969895F@PhilipOakley","threadId":"41026","inReplyTo":"1CAD7B4439DF4AE19EE8CD56CB47F399@PhilipOakley","subject":"Re: [PATCH v2 4/4] git gui: allow for a long recentrepo list","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":null,"receivedAt":"2015-12-17T22:10:45Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Philip Oakley\" \n> From: \"Junio C Hamano\" , December 17, 2015 7:32 PM\n>> Philip Oakley writes:\n>>\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 outside of the gui, or the maxrecent setting changed\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>> I do not have a strong objection either way, but the magic number 5\n>> smells like an indication that this is solving a wrong problem or\n>> solving a problem in a wrong approach. What happens to the entries\n>> beyond $maxrecent-th in the recent list with the current code? The\n>> list effectively truncated and not shown? Wouldn't the same thing\n>> happen to the entries beyond ($maxrecent+5)-th in the recent list\n>> with your patch?\n>\n> True, it was a magic number selected as being a moderate overspill \n> allowance.\n> I tried 3 and 5 as possible values, and 5 didn't look ungainly, even on my \n> small netbook, and covered all my test hacking cases.\n>\n> The issue was that the prior code assumed that the fault could never \n> happen and limited the display, which is sorted, to the max value, so in a \n> fault condition the most recent repo could fall off the bottom.\n>\n> As a bit of 'monkey patching' I wasn't sure how the dialog creation could \n> know in advance how long the list would be, so I simply added on a small \n> margin.\n>\n>>\n>> Truncating the list at $maxrecent-th to match the display\n>> (i.e. declare that \"$maxrecent\" is really the max number of entries\n>> we would keep track of), or allowing the UI to scroll so that\n>> entries beyond $maxrecent-th one can be shown, perhaps?\n>\n> This would certainly be a reasonable approach, but I don't know enough of \n> the tcl to know how to make it happen. If anyone has suggestions...\n\nI think I see the way to do it. V3 Patch 4/4 to follow.\n>\n>>\n>>>\n>>> Signed-off-by: Philip Oakley \n>>> ---\n>>> word usage corrected.\n>>> Eric's comments $gmane/282432\n>>> ---\n>>> git-gui/lib/choose_repository.tcl | 2 +-\n>>> 1 file changed, 1 insertion(+), 1 deletion(-)\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>> 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>\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":"274700","messageId":"1450392007-600-1-git-send-email-philipoakley@iee.org","threadId":"41026","inReplyTo":"2F62ABA2EE734F5F926C9E102969895F@PhilipOakley","subject":"[PATCH v3 4/4] git gui: allow for a long recentrepo list","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":"2015-12-17T22:40:07Z","receivedAt":"2015-12-17T22:40:07Z","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.\nUse the actual length determined earlier, now saved as $lenrecent.\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 outside of 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\nThe list length will be trimmed to $maxrecent after the user selects\na repo to open.\n\nSigned-off-by: Philip Oakley \n---\nV2:\nword usage corrected.\nEric's comments $gmane/282432\nV3: \nUpdated list length measure following Junio's comments $gmane/282669\n\nReplaces the previous V2 Patch 4/4.\n\n---\n git-gui/lib/choose_repository.tcl | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/git-gui/lib/choose_repository.tcl b/git-gui/lib/choose_repository.tcl\nindex ad7a888..b4cc7dd 100644\n--- a/git-gui/lib/choose_repository.tcl\n+++ b/git-gui/lib/choose_repository.tcl\n@@ -134,7 +134,8 @@ constructor pick {} {\n \t$opts conf -state disabled\n \n \tset sorted_recent [_get_recentrepos]\n-\tif {[llength $sorted_recent] > 0} {\n+\tset lenrecent [llength $sorted_recent]\n+\tif {$lenrecent > 0} {\n \t\tif {$m_repo ne {}} {\n \t\t\t$m_repo add separator\n \t\t\t$m_repo add command \\\n@@ -153,7 +154,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 $lenrecent\n \t\t$w_recentlist tag conf link \\\n \t\t\t-foreground blue \\\n \t\t\t-underline 1\n-- \n2.6.4.windows.1\n"},{"id":"274701","messageId":"xmqqbn9obqne.fsf@gitster.mtv.corp.google.com","threadId":"41026","inReplyTo":"1450392007-600-1-git-send-email-philipoakley@iee.org","subject":"Re: [PATCH v3 4/4] git gui: allow for a long recentrepo list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-17T23:29:25Z","receivedAt":"2015-12-17T23:29:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Philip Oakley writes:\n\n> The gui.recentrepo list may be longer than the maxrecent setting.\n> Use the actual length determined earlier, now saved as $lenrecent.\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 outside of the gui, or the maxrecent setting changed\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> The list length will be trimmed to $maxrecent after the user selects\n> a repo to open.\n>\n> Signed-off-by: Philip Oakley \n> ---\n> V2:\n> word usage corrected.\n> Eric's comments $gmane/282432\n> V3: \n> Updated list length measure following Junio's comments $gmane/282669\n>\n> Replaces the previous V2 Patch 4/4.\n>\n> ---\n> git-gui/lib/choose_repository.tcl | 5 +++--\n> 1 file changed, 3 insertions(+), 2 deletions(-)\n>\n> diff --git a/git-gui/lib/choose_repository.tcl b/git-gui/lib/choose_repository.tcl\n> index ad7a888..b4cc7dd 100644\n> --- a/git-gui/lib/choose_repository.tcl\n> +++ b/git-gui/lib/choose_repository.tcl\n> @@ -134,7 +134,8 @@ constructor pick {} {\n> \t$opts conf -state disabled\n> \n> \tset sorted_recent [_get_recentrepos]\n> -\tif {[llength $sorted_recent] > 0} {\n> +\tset lenrecent [llength $sorted_recent]\n> +\tif {$lenrecent > 0} {\n> \t\tif {$m_repo ne {}} {\n> \t\t\t$m_repo add separator\n> \t\t\t$m_repo add command \\\n> @@ -153,7 +154,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 $lenrecent\n> \t\t$w_recentlist tag conf link \\\n> \t\t\t-foreground blue \\\n> \t\t\t-underline 1\n\nNatural questions that come to mind after seeing the above are like\nthese:\n\n - Does maxrecent still get used after this change?\n\n - Does the remainder of the code get upset if -height is set to too\n small (lenrecent might turn out to be 1, for example)?\n\nAnd grepping for maxrecent shows there is _append_recentrepos that\ndoes this:\n\nproc _append_recentrepos {path} {\n\tset path [file normalize $path]\n\tset recent [get_config gui.recentrepo]\n\n\t...\n\tif {[set maxrecent [get_config gui.maxrecentrepo]] eq {}} {\n\t\tset maxrecent 10\n\t}\n\n\twhile {[llength $recent] > $maxrecent} {\n\t\t_unset_recentrepo [lindex $recent 0]\n\t\tset recent [lrange $recent 1 end]\n\t}\n}\n\nThe last while loop looks to me like truncating the overlong recent\nlist down to maxrecent. Perhaps a similar logic needs to be at the\nend of _get_recentrepos?\n"},{"id":"274703","messageId":"9731888BD4C348F5BFC82FA96D978034@PhilipOakley","threadId":"41026","inReplyTo":"xmqqbn9obqne.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 4/4] git gui: allow for a long recentrepo list","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":null,"receivedAt":"2015-12-17T23:35:25Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Junio C Hamano\" \n> Philip Oakley writes:\n>\n>> The gui.recentrepo list may be longer than the maxrecent setting.\n>> Use the actual length determined earlier, now saved as $lenrecent.\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 outside of the gui, or the maxrecent setting changed\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>> The list length will be trimmed to $maxrecent after the user selects\n>> a repo to open.\n>>\n>> Signed-off-by: Philip Oakley \n>> ---\n>> V2:\n>> word usage corrected.\n>> Eric's comments $gmane/282432\n>> V3:\n>> Updated list length measure following Junio's comments $gmane/282669\n>>\n>> Replaces the previous V2 Patch 4/4.\n>>\n>> ---\n>> git-gui/lib/choose_repository.tcl | 5 +++--\n>> 1 file changed, 3 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/git-gui/lib/choose_repository.tcl \n>> b/git-gui/lib/choose_repository.tcl\n>> index ad7a888..b4cc7dd 100644\n>> --- a/git-gui/lib/choose_repository.tcl\n>> +++ b/git-gui/lib/choose_repository.tcl\n>> @@ -134,7 +134,8 @@ constructor pick {} {\n>> $opts conf -state disabled\n>>\n>> set sorted_recent [_get_recentrepos]\n>> - if {[llength $sorted_recent] > 0} {\n>> + set lenrecent [llength $sorted_recent]\n>> + if {$lenrecent > 0} {\n>> if {$m_repo ne {}} {\n>> $m_repo add separator\n>> $m_repo add command \\\n>> @@ -153,7 +154,7 @@ constructor pick {} {\n>> -background [get_bg_color $w_body.recentlabel] \\\n>> -wrap none \\\n>> -width 50 \\\n>> - -height $maxrecent\n>> + -height $lenrecent\n>> $w_recentlist tag conf link \\\n>> -foreground blue \\\n>> -underline 1\n>\n> Natural questions that come to mind after seeing the above are like\n> these:\n>\n> - Does maxrecent still get used after this change?\n\nYes, essentially as you describe....\n\n>\n> - Does the remainder of the code get upset if -height is set to too\n> small (lenrecent might turn out to be 1, for example)?\n\nNo it's OK, the code has at least checked it is positive..\n\n>\n> And grepping for maxrecent shows there is _append_recentrepos that\n> does this:\n\nIt does three things:\n- removes the current (selected) repo from the list of past repos (and \nremoves any duplicates)\n- places the current repo at the end\n- trims the list to back to max repos.\n\nNominally, it can't do anything about non-selected valid repos which may be \nlisted in the config twice because git config won't selectively remove just \none of two identical key/values, however in Patch 3 I do remove both older & \nnewer entries and re-read the config (some may consider that a small \nregression).\n\nIt's not easy working around the contrary assumptions of the gui.recentrepo \nlist (once only) and the reality of the .gitconfig (duplicates possible) and \n'git config's --unset/--unset-all limitation.\n\nExactly how those extra entries get there is an open question, but it does \nhappen (so say's google, as well as the user report, I've seen it myself ;-)\n\n>\n> proc _append_recentrepos {path} {\n> set path [file normalize $path]\n> set recent [get_config gui.recentrepo]\n>\n> ...\n> if {[set maxrecent [get_config gui.maxrecentrepo]] eq {}} {\n> set maxrecent 10\n> }\n>\n> while {[llength $recent] > $maxrecent} {\n> _unset_recentrepo [lindex $recent 0]\n> set recent [lrange $recent 1 end]\n> }\n> }\n>\n> The last while loop looks to me like truncating the overlong recent\n> list down to maxrecent.\n\nYes\n\n> Perhaps a similar logic needs to be at the\n> end of _get_recentrepos?\n\nThe logic here is to show what is in the config to the user, *before* we \ntrim the list, hence we wait until the user has selected a repo to open and \ndo the purging in _append_recentrepos.\n\nAs an aside, there's also a long standing 'feature' that if one works both \non and off network, then network repo paths get trimmed as soon as the user \nstarts the gui while off-net (or the network goes down;-) because those \nrecent repo paths become 'invalid' and are purged, but let's not go there.\n\n--\nPhilip \n"}]}