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

From: Matheus Tavares Bernardino <matheus.bernardino@usp.br>
Date: Feb 24, 2020, 06:02 UTC
Message-ID: <CAHd-oW72kMzYqcRMffQRb+VB8=HdQt8noJgAfGYxpEsWZdbMuA@mail.gmail.com>
In-Reply-To: <CAGmPdrxRNVSvQYzOx9-nhtztGgS_q92y3GO8n1nqRE5UJWiQRg@mail.gmail.com>

On Mon, Feb 24, 2020 at 1:41 AM Kirill Kolyshkin <kolyshkin@gmail.com> wrote:

Show 11 quoted lines

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

Show 8 quoted lines

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

Show 8 quoted lines

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