[PATCH] completion: add diff --color-moved[-ws]

[PATCH] completion: add diff --color-moved[-ws]

[Kir Kolyshkin]

These options are available since git v2.15, but somehow
eluded from the completion script.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
---
 contrib/completion/git-completion.bash | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 1aac5a56c0..43cb6a312d 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1487,9 +1487,16 @@ __git_diff_algorithms="myers minimal patience histogram"
 
 __git_diff_submodule_formats="diff log short"
 
+__git_color_moved_opts="no default plain blocks zebra dimmed-zebra"
+
+__git_color_moved_ws_opts="no ignore-space-at-eol ignore-all-space
+			allow-indentation-change"
+
 __git_diff_common_options="--stat --numstat --shortstat --summary
 			--patch-with-stat --name-only --name-status --color
 			--no-color --color-words --no-renames --check
+			--color-moved --color-moved= --no-color-moved
+			--color-moved-ws= --no-color-moved-ws
 			--full-index --binary --abbrev --diff-filter=
 			--find-copies-harder --ignore-cr-at-eol
 			--text --ignore-space-at-eol --ignore-space-change
@@ -1520,6 +1527,14 @@ _git_diff ()
 		__gitcomp "$__git_diff_submodule_formats" "" "${cur##--submodule=}"
 		return
 		;;
+	--color-moved=*)
+		__gitcomp "$__git_color_moved_opts" "" "${cur##--color-moved=}"
+		return
+		;;
+	--color-moved-ws=*)
+		__gitcomp "$__git_color_moved_ws_opts" "" "${cur##--color-moved-ws=}"
+		return
+		;;
 	--*)
 		__gitcomp "--cached --staged --pickaxe-all --pickaxe-regex
 			--base --ours --theirs --no-index
-- 
2.24.1

Re: [PATCH] completion: add diff --color-moved[-ws]

[Matheus Tavares Bernardino]

On Thu, Feb 20, 2020 at 6:47 PM Kir Kolyshkin <kolyshkin@gmail.com> wrote:
>
> These options are available since git v2.15, but somehow
> eluded from the completion script.
>
> Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
> ---
>  contrib/completion/git-completion.bash | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 1aac5a56c0..43cb6a312d 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -1487,9 +1487,16 @@ __git_diff_algorithms="myers minimal patience histogram"
>
>  __git_diff_submodule_formats="diff log short"
>
> +__git_color_moved_opts="no default plain blocks zebra dimmed-zebra"
> +
> +__git_color_moved_ws_opts="no ignore-space-at-eol ignore-all-space
> +                       allow-indentation-change"

I think "ignore-space-change" is missing in the above list. Besides
that, the patch LGTM.

As a side-note: when we have an option with an already filled value,
e.g. `--color-moved-ws=allow-identation-change,`, although it accepts
more values as a comma separated list, pressing <tab><tab> won't
suggest others. But I think the helper functions in
git-completion.bash don't provide an easy way to do that right now
(and there might even be a bug in bash-completion involving this [1]).
So, I think the patch is good as it is (just adding the missing item
to that list) :)

[1]: https://github.com/scop/bash-completion/issues/240

Re: [PATCH] completion: add diff --color-moved[-ws]

[Matheus Tavares Bernardino]

On Thu, Feb 20, 2020 at 8:30 PM Matheus Tavares Bernardino
<matheus.bernardino@usp.br> wrote:
>
[...]
> (and there might even be a bug in bash-completion involving this [1]).

Oops, nevermind. I misread the issue, it's a problem with some of
bash-completion's pre-shipped completion functions, not regarding the
"complete" builtin. Sorry for the noise.

Re: [PATCH] completion: add diff --color-moved[-ws]

[Kirill Kolyshkin]

On Thu, 20 Feb 2020 at 15:30, Matheus Tavares Bernardino
<matheus.bernardino@usp.br> wrote:
>
> On Thu, Feb 20, 2020 at 6:47 PM Kir Kolyshkin <kolyshkin@gmail.com> wrote:
> >
> > These options are available since git v2.15, but somehow
> > eluded from the completion script.
> >
> > Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
> > ---
> >  contrib/completion/git-completion.bash | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> >
> > diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> > index 1aac5a56c0..43cb6a312d 100644
> > --- a/contrib/completion/git-completion.bash
> > +++ b/contrib/completion/git-completion.bash
> > @@ -1487,9 +1487,16 @@ __git_diff_algorithms="myers minimal patience histogram"
> >
> >  __git_diff_submodule_formats="diff log short"
> >
> > +__git_color_moved_opts="no default plain blocks zebra dimmed-zebra"
> > +
> > +__git_color_moved_ws_opts="no ignore-space-at-eol ignore-all-space
> > +                       allow-indentation-change"
>
> I think "ignore-space-change" is missing in the above list. Besides
> that, the patch LGTM.

Thanks for catching this, I'll send v2.

>
> As a side-note: when we have an option with an already filled value,
> e.g. `--color-moved-ws=allow-identation-change,`, although it accepts
> more values as a comma separated list, pressing <tab><tab> won't
> suggest others. But I think the helper functions in
> git-completion.bash don't provide an easy way to do that right now

That's right -- as much as I want to make --key=val[,val ...] work,
there is no (easy?) way to do that :(

> (and there might even be a bug in bash-completion involving this [1]).
> So, I think the patch is good as it is (just adding the missing item
> to that list) :)
>
> [1]: https://github.com/scop/bash-completion/issues/240

[PATCH v2] completion: add diff --color-moved[-ws]

[Kir Kolyshkin]

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]).

[1]: https://github.com/scop/bash-completion/issues/240

[v2: added missing ignore-space-change]

Acked-by: Matheus Tavares Bernardino <matheus.bernardino@usp.br>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
---
 contrib/completion/git-completion.bash | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 1aac5a56c0..348f0c0c57 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1487,9 +1487,16 @@ __git_diff_algorithms="myers minimal patience histogram"
 
 __git_diff_submodule_formats="diff log short"
 
+__git_color_moved_opts="no default plain blocks zebra dimmed-zebra"
+
+__git_color_moved_ws_opts="no ignore-space-at-eol ignore-space-change
+			ignore-all-space allow-indentation-change"
+
 __git_diff_common_options="--stat --numstat --shortstat --summary
 			--patch-with-stat --name-only --name-status --color
 			--no-color --color-words --no-renames --check
+			--color-moved --color-moved= --no-color-moved
+			--color-moved-ws= --no-color-moved-ws
 			--full-index --binary --abbrev --diff-filter=
 			--find-copies-harder --ignore-cr-at-eol
 			--text --ignore-space-at-eol --ignore-space-change
@@ -1520,6 +1527,14 @@ _git_diff ()
 		__gitcomp "$__git_diff_submodule_formats" "" "${cur##--submodule=}"
 		return
 		;;
+	--color-moved=*)
+		__gitcomp "$__git_color_moved_opts" "" "${cur##--color-moved=}"
+		return
+		;;
+	--color-moved-ws=*)
+		__gitcomp "$__git_color_moved_ws_opts" "" "${cur##--color-moved-ws=}"
+		return
+		;;
 	--*)
 		__gitcomp "--cached --staged --pickaxe-all --pickaxe-regex
 			--base --ours --theirs --no-index
-- 
2.24.1

Re: [PATCH v2] completion: add diff --color-moved[-ws]

[Matheus Tavares]

Hi, Kir

On Fri, Feb 21, 2020 at 5:15 PM Kir Kolyshkin <kolyshkin@gmail.com> 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 :)

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@usp.br>

If you are going to send a v3, please, also use
"Matheus Tavares <matheus.bernardino@usp.br>" instead (that's just how I
normally sign).

Re: [PATCH v2] completion: add diff --color-moved[-ws]

[Kirill Kolyshkin]

On Sat, 22 Feb 2020 at 21:06, Matheus Tavares <matheus.bernardino@usp.br> wrote:
>
> Hi, Kir
>
> On Fri, Feb 21, 2020 at 5:15 PM Kir Kolyshkin <kolyshkin@gmail.com> 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@usp.br>
>
> If you are going to send a v3, please, also use
> "Matheus Tavares <matheus.bernardino@usp.br>" instead (that's just how I
> normally sign).
>

Re: [PATCH v2] completion: add diff --color-moved[-ws]

[Matheus Tavares Bernardino]

On Mon, Feb 24, 2020 at 1:41 AM Kirill Kolyshkin <kolyshkin@gmail.com> wrote:
>
> On Sat, 22 Feb 2020 at 21:06, Matheus Tavares <matheus.bernardino@usp.br> wrote:
> >
> > On Fri, Feb 21, 2020 at 5:15 PM Kir Kolyshkin <kolyshkin@gmail.com> wrote:
> > >
> > > 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,
[...]
> 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".

In fact, it is possible to make it suggest the commas. But them, the
output would be too polluted, IMO, as we would get twice as much
suggestions. For example:

$ git diff --color-moved-ws=<tab><tab>
allow-indentation-change allow-indentation-change, ignore-all-space
ignore-all-space, ignore-space-at-eol ignore-space-at-eol,
ignore-space-change ignore-space-change, no no,

Besides, the completion without csv also does not help on the
"discovery mode" for lists of values. So I think it is still better to
use the csv completion, than not using it at all. I.e. there is no
additional downside, and we get the bonus of nice completions when the
user tries something as `git diff
--color-moved-ws=ignore-space-at-eol,<tab><tab>`.

> 2. It's not smart enough to not repeat the option that is already there,

Hmm, in my setup, it is not repeating options. I also added a test
case at t6120-describe.sh to make sure there is no repetition. Could
you please execute this test in your environment and report back if it
fails there? (maybe I'm using some non-portable code?)

For reference, the section that should be responsible for avoiding
repetitions is the following:

> > +       # 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

> 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.

I don't think completion should handle these cases. Take a look at the
completion for git-describe, for example. The option --long is
incompatible with --no-abbrev, but the completion script won't stop
suggesting one because the user already typed the other. IMO,
introducing all these rules to completion would make the script
unnecessarily complex. In the end, git-describe itself will warn
and/or exit if incompatible options are given; the completion is more
like a helper, IMO.

> So, all of the above makes me think that maybe it doesn't make
> much sense to have csv completion at all.

Hmm, I understand your point. But as I mentioned earlier, I think
adding the csv completion won't cause any extra harm; and as a bonus,
it will help users that already want to type a list of values to do it
faster. So I still think it is better than not using it at all.