{"thread":{"id":"52854","subject":"[PATCH] completion: add diff --color-moved[-ws]","startedAt":"2020-02-20T21:47:08Z","lastAt":"2020-02-24T06:02:47Z","messageCount":8,"participants":["Kir Kolyshkin","Matheus Tavares Bernardino","Kirill Kolyshkin","Matheus Tavares"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"392204","messageId":"20200220214647.451064-1-kolyshkin@gmail.com","threadId":"52854","inReplyTo":null,"subject":"[PATCH] completion: add diff --color-moved[-ws]","fromName":"Kir Kolyshkin","fromEmail":"kolyshkin@gmail.com","sentAt":"2020-02-20T21:46:47Z","receivedAt":"2020-02-20T21:47:08Z","isPatch":true,"sender":{"key":"kolyshkin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/4522509?v=4"},"body":"These options are available since git v2.15, but somehow\neluded from the completion script.\n\nSigned-off-by: Kir Kolyshkin <kolyshkin@gmail.com>\n---\n contrib/completion/git-completion.bash | 15 +++++++++++++++\n 1 file changed, 15 insertions(+)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 1aac5a56c0..43cb6a312d 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -1487,9 +1487,16 @@ __git_diff_algorithms=\"myers minimal patience histogram\"\n \n __git_diff_submodule_formats=\"diff log short\"\n \n+__git_color_moved_opts=\"no default plain blocks zebra dimmed-zebra\"\n+\n+__git_color_moved_ws_opts=\"no ignore-space-at-eol ignore-all-space\n+\t\t\tallow-indentation-change\"\n+\n __git_diff_common_options=\"--stat --numstat --shortstat --summary\n \t\t\t--patch-with-stat --name-only --name-status --color\n \t\t\t--no-color --color-words --no-renames --check\n+\t\t\t--color-moved --color-moved= --no-color-moved\n+\t\t\t--color-moved-ws= --no-color-moved-ws\n \t\t\t--full-index --binary --abbrev --diff-filter=\n \t\t\t--find-copies-harder --ignore-cr-at-eol\n \t\t\t--text --ignore-space-at-eol --ignore-space-change\n@@ -1520,6 +1527,14 @@ _git_diff ()\n \t\t__gitcomp \"$__git_diff_submodule_formats\" \"\" \"${cur##--submodule=}\"\n \t\treturn\n \t\t;;\n+\t--color-moved=*)\n+\t\t__gitcomp \"$__git_color_moved_opts\" \"\" \"${cur##--color-moved=}\"\n+\t\treturn\n+\t\t;;\n+\t--color-moved-ws=*)\n+\t\t__gitcomp \"$__git_color_moved_ws_opts\" \"\" \"${cur##--color-moved-ws=}\"\n+\t\treturn\n+\t\t;;\n \t--*)\n \t\t__gitcomp \"--cached --staged --pickaxe-all --pickaxe-regex\n \t\t\t--base --ours --theirs --no-index\n-- \n2.24.1\n\n"},{"id":"392222","messageId":"CAHd-oW61vuuvs25iOkLJ--Wv39ZXFF87_-pV=vX=oTibRinn5Q@mail.gmail.com","threadId":"52854","inReplyTo":"20200220214647.451064-1-kolyshkin@gmail.com","subject":"Re: [PATCH] completion: add diff --color-moved[-ws]","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-02-20T23:30:08Z","receivedAt":"2020-02-20T23:30:24Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Thu, Feb 20, 2020 at 6:47 PM Kir Kolyshkin <kolyshkin@gmail.com> wrote:\n>\n> These options are available since git v2.15, but somehow\n> eluded from the completion script.\n>\n> Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>\n> ---\n>  contrib/completion/git-completion.bash | 15 +++++++++++++++\n>  1 file changed, 15 insertions(+)\n>\n> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\n> index 1aac5a56c0..43cb6a312d 100644\n> --- a/contrib/completion/git-completion.bash\n> +++ b/contrib/completion/git-completion.bash\n> @@ -1487,9 +1487,16 @@ __git_diff_algorithms=\"myers minimal patience histogram\"\n>\n>  __git_diff_submodule_formats=\"diff log short\"\n>\n> +__git_color_moved_opts=\"no default plain blocks zebra dimmed-zebra\"\n> +\n> +__git_color_moved_ws_opts=\"no ignore-space-at-eol ignore-all-space\n> +                       allow-indentation-change\"\n\nI think \"ignore-space-change\" is missing in the above list. Besides\nthat, the patch LGTM.\n\nAs a side-note: when we have an option with an already filled value,\ne.g. `--color-moved-ws=allow-identation-change,`, although it accepts\nmore values as a comma separated list, pressing <tab><tab> won't\nsuggest others. But I think the helper functions in\ngit-completion.bash don't provide an easy way to do that right now\n(and there might even be a bug in bash-completion involving this [1]).\nSo, I think the patch is good as it is (just adding the missing item\nto that list) :)\n\n[1]: https://github.com/scop/bash-completion/issues/240\n"},{"id":"392226","messageId":"CAHd-oW6ytdEfmyCDtuPZ1uMc57jovnc334jMpombx-90POynkw@mail.gmail.com","threadId":"52854","inReplyTo":"CAHd-oW61vuuvs25iOkLJ--Wv39ZXFF87_-pV=vX=oTibRinn5Q@mail.gmail.com","subject":"Re: [PATCH] completion: add diff --color-moved[-ws]","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-02-21T00:11:39Z","receivedAt":"2020-02-21T00:11:54Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Thu, Feb 20, 2020 at 8:30 PM Matheus Tavares Bernardino\n<matheus.bernardino@usp.br> wrote:\n>\n[...]\n> (and there might even be a bug in bash-completion involving this [1]).\n\nOops, nevermind. I misread the issue, it's a problem with some of\nbash-completion's pre-shipped completion functions, not regarding the\n\"complete\" builtin. Sorry for the noise.\n"},{"id":"392276","messageId":"CAGmPdrwDUzk7iHYOrjPNrCJkhuJT==v5L-d_hY5gXxMLf+gFDQ@mail.gmail.com","threadId":"52854","inReplyTo":"CAHd-oW61vuuvs25iOkLJ--Wv39ZXFF87_-pV=vX=oTibRinn5Q@mail.gmail.com","subject":"Re: [PATCH] completion: add diff --color-moved[-ws]","fromName":"Kirill Kolyshkin","fromEmail":"kolyshkin@gmail.com","sentAt":"2020-02-21T20:15:08Z","receivedAt":"2020-02-21T20:15:24Z","isPatch":true,"sender":{"key":"kolyshkin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/4522509?v=4"},"body":"On Thu, 20 Feb 2020 at 15:30, Matheus Tavares Bernardino\n<matheus.bernardino@usp.br> wrote:\n>\n> On Thu, Feb 20, 2020 at 6:47 PM Kir Kolyshkin <kolyshkin@gmail.com> wrote:\n> >\n> > These options are available since git v2.15, but somehow\n> > eluded from the completion script.\n> >\n> > Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>\n> > ---\n> >  contrib/completion/git-completion.bash | 15 +++++++++++++++\n> >  1 file changed, 15 insertions(+)\n> >\n> > diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\n> > index 1aac5a56c0..43cb6a312d 100644\n> > --- a/contrib/completion/git-completion.bash\n> > +++ b/contrib/completion/git-completion.bash\n> > @@ -1487,9 +1487,16 @@ __git_diff_algorithms=\"myers minimal patience histogram\"\n> >\n> >  __git_diff_submodule_formats=\"diff log short\"\n> >\n> > +__git_color_moved_opts=\"no default plain blocks zebra dimmed-zebra\"\n> > +\n> > +__git_color_moved_ws_opts=\"no ignore-space-at-eol ignore-all-space\n> > +                       allow-indentation-change\"\n>\n> I think \"ignore-space-change\" is missing in the above list. Besides\n> that, the patch LGTM.\n\nThanks for catching this, I'll send v2.\n\n>\n> As a side-note: when we have an option with an already filled value,\n> e.g. `--color-moved-ws=allow-identation-change,`, although it accepts\n> more values as a comma separated list, pressing <tab><tab> won't\n> suggest others. But I think the helper functions in\n> git-completion.bash don't provide an easy way to do that right now\n\nThat's right -- as much as I want to make --key=val[,val ...] work,\nthere is no (easy?) way to do that :(\n\n> (and there might even be a bug in bash-completion involving this [1]).\n> So, I think the patch is good as it is (just adding the missing item\n> to that list) :)\n>\n> [1]: https://github.com/scop/bash-completion/issues/240\n"},{"id":"392277","messageId":"20200221201545.1244861-1-kolyshkin@gmail.com","threadId":"52854","inReplyTo":"20200220214647.451064-1-kolyshkin@gmail.com","subject":"[PATCH v2] completion: add diff --color-moved[-ws]","fromName":"Kir Kolyshkin","fromEmail":"kolyshkin@gmail.com","sentAt":"2020-02-21T20:15:45Z","receivedAt":"2020-02-21T20:15:58Z","isPatch":true,"sender":{"key":"kolyshkin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/4522509?v=4"},"body":"These options are available since git v2.15, but somehow\neluded from the completion script.\n\nNote that while --color-moved-ws= accepts comma-separated\nlist of values, there is no (easy?) way to make it work\nwith completion (see e.g. [1]).\n\n[1]: https://github.com/scop/bash-completion/issues/240\n\n[v2: added missing ignore-space-change]\n\nAcked-by: Matheus Tavares Bernardino <matheus.bernardino@usp.br>\nSigned-off-by: Kir Kolyshkin <kolyshkin@gmail.com>\n---\n contrib/completion/git-completion.bash | 15 +++++++++++++++\n 1 file changed, 15 insertions(+)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 1aac5a56c0..348f0c0c57 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -1487,9 +1487,16 @@ __git_diff_algorithms=\"myers minimal patience histogram\"\n \n __git_diff_submodule_formats=\"diff log short\"\n \n+__git_color_moved_opts=\"no default plain blocks zebra dimmed-zebra\"\n+\n+__git_color_moved_ws_opts=\"no ignore-space-at-eol ignore-space-change\n+\t\t\tignore-all-space allow-indentation-change\"\n+\n __git_diff_common_options=\"--stat --numstat --shortstat --summary\n \t\t\t--patch-with-stat --name-only --name-status --color\n \t\t\t--no-color --color-words --no-renames --check\n+\t\t\t--color-moved --color-moved= --no-color-moved\n+\t\t\t--color-moved-ws= --no-color-moved-ws\n \t\t\t--full-index --binary --abbrev --diff-filter=\n \t\t\t--find-copies-harder --ignore-cr-at-eol\n \t\t\t--text --ignore-space-at-eol --ignore-space-change\n@@ -1520,6 +1527,14 @@ _git_diff ()\n \t\t__gitcomp \"$__git_diff_submodule_formats\" \"\" \"${cur##--submodule=}\"\n \t\treturn\n \t\t;;\n+\t--color-moved=*)\n+\t\t__gitcomp \"$__git_color_moved_opts\" \"\" \"${cur##--color-moved=}\"\n+\t\treturn\n+\t\t;;\n+\t--color-moved-ws=*)\n+\t\t__gitcomp \"$__git_color_moved_ws_opts\" \"\" \"${cur##--color-moved-ws=}\"\n+\t\treturn\n+\t\t;;\n \t--*)\n \t\t__gitcomp \"--cached --staged --pickaxe-all --pickaxe-regex\n \t\t\t--base --ours --theirs --no-index\n-- \n2.24.1\n\n"},{"id":"392342","messageId":"20200223050557.6380-1-matheus.bernardino@usp.br","threadId":"52854","inReplyTo":"20200221201545.1244861-1-kolyshkin@gmail.com","subject":"Re: [PATCH v2] completion: add diff --color-moved[-ws]","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-02-23T05:05:57Z","receivedAt":"2020-02-23T05:06:12Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Hi, Kir\n\nOn Fri, Feb 21, 2020 at 5:15 PM Kir Kolyshkin <kolyshkin@gmail.com> wrote:\n>\n> These options are available since git v2.15, but somehow\n> eluded from the completion script.\n>\n> Note that while --color-moved-ws= accepts comma-separated\n> list of values, there is no (easy?) way to make it work\n> with completion (see e.g. [1]).\n\nThis puzzled me for some time, but I think I finally got a way to make the\ncomma-separated completion work. Bellow is the code that does the trick,\ntogether with some tests for the new __gitcomp_cvs function. The diff is on top\nof your patch, so you can incorporate it for a v3. Alternatively, if you want\nto send these changes as a preparatory patch in a two-patches series, you have\nmy Signed-off-by :)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 348f0c0c57..e12f90b1cb 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -476,6 +476,41 @@ __gitcomp_file ()\n \ttrue\n }\n \n+# The following function is based on\n+# https://unix.stackexchange.com/a/176442 (under CC BY-SA 4.0) written\n+# by diffycat (https://unix.stackexchange.com/users/17452/diffycat)\n+#\n+# Call __gitcomp for options that accept a comma separated list of values, i.e.\n+# something like '--option=val1,val2'. The caller must have already checked\n+# that `$cur == --option=*`. __gitcomp_csv requires two arguments:\n+# 1: The option in the format of '--option='\n+# 2: The list of possible values for the said option, separated by spaces. Note\n+#    that the values cannot contain commas or spaces.\n+__gitcomp_csv ()\n+{\n+\tlocal cur_values=\"${cur##$1}\" available_values\n+\n+\t# Filter out already used values from completion reply\n+\tfor value in $2\n+\tdo\n+\t\tif ! [[ \",$cur_values,\" =~ \",$value,\" ]]\n+\t\tthen\n+\t\t\tavailable_values=\"$available_values $value\"\n+\t\tfi\n+\tdone\n+\n+\tlocal prefix pattern\n+\tif [[ \"$cur_values\" == *,* ]]\n+\tthen\n+\t\tprefix=\"${cur_values%,*},\"\n+\t\tpattern=\"${cur_values##*,}\"\n+\telse\n+\t\tpattern=\"$cur_values\"\n+\tfi\n+\n+\t__gitcomp \"$available_values\" \"$prefix\" \"$pattern\"\n+}\n+\n # Execute 'git ls-files', unless the --committable option is specified, in\n # which case it runs 'git diff-index' to find out the files that can be\n # committed.  It return paths relative to the directory specified in the first\n@@ -1532,7 +1567,7 @@ _git_diff ()\n \t\treturn\n \t\t;;\n \t--color-moved-ws=*)\n-\t\t__gitcomp \"$__git_color_moved_ws_opts\" \"\" \"${cur##--color-moved-ws=}\"\n+\t\t__gitcomp_csv \"--color-moved-ws=\" \"$__git_color_moved_ws_opts\"\n \t\treturn\n \t\t;;\n \t--*)\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex 5505e5aa24..75b6afe2b7 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -122,6 +122,21 @@ test_gitcomp_nl ()\n \ttest_cmp expected out\n }\n \n+# Test __gitcomp_csv\n+# Arguments are:\n+# 1: current word (cur)\n+# -: the rest are passed to __gitcomp_csv\n+test_gitcomp_csv ()\n+{\n+\tlocal -a COMPREPLY &&\n+\tsed -e 's/Z$//' >expected &&\n+\tlocal cur=\"$1\" &&\n+\tshift &&\n+\t__gitcomp_csv \"$@\" &&\n+\tprint_comp &&\n+\ttest_cmp expected out\n+}\n+\n invalid_variable_name='${foo.bar}'\n \n actual=\"$TRASH_DIRECTORY/actual\"\n@@ -580,6 +595,21 @@ test_expect_success '__gitcomp_nl - doesnt fail because of invalid variable name\n \t__gitcomp_nl \"$invalid_variable_name\"\n '\n \n+test_expect_success '__gitcomp_csv - display all values' '\n+\ttest_gitcomp_csv \"--opt=\" \"--opt=\" \"val1 val2 val3\" <<-\\EOF\n+\tval1 Z\n+\tval2 Z\n+\tval3 Z\n+\tEOF\n+'\n+\n+test_expect_success '__gitcomp_csv - do not display values in $cur' '\n+\ttest_gitcomp_csv \"--opt=val1,\" \"--opt=\" \"val1 val2 val3\" <<-\\EOF\n+\tval1,val2 Z\n+\tval1,val3 Z\n+\tEOF\n+'\n+\n test_expect_success '__git_remotes - list remotes from $GIT_DIR/remotes and from config file' '\n \tcat >expect <<-EOF &&\n \tremote_from_file_1\n\n--->8---\n\n> [v2: added missing ignore-space-change]\n\nChange logs are quite useful for reviewers, so thanks for adding one :) However,\nthey don't really need to be part of the commit message. So the common place to\nwrite them is between the '---' line and the diff stats. Everything you write\nin this section will be available for reviewers but discarded when the patch is\napplied.\n\n> Acked-by: Matheus Tavares Bernardino <matheus.bernardino@usp.br>\n\nIf you are going to send a v3, please, also use\n\"Matheus Tavares <matheus.bernardino@usp.br>\" instead (that's just how I\nnormally sign).\n\n"},{"id":"392387","messageId":"CAGmPdrxRNVSvQYzOx9-nhtztGgS_q92y3GO8n1nqRE5UJWiQRg@mail.gmail.com","threadId":"52854","inReplyTo":"20200223050557.6380-1-matheus.bernardino@usp.br","subject":"Re: [PATCH v2] completion: add diff --color-moved[-ws]","fromName":"Kirill Kolyshkin","fromEmail":"kolyshkin@gmail.com","sentAt":"2020-02-24T04:41:40Z","receivedAt":"2020-02-24T04:41:55Z","isPatch":true,"sender":{"key":"kolyshkin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/4522509?v=4"},"body":"On Sat, 22 Feb 2020 at 21:06, Matheus Tavares <matheus.bernardino@usp.br> wrote:\n>\n> Hi, Kir\n>\n> On Fri, Feb 21, 2020 at 5:15 PM Kir Kolyshkin <kolyshkin@gmail.com> wrote:\n> >\n> > These options are available since git v2.15, but somehow\n> > eluded from the completion script.\n> >\n> > Note that while --color-moved-ws= accepts comma-separated\n> > list of values, there is no (easy?) way to make it work\n> > with completion (see e.g. [1]).\n>\n> This puzzled me for some time, but I think I finally got a way to make the\n> comma-separated completion work. Bellow is the code that does the trick,\n> together with some tests for the new __gitcomp_cvs function. The diff is on top\n> of your patch, so you can incorporate it for a v3. Alternatively, if you want\n> to send these changes as a preparatory patch in a two-patches series, you have\n> my Signed-off-by :)\n\nThanks! I played with it, and I can't say I like it. The issues I see are:\n\n1. It does not suggest a comma, so it doesn't work as \"discovery mode\".\nWhat I mean by it is, in general, bash completion does one thing beyond\nthe completion itself -- it also helps a user to discover the\nsynopsis, i.e learn\nwhat command and options are available (same as menus in GUI apps).\n\n2. It's not smart enough to not repeat the option that is already there,\ni.e. if you type git diff --color-mode-ws=ignore-space-at-eol,<Tab>\nit will offer ignore-space-at-eol again, which in this case doesn't make\nmuch sense.\n\n3. (this is not about the __gitcomp_csv, but rather about the way\n--color-moved-ws option arguments work) Some values for --color-moved-ws\nare not meant to be used together with others, e.g. --color-moved-ws=no\nis exclusive, and it looks like allow-indentation-change is exclusive, too.\nOthers are more complicated, e.g. ignore-space-change \"includes\"\nignore-space-at-eol so once the former is set, it does not make sense\nto suggest the latter. Even coding all these rules (what makes sense\nwith what, and what doesn't) sounds a bit too complicated.\n\nSo, all of the above makes me think that maybe it doesn't make\nmuch sense to have csv completion at all.\n\n>\n> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\n> index 348f0c0c57..e12f90b1cb 100644\n> --- a/contrib/completion/git-completion.bash\n> +++ b/contrib/completion/git-completion.bash\n> @@ -476,6 +476,41 @@ __gitcomp_file ()\n>         true\n>  }\n>\n> +# The following function is based on\n> +# https://unix.stackexchange.com/a/176442 (under CC BY-SA 4.0) written\n> +# by diffycat (https://unix.stackexchange.com/users/17452/diffycat)\n> +#\n> +# Call __gitcomp for options that accept a comma separated list of values, i.e.\n> +# something like '--option=val1,val2'. The caller must have already checked\n> +# that `$cur == --option=*`. __gitcomp_csv requires two arguments:\n> +# 1: The option in the format of '--option='\n> +# 2: The list of possible values for the said option, separated by spaces. Note\n> +#    that the values cannot contain commas or spaces.\n> +__gitcomp_csv ()\n> +{\n> +       local cur_values=\"${cur##$1}\" available_values\n> +\n> +       # Filter out already used values from completion reply\n> +       for value in $2\n> +       do\n> +               if ! [[ \",$cur_values,\" =~ \",$value,\" ]]\n> +               then\n> +                       available_values=\"$available_values $value\"\n> +               fi\n> +       done\n> +\n> +       local prefix pattern\n> +       if [[ \"$cur_values\" == *,* ]]\n> +       then\n> +               prefix=\"${cur_values%,*},\"\n> +               pattern=\"${cur_values##*,}\"\n> +       else\n> +               pattern=\"$cur_values\"\n> +       fi\n> +\n> +       __gitcomp \"$available_values\" \"$prefix\" \"$pattern\"\n> +}\n> +\n>  # Execute 'git ls-files', unless the --committable option is specified, in\n>  # which case it runs 'git diff-index' to find out the files that can be\n>  # committed.  It return paths relative to the directory specified in the first\n> @@ -1532,7 +1567,7 @@ _git_diff ()\n>                 return\n>                 ;;\n>         --color-moved-ws=*)\n> -               __gitcomp \"$__git_color_moved_ws_opts\" \"\" \"${cur##--color-moved-ws=}\"\n> +               __gitcomp_csv \"--color-moved-ws=\" \"$__git_color_moved_ws_opts\"\n>                 return\n>                 ;;\n>         --*)\n> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\n> index 5505e5aa24..75b6afe2b7 100755\n> --- a/t/t9902-completion.sh\n> +++ b/t/t9902-completion.sh\n> @@ -122,6 +122,21 @@ test_gitcomp_nl ()\n>         test_cmp expected out\n>  }\n>\n> +# Test __gitcomp_csv\n> +# Arguments are:\n> +# 1: current word (cur)\n> +# -: the rest are passed to __gitcomp_csv\n> +test_gitcomp_csv ()\n> +{\n> +       local -a COMPREPLY &&\n> +       sed -e 's/Z$//' >expected &&\n> +       local cur=\"$1\" &&\n> +       shift &&\n> +       __gitcomp_csv \"$@\" &&\n> +       print_comp &&\n> +       test_cmp expected out\n> +}\n> +\n>  invalid_variable_name='${foo.bar}'\n>\n>  actual=\"$TRASH_DIRECTORY/actual\"\n> @@ -580,6 +595,21 @@ test_expect_success '__gitcomp_nl - doesnt fail because of invalid variable name\n>         __gitcomp_nl \"$invalid_variable_name\"\n>  '\n>\n> +test_expect_success '__gitcomp_csv - display all values' '\n> +       test_gitcomp_csv \"--opt=\" \"--opt=\" \"val1 val2 val3\" <<-\\EOF\n> +       val1 Z\n> +       val2 Z\n> +       val3 Z\n> +       EOF\n> +'\n> +\n> +test_expect_success '__gitcomp_csv - do not display values in $cur' '\n> +       test_gitcomp_csv \"--opt=val1,\" \"--opt=\" \"val1 val2 val3\" <<-\\EOF\n> +       val1,val2 Z\n> +       val1,val3 Z\n> +       EOF\n> +'\n> +\n>  test_expect_success '__git_remotes - list remotes from $GIT_DIR/remotes and from config file' '\n>         cat >expect <<-EOF &&\n>         remote_from_file_1\n>\n> --->8---\n>\n> > [v2: added missing ignore-space-change]\n>\n> Change logs are quite useful for reviewers, so thanks for adding one :) However,\n> they don't really need to be part of the commit message. So the common place to\n> write them is between the '---' line and the diff stats. Everything you write\n> in this section will be available for reviewers but discarded when the patch is\n> applied.\n>\n> > Acked-by: Matheus Tavares Bernardino <matheus.bernardino@usp.br>\n>\n> If you are going to send a v3, please, also use\n> \"Matheus Tavares <matheus.bernardino@usp.br>\" instead (that's just how I\n> normally sign).\n>\n"},{"id":"392390","messageId":"CAHd-oW72kMzYqcRMffQRb+VB8=HdQt8noJgAfGYxpEsWZdbMuA@mail.gmail.com","threadId":"52854","inReplyTo":"CAGmPdrxRNVSvQYzOx9-nhtztGgS_q92y3GO8n1nqRE5UJWiQRg@mail.gmail.com","subject":"Re: [PATCH v2] completion: add diff --color-moved[-ws]","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-02-24T06:02:30Z","receivedAt":"2020-02-24T06:02:47Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Mon, Feb 24, 2020 at 1:41 AM Kirill Kolyshkin <kolyshkin@gmail.com> wrote:\n>\n> On Sat, 22 Feb 2020 at 21:06, Matheus Tavares <matheus.bernardino@usp.br> wrote:\n> >\n> > On Fri, Feb 21, 2020 at 5:15 PM Kir Kolyshkin <kolyshkin@gmail.com> wrote:\n> > >\n> > > Note that while --color-moved-ws= accepts comma-separated\n> > > list of values, there is no (easy?) way to make it work\n> > > with completion (see e.g. [1]).\n> >\n> > This puzzled me for some time, but I think I finally got a way to make the\n> > comma-separated completion work. Bellow is the code that does the trick,\n[...]\n> Thanks! I played with it, and I can't say I like it. The issues I see are:\n>\n> 1. It does not suggest a comma, so it doesn't work as \"discovery mode\".\n\nIn fact, it is possible to make it suggest the commas. But them, the\noutput would be too polluted, IMO, as we would get twice as much\nsuggestions. For example:\n\n$ git diff --color-moved-ws=<tab><tab>\nallow-indentation-change allow-indentation-change, ignore-all-space\nignore-all-space, ignore-space-at-eol ignore-space-at-eol,\nignore-space-change ignore-space-change, no no,\n\nBesides, the completion without csv also does not help on the\n\"discovery mode\" for lists of values. So I think it is still better to\nuse the csv completion, than not using it at all. I.e. there is no\nadditional downside, and we get the bonus of nice completions when the\nuser tries something as `git diff\n--color-moved-ws=ignore-space-at-eol,<tab><tab>`.\n\n> 2. It's not smart enough to not repeat the option that is already there,\n\nHmm, in my setup, it is not repeating options. I also added a test\ncase at t6120-describe.sh to make sure there is no repetition. Could\nyou please execute this test in your environment and report back if it\nfails there? (maybe I'm using some non-portable code?)\n\nFor reference, the section that should be responsible for avoiding\nrepetitions is the following:\n\n> > +       # Filter out already used values from completion reply\n> > +       for value in $2\n> > +       do\n> > +               if ! [[ \",$cur_values,\" =~ \",$value,\" ]]\n> > +               then\n> > +                       available_values=\"$available_values $value\"\n> > +               fi\n> > +       done\n\n> 3. (this is not about the __gitcomp_csv, but rather about the way\n> --color-moved-ws option arguments work) Some values for --color-moved-ws\n> are not meant to be used together with others, e.g. --color-moved-ws=no\n> is exclusive, and it looks like allow-indentation-change is exclusive, too.\n> Others are more complicated, e.g. ignore-space-change \"includes\"\n> ignore-space-at-eol so once the former is set, it does not make sense\n> to suggest the latter. Even coding all these rules (what makes sense\n> with what, and what doesn't) sounds a bit too complicated.\n\nI don't think completion should handle these cases. Take a look at the\ncompletion for git-describe, for example. The option --long is\nincompatible with --no-abbrev, but the completion script won't stop\nsuggesting one because the user already typed the other. IMO,\nintroducing all these rules to completion would make the script\nunnecessarily complex. In the end, git-describe itself will warn\nand/or exit if incompatible options are given; the completion is more\nlike a helper, IMO.\n\n> So, all of the above makes me think that maybe it doesn't make\n> much sense to have csv completion at all.\n\nHmm, I understand your point. But as I mentioned earlier, I think\nadding the csv completion won't cause any extra harm; and as a bonus,\nit will help users that already want to type a list of values to do it\nfaster. So I still think it is better than not using it at all.\n"}]}