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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 1, 2021, 22:41 UTC
Message-ID: <xmqqa6hkyml0.fsf@gitster.g>
In-Reply-To: <211201.86pmqgbful.gmgdl@evledraar.gmail.com>

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

Show 21 quoted lines

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

Show 8 quoted lines

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