Re: [PATCH] completion: optionally disable checkout DWIM

From: SZEDER Gábor <szeder.dev@gmail.com>
Date: Apr 22, 2017, 17:32 UTC
Message-ID: <CAM0VKj=j52hDD1TKqbube3Of1qo8zZ7e4q4yAmxbNORRRDZyFQ@mail.gmail.com>
In-Reply-To: <20170421201957.ztwnyw6mut4b5u53@sigill.intra.peff.net>

On Fri, Apr 21, 2017 at 10:19 PM, Jeff King <peff@peff.net> wrote:

Show 24 quoted lines

> On Fri, Apr 21, 2017 at 10:14:48PM +0200, SZEDER Gábor wrote:
>
>> >> This is flexible enough for me, but it's possible somebody would want
>> >> this on a per-repo basis. I don't know that we want to read from `git
>> >> config`, though, because it's relatively expensive to do so. People who
>> >> want per-repo settings are probably better off with a hook that triggers
>> >> when they "cd" around, and sets up their preferences.
>>
>> We could discern between more than just empty vs. non-empty state of
>> the environment variable, e.g.:
>>
>>   - if empty/unset, then include "DWIM" suggestions.
>>   - if set to 'config', then query the 'completion.checkoutNoGuess'
>>     configuration variable, and omit "DWIM" suggestions if its true.
>>   - if set to something else, then omit "DWIM" suggestions.
>>
>> Then users can themselves decide, whether the per-repo configurability
>> is worth the overhead of running 'git config'.
>
> Yep, that would work. I wasn't going to bother with that complexity
> unless somebody really wanted it. The important thing is not to paint
> ourselves into a corner. By the rules you gave above, it would probably
> be fine to extend my patch later to match. But we could also be more
> specific (e.g., look for some positive value like "1").

That's fine with me.

We should have done this when we introduced env variables controlling the prompt script, we could spare a 'git config' execution in a subshell or two.

Show 14 quoted lines

>> >> @@ -1248,7 +1256,8 @@ _git_checkout ()
>> >>               # check if --track, --no-track, or --no-guess was specified
>> >>               # if so, disable DWIM mode
>> >>               local flags="--track --no-track --no-guess" track_opt="--track"
>> >> -             if [ -n "$(__git_find_on_cmdline "$flags")" ]; then
>> >> +             if [ -n "$GIT_COMPLETION_CHECKOUT_NO_GUESS" -o \
>> >> +                  -n "$(__git_find_on_cmdline "$flags")" ]; then
>>
>> || would be better than '-o', because the former short-circuits when
>> the first condition is true, but the latter doesn't.
>
> Ah, I didn't know that. Usually I use "||", but I thought "-o" was
> generally preferred in bash-specific scripts. We definitely want to
> short circuit here.

No, we use || and && fairly consistently in conditions in Bash scripts. There is no '-o' in the completion or prompt scripts, and there is only a single '-a' in the former, but it only checks two one-letter variables, so it doesn't matter. (Besides, it's from my second ever commit, so it has great sentimental value ;)

There are some inconsistencies using 'if [ ... ]' and 'if [[ ... ]]', but oh well.

Anyway, v2 looks good to me.