Re: [PATCH v5 1/3] completion: add new __gitcompadd helper

>> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
>> index d743e56..01325de 100644
>> --- a/contrib/completion/git-completion.bash
>> +++ b/contrib/completion/git-completion.bash
>> @@ -225,6 +225,11 @@ _get_comp_words_by_ref ()
>>  fi
>>  fi
>>
>> +__gitcompadd ()
>> +{
>> +     COMPREPLY=($(compgen -W "$1" -P "$2" -S "$4" -- "$3"))
>> +}
>> +
>>  # Generates completion reply with compgen, appending a space to possible
>>  # completion words, if necessary.
>>  # It accepts 1 to 4 arguments:
>> @@ -238,13 +243,11 @@ __gitcomp ()
>>
>>       case "$cur_" in
>>       --*=)
>> -             COMPREPLY=()
>> +             __gitcompadd
>>               ;;
>>       *)
>>               local IFS=$'\n'
>> -             COMPREPLY=($(compgen -P "${2-}" \
>> -                     -W "$(__gitcomp_1 "${1-}" "${4-}")" \
>> -                     -- "$cur_"))
>> +             __gitcompadd "$(__gitcomp_1 "${1-}" "${4-}")" "${2-}" "$cur_" ""
>>               ;;
>>       esac
>>  }
>> @@ -261,7 +264,7 @@ __gitcomp ()
>>  __gitcomp_nl ()
>>  {
>>       local IFS=$'\n'
>> -     COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}"))
>> +     __gitcompadd "$1" "${2-}" "${3-$cur}" "${4- }"
>>  }
>
> I feel hesitant about this change.  One of the ways I'm exploring to
> fix the issues with shell metacharacters and expansion in compgen is
> to actually replace compgen.  We already iterate over all possible
> completion words in __gitcomp_1(), so it doesn't make much of a
> difference to do the filtering for the current word while we are at
> it.  However, the way __gitcompadd() encapsulates COMPREPLY=($(compgen
> ...)), and tha basic idea of never touching COMPREPLY directly make
> this basically impossible.

>>  __git_heads ()
>> @@ -486,7 +489,7 @@ __git_complete_remote_or_refspec ()
>>                       case "$cmd" in
>>                       push) no_complete_refspec=1 ;;
>>                       fetch)
>> -                             COMPREPLY=()
>> +                             __gitcompadd
>>                               return
>>                               ;;
>>                       *) ;;
>> @@ -502,7 +505,7 @@ __git_complete_remote_or_refspec ()
>>               return
>>       fi
>>       if [ $no_complete_refspec = 1 ]; then
>> -             COMPREPLY=()
>> +             __gitcompadd
>>               return
>>       fi
>>       [ "$remote" = "." ] && remote=
>> @@ -776,7 +779,7 @@ _git_am ()
>>                       "
>>               return
>>       esac
>> -     COMPREPLY=()
>> +     __gitcompadd
>
> These changes effectively run compgen in a subshell to generate an
> empty completion reply.  While it doesn't really matter on Linux,
> it'll add another half a tenth of a second delay in those cases on my
> Windows machine.  At least it should be conditional, i.e. $(compgen
> ...) shouldn't be executed when there are no possible completion
> words.
>
> However, I think those COMPREPLY=() assignments are pointless anyway.
> COMPREPLY is always empty when completion functions are invoked, so
> there is no need to explicitly set it to an empty array when we don't
> provide any words for completion.  Their only use is basically to
> explicitly tell us humans that in those cases we don't offer any words
> for completion.  But we don't do that consistently: there are several
> places without offering words for completion and without COMPREPLY=(),
> e.g. the '__git_has_doubledash && return' pattern.
>
> Perhaps it would be time to get rid of these COMPREPLY=() assignments?