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

From: Junio C Hamano <[email-masked]>
Date: Wed, 01 Dec 2021 14:41:47 -0800

Ævar Arnfjörð Bjarmason <[email-masked]> 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.