{"thread":{"id":"54900","subject":"[PATCH v2 0/3] completion: make __git_complete public","startedAt":"2020-12-29T17:09:22Z","lastAt":"2021-01-06T08:31:40Z","messageCount":10,"participants":["Felipe Contreras","René Scharfe","SZEDER Gábor","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":3},"messages":[{"id":"413130","messageId":"20201229170837.297857-1-felipe.contreras@gmail.com","threadId":"54900","inReplyTo":null,"subject":"[PATCH v2 0/3] completion: make __git_complete public","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-12-29T17:08:34Z","receivedAt":"2020-12-29T17:09:22Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Back in 2012 I argued [1] for the introduction of a helper that would\nallow users to specify aliases like:\n\n  git_complete gf git_fetch\n\nThere was pushback because there was no clear guideline for public\nfunctions (git_complete vs. _git_complete vs. _GIT_complete), and some\naliases didn't actually work.\n\nFast-forward to 2020, and there's still no guideline for public\nfunctions, and those aliases still don't work (even though I sent the\nfixes).\n\nThis has not prevented people from using this function that is clearly\nneeded to setup custom aliases [2], and in fact that's the recommended\nway since there's no other.\n\nBut it is cumbersome that the user must type:\n\n  __git_complete gf _git_fetch\n\nOr worse:\n\n  __git_complete gk __gitk_main\n\n8 years is more than enough time to stop waiting for the perfect to\ncome; let's define a public function (with the same name) that is\nactually user-friendly:\n\n  __git_complete gf git_fetch\n  __git_complete gk gitk\n\nWhile also maintaining backwards compatibility.\n\n[1] https://lore.kernel.org/git/1334524814-13581-1-git-send-email-felipe.contreras@gmail.com/\n[2] https://stackoverflow.com/questions/342969/how-do-i-get-bash-completion-to-work-with-aliases\n\nFelipe Contreras (3):\n  completion: bash: add __git_have_func helper\n  test: completion: add tests for __git_complete\n  completion: add proper public __git_complete\n\n contrib/completion/git-completion.bash | 41 +++++++++++++++++++-------\n t/t9902-completion.sh                  | 20 +++++++++++++\n 2 files changed, 51 insertions(+), 10 deletions(-)\n\nRange-diff:\n-:  ---------- > 1:  0993732142 completion: bash: add __git_have_func helper\n1:  ea77b1a140 ! 2:  7918c34d0e test: completion: add tests for __git_complete\n    @@ Metadata\n      ## Commit message ##\n         test: completion: add tests for __git_complete\n     \n    +    Even though the function was marked as not public, it's already used in\n    +    the wild.\n    +\n    +    We should at least test basic functionality.\n    +\n         Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n     \n      ## t/t9902-completion.sh ##\n    @@ t/t9902-completion.sh: test_expect_success 'sourcing the completion script clear\n      '\n      \n     +test_expect_success '__git_complete' '\n    ++\tunset -f __git_wrap__git_main &&\n     +\t__git_complete foo __git_main &&\n    -+\ttest \"$(type -t __git_wrap__git_main)\" == function &&\n    ++\t__git_have_func __git_wrap__git_main &&\n     +\t__git_complete gf _git_fetch &&\n    -+\ttest \"$(type -t __git_wrap_git_fetch)\" == function\n    ++\t__git_have_func __git_wrap_git_fetch\n     +'\n     +\n      test_done\n2:  aec19e61ee ! 3:  8a6cc52063 completion: add proper public __git_complete\n    @@ Metadata\n      ## Commit message ##\n         completion: add proper public __git_complete\n     \n    -    Back in 2012 I argued [1] for the introduction of a helper that would\n    -    allow users to specify aliases like:\n    +    When __git_complete was introduced, it was meant to be temporarily, while\n    +    a proper guideline for public shell functions was established\n    +    (tentatively _GIT_complete), but since that never happened, people\n    +    in the wild started to use __git_complete, even though it was marked as\n    +    not public.\n     \n    -      git_complete gf git_fetch\n    +    Eight years is more than enough wait, let's mark this function as\n    +    public, and make it a bit more user-friendly.\n     \n    -    Back then there was pushback because there was no clear guideline for\n    -    public functions (git_complete vs _git_complete vs _GIT_complete), and\n    -    some aliases didn't actually work.\n    -\n    -    Fast-forward to 2020 and there's still no guideline for public\n    -    functions, and those aliases still don't work (even though I sent the\n    -    fixes).\n    -\n    -    This has not prevented people from using this function that is clearly\n    -    needed to setup custom aliases [2], and in fact it's the recommended\n    -    way. But it is cumbersome that the user must type:\n    -\n    -      __git_complete gf _git_fetch\n    -\n    -    Or worse:\n    +    So that instead of doing:\n     \n           __git_complete gk __gitk_main\n     \n    -    8 years is more than enough time to stop waiting for the perfect to\n    -    come; let's define a public function (with the same name) that is\n    -    actually user-friendly:\n    +    The user can do:\n     \n    -      __git_complete gf git_fetch\n           __git_complete gk gitk\n     \n    -    While also maintaining backwards compatibility.\n    +    And instead of:\n     \n    -    The logic is:\n    +      __git_complete gf _git_fetch\n    +\n    +    Do:\n     \n    -     1. If $2 exists, use it directly\n    -     2. If not, check if __$2_main exists\n    -     3. If not, check if _$2 exists\n    -     4. If not, fail\n    +      __git_complete gf git_fetch\n     \n    -    [1] https://lore.kernel.org/git/1334524814-13581-1-git-send-email-felipe.contreras@gmail.com/\n    -    [2] https://stackoverflow.com/questions/342969/how-do-i-get-bash-completion-to-work-with-aliases\n    +    Backwards compatibility is maintained.\n     \n         Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n     \n    @@ contrib/completion/git-completion.bash: __git_complete ()\n     +{\n     +\tlocal func\n     +\n    -+\tif [[ \"$(type -t $2)\" == function ]]; then\n    ++\tif __git_have_func $2; then\n     +\t\tfunc=$2\n    -+\telif [[ \"$(type -t __$2_main)\" == function ]]; then\n    ++\telif __git_have_func __$2_main; then\n     +\t\tfunc=__$2_main\n    -+\telif [[ \"$(type -t _$2)\" == function ]]; then\n    ++\telif __git_have_func _$2; then\n     +\t\tfunc=_$2\n     +\telse\n     +\t\techo \"ERROR: could not find function '$2'\" 1>&2\n    @@ contrib/completion/git-completion.bash: __git_complete ()\n     \n      ## t/t9902-completion.sh ##\n     @@ t/t9902-completion.sh: test_expect_success 'sourcing the completion script clears cached --options' '\n    - '\n      \n      test_expect_success '__git_complete' '\n    -+\tunset -f __git_wrap__git_main &&\n    + \tunset -f __git_wrap__git_main &&\n     +\n      \t__git_complete foo __git_main &&\n    - \ttest \"$(type -t __git_wrap__git_main)\" == function &&\n    + \t__git_have_func __git_wrap__git_main &&\n     +\tunset -f __git_wrap__git_main &&\n     +\n      \t__git_complete gf _git_fetch &&\n    --\ttest \"$(type -t __git_wrap_git_fetch)\" == function\n    -+\ttest \"$(type -t __git_wrap_git_fetch)\" == function &&\n    +-\t__git_have_func __git_wrap_git_fetch\n    ++\t__git_have_func __git_wrap_git_fetch &&\n     +\n     +\t__git_complete foo git &&\n    -+\ttest \"$(type -t __git_wrap__git_main)\" == function &&\n    ++\t__git_have_func __git_wrap__git_main &&\n     +\tunset -f __git_wrap__git_main &&\n     +\n     +\t__git_complete gd git_diff &&\n    -+\ttest \"$(type -t __git_wrap_git_diff)\" == function &&\n    ++\t__git_have_func __git_wrap_git_diff &&\n     +\n     +\ttest_must_fail __git_complete ga missing\n      '\n-- \n2.30.0\n\n"},{"id":"413131","messageId":"20201229170837.297857-3-felipe.contreras@gmail.com","threadId":"54900","inReplyTo":"20201229170837.297857-1-felipe.contreras@gmail.com","subject":"[PATCH v2 2/3] test: completion: add tests for __git_complete","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-12-29T17:08:36Z","receivedAt":"2020-12-29T17:09:39Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Even though the function was marked as not public, it's already used in\nthe wild.\n\nWe should at least test basic functionality.\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n t/t9902-completion.sh | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex a1c4f1f6d4..c0b4380eae 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -2380,4 +2380,12 @@ test_expect_success 'sourcing the completion script clears cached --options' '\n \tverbose test -z \"$__gitcomp_builtin_notes_edit\"\n '\n \n+test_expect_success '__git_complete' '\n+\tunset -f __git_wrap__git_main &&\n+\t__git_complete foo __git_main &&\n+\t__git_have_func __git_wrap__git_main &&\n+\t__git_complete gf _git_fetch &&\n+\t__git_have_func __git_wrap_git_fetch\n+'\n+\n test_done\n-- \n2.30.0\n\n"},{"id":"413132","messageId":"20201229170837.297857-2-felipe.contreras@gmail.com","threadId":"54900","inReplyTo":"20201229170837.297857-1-felipe.contreras@gmail.com","subject":"[PATCH v2 1/3] completion: bash: add __git_have_func helper","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-12-29T17:08:35Z","receivedAt":"2020-12-29T17:09:39Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"This makes the code more readable, and also will help when new code\nwants to do similar checks.\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n contrib/completion/git-completion.bash | 10 +++++++---\n 1 file changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 463a3124da..869c73ee2c 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -3358,15 +3358,19 @@ __git_support_parseopt_helper () {\n \tesac\n }\n \n+__git_have_func () {\n+\tdeclare -f $1 >/dev/null 2>/dev/null\n+}\n+\n __git_complete_command () {\n \tlocal command=\"$1\"\n \tlocal completion_func=\"_git_${command//-/_}\"\n-\tif ! declare -f $completion_func >/dev/null 2>/dev/null &&\n-\t\tdeclare -f _completion_loader >/dev/null 2>/dev/null\n+\tif ! __git_have_func $completion_func &&\n+\t\t__git_have_func _completion_loader\n \tthen\n \t\t_completion_loader \"git-$command\"\n \tfi\n-\tif declare -f $completion_func >/dev/null 2>/dev/null\n+\tif __git_have_func $completion_func\n \tthen\n \t\t$completion_func\n \t\treturn 0\n-- \n2.30.0\n\n"},{"id":"413133","messageId":"20201229170837.297857-4-felipe.contreras@gmail.com","threadId":"54900","inReplyTo":"20201229170837.297857-1-felipe.contreras@gmail.com","subject":"[PATCH v2 3/3] completion: add proper public __git_complete","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-12-29T17:08:37Z","receivedAt":"2020-12-29T17:09:39Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"When __git_complete was introduced, it was meant to be temporarily, while\na proper guideline for public shell functions was established\n(tentatively _GIT_complete), but since that never happened, people\nin the wild started to use __git_complete, even though it was marked as\nnot public.\n\nEight years is more than enough wait, let's mark this function as\npublic, and make it a bit more user-friendly.\n\nSo that instead of doing:\n\n  __git_complete gk __gitk_main\n\nThe user can do:\n\n  __git_complete gk gitk\n\nAnd instead of:\n\n  __git_complete gf _git_fetch\n\nDo:\n\n  __git_complete gf git_fetch\n\nBackwards compatibility is maintained.\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n contrib/completion/git-completion.bash | 31 ++++++++++++++++++++------\n t/t9902-completion.sh                  | 14 +++++++++++-\n 2 files changed, 37 insertions(+), 8 deletions(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 869c73ee2c..1179bd2876 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -3497,10 +3497,7 @@ __git_func_wrap ()\n \t$1\n }\n \n-# Setup completion for certain functions defined above by setting common\n-# variables and workarounds.\n-# This is NOT a public function; use at your own risk.\n-__git_complete ()\n+___git_complete ()\n {\n \tlocal wrapper=\"__git_wrap${2}\"\n \teval \"$wrapper () { __git_func_wrap $2 ; }\"\n@@ -3508,13 +3505,33 @@ __git_complete ()\n \t\t|| complete -o default -o nospace -F $wrapper $1\n }\n \n-__git_complete git __git_main\n-__git_complete gitk __gitk_main\n+# Setup the completion for git commands\n+# 1: command or alias\n+# 2: function to call (e.g. `git`, `gitk`, `git_fetch`)\n+__git_complete ()\n+{\n+\tlocal func\n+\n+\tif __git_have_func $2; then\n+\t\tfunc=$2\n+\telif __git_have_func __$2_main; then\n+\t\tfunc=__$2_main\n+\telif __git_have_func _$2; then\n+\t\tfunc=_$2\n+\telse\n+\t\techo \"ERROR: could not find function '$2'\" 1>&2\n+\t\treturn 1\n+\tfi\n+\t___git_complete $1 $func\n+}\n+\n+___git_complete git __git_main\n+___git_complete gitk __gitk_main\n \n # The following are necessary only for Cygwin, and only are needed\n # when the user has tab-completed the executable name and consequently\n # included the '.exe' suffix.\n #\n if [ \"$OSTYPE\" = cygwin ]; then\n-\t__git_complete git.exe __git_main\n+\t___git_complete git.exe __git_main\n fi\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex c0b4380eae..c4a7758409 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -2382,10 +2382,22 @@ test_expect_success 'sourcing the completion script clears cached --options' '\n \n test_expect_success '__git_complete' '\n \tunset -f __git_wrap__git_main &&\n+\n \t__git_complete foo __git_main &&\n \t__git_have_func __git_wrap__git_main &&\n+\tunset -f __git_wrap__git_main &&\n+\n \t__git_complete gf _git_fetch &&\n-\t__git_have_func __git_wrap_git_fetch\n+\t__git_have_func __git_wrap_git_fetch &&\n+\n+\t__git_complete foo git &&\n+\t__git_have_func __git_wrap__git_main &&\n+\tunset -f __git_wrap__git_main &&\n+\n+\t__git_complete gd git_diff &&\n+\t__git_have_func __git_wrap_git_diff &&\n+\n+\ttest_must_fail __git_complete ga missing\n '\n \n test_done\n-- \n2.30.0\n\n"},{"id":"413176","messageId":"1402dabd-089d-616a-ef79-ff1ae3fcb438@web.de","threadId":"54900","inReplyTo":"20201229170837.297857-2-felipe.contreras@gmail.com","subject":"Re: [PATCH v2 1/3] completion: bash: add __git_have_func helper","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-12-30T17:17:52Z","receivedAt":"2020-12-30T17:20:27Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 29.12.20 um 18:08 schrieb Felipe Contreras:\n> This makes the code more readable, and also will help when new code\n> wants to do similar checks.\n>\n> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n> ---\n>  contrib/completion/git-completion.bash | 10 +++++++---\n>  1 file changed, 7 insertions(+), 3 deletions(-)\n>\n> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\n> index 463a3124da..869c73ee2c 100644\n> --- a/contrib/completion/git-completion.bash\n> +++ b/contrib/completion/git-completion.bash\n> @@ -3358,15 +3358,19 @@ __git_support_parseopt_helper () {\n>  \tesac\n>  }\n>\n> +__git_have_func () {\n> +\tdeclare -f $1 >/dev/null 2>/dev/null\n\nI stumbled slightly over the lack of quoting.  It doesn't matter for\nthe callers below, but new callers passing arbitrary strings could\ncause strange effects:\n\n\tx() { echo x; }\n\ty() { echo y; }\n\t__git_have_func \"x y\" # succeeds\n\n\t__git_have_func -a # succeeds\n\nI just skimmed patch 3, but it seems to call __git_have_func with\nuser-supplied strings, so this might become relevant.\n\nAnd then I wondered why use declare -f, which prints the function's\nbody, when there is -F, which just prints the function's name.  And why\nrepeat /dev/null when redirecting stderr when the more shorter 2>&1\nwould do the same?  None of hat was introduced by you patch, of course.\nAnyway, this seems to work for me:\n\n\t__git_have_func () {\n\t\tcase \"$1\" in\n\t\t-*) return 1 ;;\n\t\tesac\n\t\tdeclare -F \"$1\" >/dev/null 2>&1\n\t}\n\n> +}\n> +\n>  __git_complete_command () {\n>  \tlocal command=\"$1\"\n>  \tlocal completion_func=\"_git_${command//-/_}\"\n> -\tif ! declare -f $completion_func >/dev/null 2>/dev/null &&\n> -\t\tdeclare -f _completion_loader >/dev/null 2>/dev/null\n> +\tif ! __git_have_func $completion_func &&\n> +\t\t__git_have_func _completion_loader\n>  \tthen\n>  \t\t_completion_loader \"git-$command\"\n>  \tfi\n> -\tif declare -f $completion_func >/dev/null 2>/dev/null\n> +\tif __git_have_func $completion_func\n>  \tthen\n>  \t\t$completion_func\n>  \t\treturn 0\n>\n\n"},{"id":"413177","messageId":"5fecbb6b2f9a_871cb208f6@natae.notmuch","threadId":"54900","inReplyTo":"1402dabd-089d-616a-ef79-ff1ae3fcb438@web.de","subject":"Re: [PATCH v2 1/3] completion: bash: add __git_have_func helper","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-12-30T17:39:55Z","receivedAt":"2020-12-30T17:40:39Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"René Scharfe wrote:\n> Am 29.12.20 um 18:08 schrieb Felipe Contreras:\n> > This makes the code more readable, and also will help when new code\n> > wants to do similar checks.\n> >\n> > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n> > ---\n> >  contrib/completion/git-completion.bash | 10 +++++++---\n> >  1 file changed, 7 insertions(+), 3 deletions(-)\n> >\n> > diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\n> > index 463a3124da..869c73ee2c 100644\n> > --- a/contrib/completion/git-completion.bash\n> > +++ b/contrib/completion/git-completion.bash\n> > @@ -3358,15 +3358,19 @@ __git_support_parseopt_helper () {\n> >  \tesac\n> >  }\n> >\n> > +__git_have_func () {\n> > +\tdeclare -f $1 >/dev/null 2>/dev/null\n> \n> I stumbled slightly over the lack of quoting.  It doesn't matter for\n> the callers below, but new callers passing arbitrary strings could\n> cause strange effects:\n> \n> \tx() { echo x; }\n> \ty() { echo y; }\n> \t__git_have_func \"x y\" # succeeds\n> \n> \t__git_have_func -a # succeeds\n> \n> I just skimmed patch 3, but it seems to call __git_have_func with\n> user-supplied strings, so this might become relevant.\n\nYes. I just just copied the code to minimize the changes, but this is a\nvalid concern.\n\n> And then I wondered why use declare -f, which prints the function's\n> body, when there is -F, which just prints the function's name.  And why\n> repeat /dev/null when redirecting stderr when the more shorter 2>&1\n> would do the same?  None of hat was introduced by you patch, of course.\n> Anyway, this seems to work for me:\n> \n> \t__git_have_func () {\n> \t\tcase \"$1\" in\n> \t\t-*) return 1 ;;\n> \t\tesac\n> \t\tdeclare -F \"$1\" >/dev/null 2>&1\n> \t}\n\nI wondered some of those things too, but opted for the minimal approach.\n\nYour change seems good to me, however I prefer this to the case\nstatement:\n\n  [[ \"$1\" == -* ]] && return 1\n\nBut doesn't seem to be too welcomed in git's bash style.\n\nLooks like this would be prefered:\n\n  if [[ \"$1\" == -* ]]; then\n    return 1\n  fi\n\nI would prefer either one of those to the case statement.\n\nBut the other change is good. I'll include that as a separate patch on\nthe next version.\n\nCheers.\n\n-- \nFelipe Contreras"},{"id":"413178","messageId":"5fecbfbf46fe5_8775920846@natae.notmuch","threadId":"54900","inReplyTo":"5fecbb6b2f9a_871cb208f6@natae.notmuch","subject":"Re: [PATCH v2 1/3] completion: bash: add __git_have_func helper","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-12-30T17:58:23Z","receivedAt":"2020-12-30T17:59:07Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Felipe Contreras wrote:\n> René Scharfe wrote:\n> > And then I wondered why use declare -f, which prints the function's\n> > body, when there is -F, which just prints the function's name.  And why\n> > repeat /dev/null when redirecting stderr when the more shorter 2>&1\n> > would do the same?  None of hat was introduced by you patch, of course.\n> > Anyway, this seems to work for me:\n> > \n> > \t__git_have_func () {\n> > \t\tcase \"$1\" in\n> > \t\t-*) return 1 ;;\n> > \t\tesac\n> > \t\tdeclare -F \"$1\" >/dev/null 2>&1\n> > \t}\n\nActually... How about:\n\n  declare -F -- \"$1\" >/dev/null 2>&1\n\n?\n\n-- \nFelipe Contreras"},{"id":"413179","messageId":"20201230180008.GL8396@szeder.dev","threadId":"54900","inReplyTo":"1402dabd-089d-616a-ef79-ff1ae3fcb438@web.de","subject":"Re: [PATCH v2 1/3] completion: bash: add __git_have_func helper","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2020-12-30T18:00:08Z","receivedAt":"2020-12-30T18:01:08Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Wed, Dec 30, 2020 at 06:17:52PM +0100, René Scharfe wrote:\n> > +__git_have_func () {\n> > +\tdeclare -f $1 >/dev/null 2>/dev/null\n\n> And then I wondered why use declare -f, which prints the function's\n> body, when there is -F, which just prints the function's name.  And why\n> repeat /dev/null when redirecting stderr when the more shorter 2>&1\n> would do the same?  None of hat was introduced by you patch, of course.\n> Anyway, this seems to work for me:\n> \n> \t__git_have_func () {\n> \t\tcase \"$1\" in\n> \t\t-*) return 1 ;;\n> \t\tesac\n> \t\tdeclare -F \"$1\" >/dev/null 2>&1\n> \t}\n\nThe Bash completion script should be usable from Zsh as well, and Zsh\nonly supports 'declare -f' but not '-F', see 06f44c3cc5 (completion:\nmake compatible with zsh, 2010-09-06).  The Zsh version included in\nthe 16.04 based LTS setup that I have at hand doesn't yet seem to have\na (for us) usable 'declare -F' yet.\n\n"},{"id":"413180","messageId":"5fecc25b369f9_879c52083c@natae.notmuch","threadId":"54900","inReplyTo":"20201230180008.GL8396@szeder.dev","subject":"Re: [PATCH v2 1/3] completion: bash: add __git_have_func helper","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-12-30T18:09:31Z","receivedAt":"2020-12-30T18:10:15Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"SZEDER Gábor wrote:\n> On Wed, Dec 30, 2020 at 06:17:52PM +0100, René Scharfe wrote:\n> > > +__git_have_func () {\n> > > +\tdeclare -f $1 >/dev/null 2>/dev/null\n> \n> > And then I wondered why use declare -f, which prints the function's\n> > body, when there is -F, which just prints the function's name.  And why\n> > repeat /dev/null when redirecting stderr when the more shorter 2>&1\n> > would do the same?  None of hat was introduced by you patch, of course.\n> > Anyway, this seems to work for me:\n> > \n> > \t__git_have_func () {\n> > \t\tcase \"$1\" in\n> > \t\t-*) return 1 ;;\n> > \t\tesac\n> > \t\tdeclare -F \"$1\" >/dev/null 2>&1\n> > \t}\n> \n> The Bash completion script should be usable from Zsh as well, and Zsh\n> only supports 'declare -f' but not '-F', see 06f44c3cc5 (completion:\n> make compatible with zsh, 2010-09-06).  The Zsh version included in\n> the 16.04 based LTS setup that I have at hand doesn't yet seem to have\n> a (for us) usable 'declare -F' yet.\n\nAhh. Good catch.\n\n  declare -f -- \"$1\" >/dev/null 2>&1\n\nThen.\n\n-- \nFelipe Contreras"},{"id":"413543","messageId":"xmqqh7nusc0r.fsf@gitster.c.googlers.com","threadId":"54900","inReplyTo":"5fecbb6b2f9a_871cb208f6@natae.notmuch","subject":"Re: [PATCH v2 1/3] completion: bash: add __git_have_func helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-06T08:30:28Z","receivedAt":"2021-01-06T08:31:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n>> Anyway, this seems to work for me:\n>> \n>> \t__git_have_func () {\n>> \t\tcase \"$1\" in\n>> \t\t-*) return 1 ;;\n>> \t\tesac\n>> \t\tdeclare -F \"$1\" >/dev/null 2>&1\n>> \t}\n>\n> I wondered some of those things too, but opted for the minimal approach.\n>\n> Your change seems good to me, however I prefer this to the case\n> statement:\n>\n>   [[ \"$1\" == -* ]] && return 1\n>\n> But doesn't seem to be too welcomed in git's bash style.\n\nActually, there is no formal \"bash style\" guide for our codebase.\nIn bash completion script, you are free to use any bashisms as you\nfind convenient, as long as it is consistent with the surrounding\ncode.\n\nAs SZEDER and you later agree in the downthread, Zsh compatibility\nwould be a lot more important than sticking to general POSIX shell\nsubset (and following our shell style guide for the rest of the\nsystem that tries hard to be independent from bashism) when\nimproving this script.\n\n> Looks like this would be prefered:\n>\n>   if [[ \"$1\" == -* ]]; then\n>     return 1\n>   fi\n\nThis is another example that we do not apply the shell style guide\nin Documentation/CodingGuidelines to this script (if we did, we\nwouldn't see the semicolon there).\n\nThanks.\n"}]}