Re: [PATCH 1/2] parse-options: allow for configuring option abbreviation

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Tue, Mar 26 2019, Duy Nguyen wrote:

> On Tue, Mar 26, 2019 at 5:48 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>>
>>
>> On Mon, Mar 25 2019, Eric Sunshine wrote:
>>
>> > On Mon, Mar 25, 2019 at 4:23 PM Ævar Arnfjörð Bjarmason
>> > <avarab@gmail.com> wrote:
>> >> diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt
>> >> @@ -1,3 +1,15 @@
>> >> +core.abbreviatedOptions::
>> >> +       Defaults to `true` which allows options to be abbreviated as
>> >> +       long as they aren't ambiguous, e.g. for linkgit:git-init[1]
>> >> +       the `--bare` option can be abbreviated as `--bar`, `--ba` or
>> >> +       even `--b` since no other option starts with those
>> >> +       prefixes. However, if such an option were added in the future
>> >> +       any use of these abbreviations would break.
>> >> ++
>> >> +By setting this to false (e.g. in scripts) you can defend against such
>> >> +future breakages by enforcing that options must always be fully
>> >> +provided.
>> >
>> > I don't get why having a configuration option is better for defending
>> > scripts against this problem than a simple environment variable. It
>> > seems easier for the script prologue to contain:
>> >
>> >     GIT_TEST_ABBREVIATED_OPTIONS=false
>> >     export GIT_TEST_ABBREVIATED_OPTIONS
>> >
>> > than for it to muck about with git-config or use "git -c
>> > core.abbreviatedOptions=false ..." everywhere. The commit message
>> > doesn't do a good enough job of justifying the configuration option
>> > over the environment variable.
>> >
>> > Also, if this is now intended to be more general (aiding script
>> > writers) than just being for our test suite, then dropping "TEST" from
>> > the name seems warranted:
>> >
>> >     GIT_ABBREVIATED_OPTIONS
>>
>> If we want to make something user-configurable we tend to add config
>> variables. The GIT_TEST_* variables are only intended for our own test
>> suite, see t/README.
>>
>> I don't mind documenting this, but it's a well-established pattern, so
>> if we're going to describe how this works/why use one or the other it
>> should probably be some other series to t/README and/or git-config.txt
>>
>> We traditionally *only* expose this sort of thing to users via config,
>> and not via env variables.

FWIW I replied so quickly to this with patches since I had something WIP
to do this, it's annoyed me as a user in the past that I couldn't turn
this off, IIRC some upgrade of git broke my "bad" muscle memory /
scripts.

> If this is mostly useful for scripts then I agree with Eric an
> environment variable is the way to go. A configuration variable does
> not make it more convenient.

I think both of you might be assuming that when you want to configure
something it's as easy to tweak every run time environment (set an env
var) as it is to set a global config. See the trace2.* config discussion
for similar use-cases.

> And no we don't only export via config. There are a bunch of public
> env variables in git.txt. "core" namespace is already very crowded. If
> this one is only rarely used, I'd rather not add a new config
> variable.

I don't see how a new "abbreviatedOptions" is plausibly going to crowd
out anything else, sounds pretty unambiguous to me.

By my count out of the the existing GIT_* variables in git.txt around
1/3 are already configurable via config, another 1/3 (all the GIT_TRACE
stuff) is something we've wanted to have configurable in the past (but
nobody's gotten around to writing patches for).

I think it's fair to say that when we normally add user-configurable
stuff we do it as config, not as new env vars.