{"thread":{"id":"48114","subject":"[PATCH] completion: clear cached --options when sourcing the completion script","startedAt":"2018-03-22T14:16:16Z","lastAt":"2018-03-26T11:55:26Z","messageCount":6,"participants":["SZEDER Gábor","Duy Nguyen","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"342503","messageId":"20180322141604.15957-1-szeder.dev@gmail.com","threadId":"48114","inReplyTo":null,"subject":"[PATCH] completion: clear cached --options when sourcing the completion script","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-03-22T14:16:04Z","receivedAt":"2018-03-22T14:16:16Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The established way to update the completion script in an already\nrunning shell is to simply source it again: this brings in any new\n--options and features, and clears caching variables.  E.g. it clears\nthe variables caching the list of (all|porcelain) git commands, so\nwhen they are later lazy-initialized again, then they will list and\ncache any newly installed commmands as well.\n\nUnfortunately, since d401f3debc (git-completion.bash: introduce\n__gitcomp_builtin, 2018-02-09) and subsequent patches this doesn't\nwork for a lot of git commands' options.  To eliminate a lot of\nhard-to-maintain hard-coded lists of options, those commits changed\nthe completion script to use a bunch of programmatically created and\nlazy-initialized variables to cache the options of those builtin\nporcelain commands that use parse-options.  These variables are not\ncleared upon sourcing the completion script, therefore they continue\ncaching the old lists of options, even when some commands recently\nlearned new options or when deprecated options were removed.\n\nAlways 'unset' these variables caching the options of builtin commands\nwhen sourcing the completion script.\n\nRedirect 'unset's stderr to /dev/null, because ZSH's 'unset' complains\nif it's invoked without any arguments, i.e. no variables caching\nbuiltin's options are set.  This can happen, if someone were to source\nthe completion script twice without completing any --options in\nbetween.  Bash stays silent in this case.\n\nAdd tests to ensure that these variables are indeed cleared when the\ncompletion script is sourced; not just the variables caching options,\nbut all other caching variables, i.e. the variables caching commands,\nporcelain commands and merge strategies as well.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n\nFixes a recent regression introduced in 'nd/parseopt-completion'.\n\n contrib/completion/git-completion.bash |  4 ++++\n t/t9902-completion.sh                  | 31 ++++++++++++++++++++++++++\n 2 files changed, 35 insertions(+)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 7c84eb1912..602352f952 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -280,6 +280,10 @@ __gitcomp ()\n \tesac\n }\n \n+# Clear the variables caching builtins' options when (re-)sourcing\n+# the completion script.\n+unset $(set |sed -ne 's/^\\(__gitcomp_builtin_[a-zA-Z0-9_][a-zA-Z0-9_]*\\)=.*/\\1/p') 2>/dev/null\n+\n # This function is equivalent to\n #\n #    __gitcomp \"$(git xxx --git-completion-helper) ...\"\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex e6485feb0a..4c86adadf2 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -1497,4 +1497,35 @@ do\n \t'\n done\n \n+test_expect_success 'sourcing the completion script clears cached commands' '\n+\t__git_compute_all_commands &&\n+\tverbose test -n \"$__git_all_commands\" &&\n+\t. \"$GIT_BUILD_DIR/contrib/completion/git-completion.bash\" &&\n+\tverbose test -z \"$__git_all_commands\"\n+'\n+\n+test_expect_success 'sourcing the completion script clears cached porcelain commands' '\n+\t__git_compute_porcelain_commands &&\n+\tverbose test -n \"$__git_porcelain_commands\" &&\n+\t. \"$GIT_BUILD_DIR/contrib/completion/git-completion.bash\" &&\n+\tverbose test -z \"$__git_porcelain_commands\"\n+'\n+\n+test_expect_success 'sourcing the completion script clears cached merge strategies' '\n+\t__git_compute_merge_strategies &&\n+\tverbose test -n \"$__git_merge_strategies\" &&\n+\t. \"$GIT_BUILD_DIR/contrib/completion/git-completion.bash\" &&\n+\tverbose test -z \"$__git_merge_strategies\"\n+'\n+\n+test_expect_success 'sourcing the completion script clears cached --options' '\n+\t__gitcomp_builtin checkout &&\n+\tverbose test -n \"$__gitcomp_builtin_checkout\" &&\n+\t__gitcomp_builtin notes_edit &&\n+\tverbose test -n \"$__gitcomp_builtin_notes_edit\" &&\n+\t. \"$GIT_BUILD_DIR/contrib/completion/git-completion.bash\" &&\n+\tverbose test -z \"$__gitcomp_builtin_checkout\" &&\n+\tverbose test -z \"$__gitcomp_builtin_notes_edit\"\n+'\n+\n test_done\n-- \n2.17.0.rc0.103.gbdc5836ed3\n\n"},{"id":"342523","messageId":"CACsJy8DZo_ggxhg0sKXiVSDBahNBTu_Kfowvay5urapaZ7dVGw@mail.gmail.com","threadId":"48114","inReplyTo":"20180322141604.15957-1-szeder.dev@gmail.com","subject":"Re: [PATCH] completion: clear cached --options when sourcing the completion script","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-03-22T17:06:24Z","receivedAt":"2018-03-22T17:07:08Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Mar 22, 2018 at 3:16 PM, SZEDER Gábor <szeder.dev@gmail.com> wrote:\n> The established way to update the completion script in an already\n> running shell is to simply source it again: this brings in any new\n> --options and features, and clears caching variables.  E.g. it clears\n> the variables caching the list of (all|porcelain) git commands, so\n> when they are later lazy-initialized again, then they will list and\n> cache any newly installed commmands as well.\n>\n> Unfortunately, since d401f3debc (git-completion.bash: introduce\n> __gitcomp_builtin, 2018-02-09) and subsequent patches this doesn't\n> work for a lot of git commands' options.  To eliminate a lot of\n> hard-to-maintain hard-coded lists of options, those commits changed\n> the completion script to use a bunch of programmatically created and\n> lazy-initialized variables to cache the options of those builtin\n> porcelain commands that use parse-options.  These variables are not\n> cleared upon sourcing the completion script, therefore they continue\n> caching the old lists of options, even when some commands recently\n> learned new options or when deprecated options were removed.\n>\n> Always 'unset' these variables caching the options of builtin commands\n> when sourcing the completion script.\n\nAnd here I have been happily unsetting these manually when I re-source\nto test stuff, not thinking it as a bug. Thanks!\n\n> Redirect 'unset's stderr to /dev/null, because ZSH's 'unset' complains\n> if it's invoked without any arguments, i.e. no variables caching\n> builtin's options are set.  This can happen, if someone were to source\n> the completion script twice without completing any --options in\n> between.  Bash stays silent in this case.\n>\n> Add tests to ensure that these variables are indeed cleared when the\n> completion script is sourced; not just the variables caching options,\n> but all other caching variables, i.e. the variables caching commands,\n> porcelain commands and merge strategies as well.\n>\n> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n> ---\n>\n> Fixes a recent regression introduced in 'nd/parseopt-completion'.\n>\n>  contrib/completion/git-completion.bash |  4 ++++\n>  t/t9902-completion.sh                  | 31 ++++++++++++++++++++++++++\n>  2 files changed, 35 insertions(+)\n>\n> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\n> index 7c84eb1912..602352f952 100644\n> --- a/contrib/completion/git-completion.bash\n> +++ b/contrib/completion/git-completion.bash\n> @@ -280,6 +280,10 @@ __gitcomp ()\n>         esac\n>  }\n>\n> +# Clear the variables caching builtins' options when (re-)sourcing\n> +# the completion script.\n> +unset $(set |sed -ne 's/^\\(__gitcomp_builtin_[a-zA-Z0-9_][a-zA-Z0-9_]*\\)=.*/\\1/p') 2>/dev/null\n> +\n>  # This function is equivalent to\n>  #\n>  #    __gitcomp \"$(git xxx --git-completion-helper) ...\"\n> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\n> index e6485feb0a..4c86adadf2 100755\n> --- a/t/t9902-completion.sh\n> +++ b/t/t9902-completion.sh\n> @@ -1497,4 +1497,35 @@ do\n>         '\n>  done\n>\n> +test_expect_success 'sourcing the completion script clears cached commands' '\n> +       __git_compute_all_commands &&\n> +       verbose test -n \"$__git_all_commands\" &&\n> +       . \"$GIT_BUILD_DIR/contrib/completion/git-completion.bash\" &&\n> +       verbose test -z \"$__git_all_commands\"\n> +'\n> +\n> +test_expect_success 'sourcing the completion script clears cached porcelain commands' '\n> +       __git_compute_porcelain_commands &&\n> +       verbose test -n \"$__git_porcelain_commands\" &&\n> +       . \"$GIT_BUILD_DIR/contrib/completion/git-completion.bash\" &&\n> +       verbose test -z \"$__git_porcelain_commands\"\n> +'\n> +\n> +test_expect_success 'sourcing the completion script clears cached merge strategies' '\n> +       __git_compute_merge_strategies &&\n> +       verbose test -n \"$__git_merge_strategies\" &&\n> +       . \"$GIT_BUILD_DIR/contrib/completion/git-completion.bash\" &&\n> +       verbose test -z \"$__git_merge_strategies\"\n> +'\n> +\n> +test_expect_success 'sourcing the completion script clears cached --options' '\n> +       __gitcomp_builtin checkout &&\n> +       verbose test -n \"$__gitcomp_builtin_checkout\" &&\n> +       __gitcomp_builtin notes_edit &&\n> +       verbose test -n \"$__gitcomp_builtin_notes_edit\" &&\n> +       . \"$GIT_BUILD_DIR/contrib/completion/git-completion.bash\" &&\n> +       verbose test -z \"$__gitcomp_builtin_checkout\" &&\n> +       verbose test -z \"$__gitcomp_builtin_notes_edit\"\n> +'\n> +\n>  test_done\n> --\n> 2.17.0.rc0.103.gbdc5836ed3\n>\n\n\n\n-- \nDuy\n"},{"id":"342590","messageId":"CAM0VKjm3WKkxjEN09Dv1wUnuBf8CSsUvmLjmSVb1fbHTyAdXEQ@mail.gmail.com","threadId":"48114","inReplyTo":"20180322141604.15957-1-szeder.dev@gmail.com","subject":"Re: [PATCH] completion: clear cached --options when sourcing the completion script","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-03-23T15:14:45Z","receivedAt":"2018-03-23T15:14:52Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Thu, Mar 22, 2018 at 3:16 PM, SZEDER Gábor <szeder.dev@gmail.com> wrote:\n\n> Add tests to ensure that these variables are indeed cleared when the\n> completion script is sourced; not just the variables caching options,\n> but all other caching variables, i.e. the variables caching commands,\n> porcelain commands and merge strategies as well.\n\n> +test_expect_success 'sourcing the completion script clears cached merge strategies' '\n> +       __git_compute_merge_strategies &&\n> +       verbose test -n \"$__git_merge_strategies\" &&\n> +       . \"$GIT_BUILD_DIR/contrib/completion/git-completion.bash\" &&\n> +       verbose test -z \"$__git_merge_strategies\"\n> +'\n\nHang on, this test fails in the GETTEXT_POISON build.\n\nThe thing is, we get the merge strategies with this piece of code in\n__git_list_merge_strategies() in master:\n\n    LANG=C LC_ALL=C git merge -s help 2>&1 |\n    sed -n -e '/[Aa]vailable strategies are: /,/^$/{\n        # a couple of s/// commands\n    }'\n\n\nand that '/[Aa]vailable strategies are: /' won't match in a\nGETTEXT_POISON-ed output, because that string is translated.\n\nI think for now (-rc phase) we should just drop this test, and in the\nfuture we should consider adding a 'git merge --list-strategies' option.\n"},{"id":"342636","messageId":"xmqqbmfek8ml.fsf@gitster-ct.c.googlers.com","threadId":"48114","inReplyTo":"CAM0VKjm3WKkxjEN09Dv1wUnuBf8CSsUvmLjmSVb1fbHTyAdXEQ@mail.gmail.com","subject":"Re: [PATCH] completion: clear cached --options when sourcing the completion script","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-23T17:24:34Z","receivedAt":"2018-03-23T17:24:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor <szeder.dev@gmail.com> writes:\n\n> Hang on, this test fails in the GETTEXT_POISON build.\n\nThanks.\n\n> The thing is, we get the merge strategies with this piece of code in\n> __git_list_merge_strategies() in master:\n>\n>     LANG=C LC_ALL=C git merge -s help 2>&1 |\n>     sed -n -e '/[Aa]vailable strategies are: /,/^$/{\n>         # a couple of s/// commands\n>     }'\n>\n>\n> and that '/[Aa]vailable strategies are: /' won't match in a\n> GETTEXT_POISON-ed output, because that string is translated.\n>\n> I think for now (-rc phase) we should just drop this test, and in the\n> future we should consider adding a 'git merge --list-strategies' option.\n\nI'd say we should just add !GETTEXT_POISON prereq to the problematic\ntests.  \n\n\"git merge -s help\" output under forced C locale is dependable in\nthe real world.  It is GETTEXT_POISON that does not get this fact\nright.  There is no need for '--list-strat' option to make this test\npass.\n\n\n\n\n\n\n"},{"id":"342637","messageId":"xmqq7eq2k840.fsf@gitster-ct.c.googlers.com","threadId":"48114","inReplyTo":"xmqqbmfek8ml.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] completion: clear cached --options when sourcing the completion script","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-23T17:35:43Z","receivedAt":"2018-03-23T17:35:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> SZEDER Gábor <szeder.dev@gmail.com> writes:\n>\n>> Hang on, this test fails in the GETTEXT_POISON build.\n>\n> Thanks.\n>\n>> The thing is, we get the merge strategies with this piece of code in\n>> __git_list_merge_strategies() in master:\n>>\n>>     LANG=C LC_ALL=C git merge -s help 2>&1 |\n>>     sed -n -e '/[Aa]vailable strategies are: /,/^$/{\n>>         # a couple of s/// commands\n>>     }'\n>>\n>>\n>> and that '/[Aa]vailable strategies are: /' won't match in a\n>> GETTEXT_POISON-ed output, because that string is translated.\n>>\n>> I think for now (-rc phase) we should just drop this test, and in the\n>> future we should consider adding a 'git merge --list-strategies' option.\n>\n> I'd say we should just add !GETTEXT_POISON prereq to the problematic\n> tests.  \n>\n> \"git merge -s help\" output under forced C locale is dependable in\n> the real world.  It is GETTEXT_POISON that does not get this fact\n> right.  There is no need for '--list-strat' option to make this test\n> pass.\n\nIOW, this is the minumum required.\n\nBy the way, shouldn't we be running the body of these new tests\ninside a subshell?  Otherwise a dot-sourcing by an earlier test of\nthese new ones _will_ affect all the subsequent tests.\n\n t/t9902-completion.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex 4c86adadf2..b7f5b1e632 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -1511,7 +1511,7 @@ test_expect_success 'sourcing the completion script clears cached porcelain comm\n \tverbose test -z \"$__git_porcelain_commands\"\n '\n \n-test_expect_success 'sourcing the completion script clears cached merge strategies' '\n+test_expect_success !GETTEXT_POISON 'sourcing the completion script clears cached merge strategies' '\n \t__git_compute_merge_strategies &&\n \tverbose test -n \"$__git_merge_strategies\" &&\n \t. \"$GIT_BUILD_DIR/contrib/completion/git-completion.bash\" &&\n"},{"id":"342989","messageId":"CAM0VKjnVM6sjkZjQNLtr+rm34=pPfihWzrk6L+r=ky5UZ_JsrA@mail.gmail.com","threadId":"48114","inReplyTo":"xmqq7eq2k840.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] completion: clear cached --options when sourcing the completion script","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-03-26T11:55:20Z","receivedAt":"2018-03-26T11:55:26Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Mar 23, 2018 at 6:35 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n\n>> I'd say we should just add !GETTEXT_POISON prereq to the problematic\n>> tests.\n\n> IOW, this is the minumum required.\n\nThanks for already committing the fix, I couldn't get around to it.\n\n> By the way, shouldn't we be running the body of these new tests\n> inside a subshell?  Otherwise a dot-sourcing by an earlier test of\n> these new ones _will_ affect all the subsequent tests.\n\nI don't think it matters.  All new tests first initialize the variable\nthey are interested in before sourcing the completion script, to avoid\nfalse successes caused by the variable being empty to begin with.\n\nAnd it shouldn't matter, either, because in the end the users will\nsource the completion script into their main shell process, and it\nshould just work no matter how many times it gets sourced.\n\n\n>  t/t9902-completion.sh | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\n> index 4c86adadf2..b7f5b1e632 100755\n> --- a/t/t9902-completion.sh\n> +++ b/t/t9902-completion.sh\n> @@ -1511,7 +1511,7 @@ test_expect_success 'sourcing the completion script clears cached porcelain comm\n>         verbose test -z \"$__git_porcelain_commands\"\n>  '\n>\n> -test_expect_success 'sourcing the completion script clears cached merge strategies' '\n> +test_expect_success !GETTEXT_POISON 'sourcing the completion script clears cached merge strategies' '\n>         __git_compute_merge_strategies &&\n>         verbose test -n \"$__git_merge_strategies\" &&\n>         . \"$GIT_BUILD_DIR/contrib/completion/git-completion.bash\" &&\n"}]}