Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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.
