Re: [PATCH 2/2] add -i: default to the built-in implementation

From: Junio C Hamano <gitster@pobox.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>,
	git@vger.kernel.org, "Slavica Đukić" <slawica92@hotmail.com>,
	"Denton Liu" <liu.denton@gmail.com>,
	"Johannes Schindelin" <johannes.schindelin@gmx.de>
Subject: Re: [PATCH 2/2] add -i: default to the built-in implementation
Date: Wed, 01 Dec 2021 14:41:47 -0800	[thread overview]
Message-ID: <xmqqa6hkyml0.fsf@gitster.g> (raw)
In-Reply-To: <211201.86pmqgbful.gmgdl@evledraar.gmail.com> (=?utf-8?B?IsOG?= =?utf-8?B?dmFyIEFybmZqw7Zyw7A=?= Bjarmason"'s message of "Wed, 01 Dec 2021 14:37:47 +0100")

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

>>  add.interactive.useBuiltin::
>> -	[EXPERIMENTAL] Set to `true` to use the experimental built-in
>> -	implementation of the interactive version of linkgit:git-add[1]
>> -	instead of the Perl script version. Is `false` by default.
>> +	Set to `false` to fall back to the original Perl implementation of
>> +	the interactive version of linkgit:git-add[1] instead of the built-in
>> +	version. Is `true` by default.
>
> I think this would be a bit better if we just stole the version you
> added for stash.useBuiltin entirely. I.e. from your 336ad8424cb (stash:
> document stash.useBuiltin, 2019-05-14), with the relevant s/shell
> script/Perl/g etc. replaced.
>
> I.e. that version encouraged users to report any bugs, because we were
> really going to remove it soon, as we then did for rebase.useBuiltin in
> 9bcde4d5314 (rebase: remove transitory rebase.useBuiltin setting & env,
> 2021-03-23).
>
> The wording in the opening paragraph is also a bit more to the point
> there, i.e. calling it "legacy" rather than "original [...]
> implementation".

After reading the description on stash.useBuiltin, I'd agree with
you, except I do not see the distinction between "original" vs
"legacy".  I very much like the way the last paragraph for
"stash.useBuiltin" delivers the right message.

    +If you find some reason to set this option to `false`, other than
    +one-off testing, you should report the behavior difference as a bug in
    +Git (see https://git-scm.com/community for details).

>> -	if (use_builtin_add_i == 1) {
>> +	if (use_builtin_add_i != 0) {
>
> Style/idiom: This should just be "if (use_builtin_add_i)".
>
> I.e. before we cared about not catching -1 here, but now that it's true
> by default we don't care about the distinction between -1 or 1 anymore,
> we just want it not to be 0 here.

Yes, if we are redoing this step anyway, we should lose the explicit
comparison with zero here.