On Sat, 22 Feb 2020 at 21:06, Matheus Tavares <matheus.bernardino@xxxxxx> wrote: > > Hi, Kir > > On Fri, Feb 21, 2020 at 5:15 PM Kir Kolyshkin <kolyshkin@xxxxxxxxx> wrote: > > > > These options are available since git v2.15, but somehow > > eluded from the completion script. > > > > Note that while --color-moved-ws= accepts comma-separated > > list of values, there is no (easy?) way to make it work > > with completion (see e.g. [1]). > > This puzzled me for some time, but I think I finally got a way to make the > comma-separated completion work. Bellow is the code that does the trick, > together with some tests for the new __gitcomp_cvs function. The diff is on top > of your patch, so you can incorporate it for a v3. Alternatively, if you want > to send these changes as a preparatory patch in a two-patches series, you have > my Signed-off-by :) Thanks! I played with it, and I can't say I like it. The issues I see are: 1. It does not suggest a comma, so it doesn't work as "discovery mode". What I mean by it is, in general, bash completion does one thing beyond the completion itself -- it also helps a user to discover the synopsis, i.e learn what command and options are available (same as menus in GUI apps). 2. It's not smart enough to not repeat the option that is already there, i.e. if you type git diff --color-mode-ws=ignore-space-at-eol,<Tab> it will offer ignore-space-at-eol again, which in this case doesn't make much sense. 3. (this is not about the __gitcomp_csv, but rather about the way --color-moved-ws option arguments work) Some values for --color-moved-ws are not meant to be used together with others, e.g. --color-moved-ws=no is exclusive, and it looks like allow-indentation-change is exclusive, too. Others are more complicated, e.g. ignore-space-change "includes" ignore-space-at-eol so once the former is set, it does not make sense to suggest the latter. Even coding all these rules (what makes sense with what, and what doesn't) sounds a bit too complicated. So, all of the above makes me think that maybe it doesn't make much sense to have csv completion at all. > > diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash > index 348f0c0c57..e12f90b1cb 100644 > --- a/contrib/completion/git-completion.bash > +++ b/contrib/completion/git-completion.bash > @@ -476,6 +476,41 @@ __gitcomp_file () > true > } > > +# The following function is based on > +# https://unix.stackexchange.com/a/176442 (under CC BY-SA 4.0) written > +# by diffycat (https://unix.stackexchange.com/users/17452/diffycat) > +# > +# Call __gitcomp for options that accept a comma separated list of values, i.e. > +# something like '--option=val1,val2'. The caller must have already checked > +# that `$cur == --option=*`. __gitcomp_csv requires two arguments: > +# 1: The option in the format of '--option=' > +# 2: The list of possible values for the said option, separated by spaces. Note > +# that the values cannot contain commas or spaces. > +__gitcomp_csv () > +{ > + local cur_values="${cur##$1}" available_values > + > + # Filter out already used values from completion reply > + for value in $2 > + do > + if ! [[ ",$cur_values," =~ ",$value," ]] > + then > + available_values="$available_values $value" > + fi > + done > + > + local prefix pattern > + if [[ "$cur_values" == *,* ]] > + then > + prefix="${cur_values%,*}," > + pattern="${cur_values##*,}" > + else > + pattern="$cur_values" > + fi > + > + __gitcomp "$available_values" "$prefix" "$pattern" > +} > + > # Execute 'git ls-files', unless the --committable option is specified, in > # which case it runs 'git diff-index' to find out the files that can be > # committed. It return paths relative to the directory specified in the first > @@ -1532,7 +1567,7 @@ _git_diff () > return > ;; > --color-moved-ws=*) > - __gitcomp "$__git_color_moved_ws_opts" "" "${cur##--color-moved-ws=}" > + __gitcomp_csv "--color-moved-ws=" "$__git_color_moved_ws_opts" > return > ;; > --*) > diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh > index 5505e5aa24..75b6afe2b7 100755 > --- a/t/t9902-completion.sh > +++ b/t/t9902-completion.sh > @@ -122,6 +122,21 @@ test_gitcomp_nl () > test_cmp expected out > } > > +# Test __gitcomp_csv > +# Arguments are: > +# 1: current word (cur) > +# -: the rest are passed to __gitcomp_csv > +test_gitcomp_csv () > +{ > + local -a COMPREPLY && > + sed -e 's/Z$//' >expected && > + local cur="$1" && > + shift && > + __gitcomp_csv "$@" && > + print_comp && > + test_cmp expected out > +} > + > invalid_variable_name='${foo.bar}' > > actual="$TRASH_DIRECTORY/actual" > @@ -580,6 +595,21 @@ test_expect_success '__gitcomp_nl - doesnt fail because of invalid variable name > __gitcomp_nl "$invalid_variable_name" > ' > > +test_expect_success '__gitcomp_csv - display all values' ' > + test_gitcomp_csv "--opt=" "--opt=" "val1 val2 val3" <<-\EOF > + val1 Z > + val2 Z > + val3 Z > + EOF > +' > + > +test_expect_success '__gitcomp_csv - do not display values in $cur' ' > + test_gitcomp_csv "--opt=val1," "--opt=" "val1 val2 val3" <<-\EOF > + val1,val2 Z > + val1,val3 Z > + EOF > +' > + > test_expect_success '__git_remotes - list remotes from $GIT_DIR/remotes and from config file' ' > cat >expect <<-EOF && > remote_from_file_1 > > --->8--- > > > [v2: added missing ignore-space-change] > > Change logs are quite useful for reviewers, so thanks for adding one :) However, > they don't really need to be part of the commit message. So the common place to > write them is between the '---' line and the diff stats. Everything you write > in this section will be available for reviewers but discarded when the patch is > applied. > > > Acked-by: Matheus Tavares Bernardino <matheus.bernardino@xxxxxx> > > If you are going to send a v3, please, also use > "Matheus Tavares <matheus.bernardino@xxxxxx>" instead (that's just how I > normally sign). >
