| Message ID | pull.1087.git.1638281655.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Use the built-in implementation of the interactive add command by default | expand |
On Tue, Nov 30, 2021 at 02:14:13PM +0000, Johannes Schindelin via GitGitGadget wrote: > Over two years ago, Slavica Đukić participated in the Outreachy project, > starting to implement a built-in version of the interactive git add command. > A little over a year ago, Git turned on that mode whenever users were > running with feature.experimental = true. > > It is time to declare this implementation robust, to use it by default, and > to start deprecating the scripted implementation. Yay. I agree it is time. It's still possible there are bugs that feature.experimental folks missed, but at some point we need to flip this switch to get the exposure to find those bugs. Doing it early in a cycle makes sense. The patches themselves look good to me. I look forward to dropping the perl version entirely, just to reduce the duplicated code, but I think your approach of leaving it as an escape hatch for now makes sense in the shorter term. -Peff
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes: > Over two years ago, Slavica Đukić participated in the Outreachy project, > starting to implement a built-in version of the interactive git add command. > A little over a year ago, Git turned on that mode whenever users were > running with feature.experimental = true. > > It is time to declare this implementation robust, to use it by default, and > to start deprecating the scripted implementation. Yes, it is time to use it by default to expose any remaining bugs that those with feature.experimental set failed to catch. Unfortunately, we do not catch issues real users would encounter in any "opt-in" feature, until it becomes "opt-out" and unconfigured users are exposed to it. This is true even in the presense of large corporations that expose their internal users to versions based on 'next' regularly.
On Tue, Nov 30 2021, Johannes Schindelin via GitGitGadget wrote: > Over two years ago, Slavica Đukić participated in the Outreachy project, > starting to implement a built-in version of the interactive git add command. > A little over a year ago, Git turned on that mode whenever users were > running with feature.experimental = true. > > It is time to declare this implementation robust, to use it by default, and > to start deprecating the scripted implementation. > > Johannes Schindelin (2): > t2016: require the PERL prereq only when necessary > add -i: default to the built-in implementation I'm very happy to see this. I left some minor nits on 2/2[1], but with/without those suggested changes this LGTM. I was a tad surprised that feature.experimental=false doesn't disable this anymore, but after looking into it a bit that's how we should be doing this. I.e. the life cycle for these has been opt in setting [&& experimental] -> opt-out setting [&& !experimental] -> remove opt-out If you're intending to re-roll anyway I think a brief mention of that being intended & correct would be nice. I.e. I went looking down that rabbit hole since there was no mention of it in the commit message, and wondered if it was intentional & correct, which I then found it is (well, correct, but I'm assuming also intentional). Thanks! 1. https://lore.kernel.org/git/211201.86pmqgbful.gmgdl@evledraar.gmail.com/
On Wed, Dec 1, 2021 at 12:40 AM Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com> wrote: > It is time to declare this implementation robust, to use it by default, and > to start deprecating the scripted implementation. > > Johannes Schindelin (2): > t2016: require the PERL prereq only when necessary > add -i: default to the built-in implementation Sadly this implementation has a few bugs that still need fixing, with at least one IMHO being a showstopper. The way macOS implements stdin (through a device) it will always timeout in poll(), so escape keys that are left in the unread buffer and that could match some of the entries will result in the wrong entry being selected. I have a series[1] that reimplements this and that seemed to work fine in my tests while making the code simpler, but that I didn't prioritize (and wanted to clean up further) since I wanted to prioritize the EDITOR fixes in the same area. Carlo [1] https://github.com/git/git/pull/1150
Hi Carlo, On Wed, 1 Dec 2021, Carlo Arenas wrote: > On Wed, Dec 1, 2021 at 12:40 AM Johannes Schindelin via GitGitGadget > <gitgitgadget@gmail.com> wrote: > > It is time to declare this implementation robust, to use it by default, and > > to start deprecating the scripted implementation. > > > > Johannes Schindelin (2): > > t2016: require the PERL prereq only when necessary > > add -i: default to the built-in implementation > > Sadly this implementation has a few bugs that still need fixing, with > at least one IMHO being a showstopper. > > The way macOS implements stdin (through a device) it will always > timeout in poll(), so escape keys that are left in the unread buffer > and that could match some of the entries will result in the wrong > entry being selected. > > I have a series[1] that reimplements this and that seemed to work fine > in my tests while making the code simpler, but that I didn't > prioritize (and wanted to clean up further) since I wanted to > prioritize the EDITOR fixes in the same area. > > Carlo > > [1] https://github.com/git/git/pull/1150 Thank you for pointing that out. I agree both with prioritizing your macOS patches, and with prioritizing the editor patches before that. Please just let me know when would be a good time to move forward with this here patch series. Thank you, Dscho
Hi Dscho, Le 2021-11-30 à 09:14, Johannes Schindelin via GitGitGadget a écrit : > Over two years ago, Slavica Đukić participated in the Outreachy project, > starting to implement a built-in version of the interactive git add command. > A little over a year ago, Git turned on that mode whenever users were > running with feature.experimental = true. > > It is time to declare this implementation robust, to use it by default, and > to start deprecating the scripted implementation. > > Johannes Schindelin (2): > t2016: require the PERL prereq only when necessary > add -i: default to the built-in implementation > > Documentation/config/add.txt | 6 +++--- > builtin/add.c | 15 +++++-------- > ci/run-build-and-tests.sh | 2 +- > t/README | 2 +- > t/t2016-checkout-patch.sh | 42 +++++++++++++++++++----------------- > 5 files changed, 32 insertions(+), 35 deletions(-) I just noticed that 'INSTALL' mentions that Perl is needed for 'git add interactive' et al, so maybe we would want to tweak the wording a bit in there when switch the default to the C version ? Cheers, Philippe.
Hi Philippe, On Fri, 3 Dec 2021, Philippe Blain wrote: > Le 2021-11-30 à 09:14, Johannes Schindelin via GitGitGadget a écrit : > > Over two years ago, Slavica Đukić participated in the Outreachy project, > > starting to implement a built-in version of the interactive git add command. > > A little over a year ago, Git turned on that mode whenever users were > > running with feature.experimental = true. > > > > It is time to declare this implementation robust, to use it by default, and > > to start deprecating the scripted implementation. > > > > Johannes Schindelin (2): > > t2016: require the PERL prereq only when necessary > > add -i: default to the built-in implementation > > > > Documentation/config/add.txt | 6 +++--- > > builtin/add.c | 15 +++++-------- > > ci/run-build-and-tests.sh | 2 +- > > t/README | 2 +- > > t/t2016-checkout-patch.sh | 42 +++++++++++++++++++----------------- > > 5 files changed, 32 insertions(+), 35 deletions(-) > > I just noticed that 'INSTALL' mentions that Perl is needed for 'git add > interactive' > et al, so maybe we would want to tweak the wording a bit in there when switch > the default > to the C version ? Not yet. Only once we remove `git-add--interactive.perl`. Thanks, Dscho