On Thu, Aug 13, 2020 at 10:55:15AM -0400, Jeff King wrote: > A while ago I was looking at the size of an installed (and stripped) > build of Git, and noticed we could make a few more things into builtins. > > Patches 2-4 do that. I don't think there's any real downside here. > We're not bringing in any new link dependencies to the main binary. And > even if these programs don't use all of the code that's in the main > binary, operating systems are good at just loading the pages that we do > need (and you're probably running other Git programs anyway, which means > the main binary is in RAM anyway and we're actually reducing the overall > memory footprint). > > The final patch is not strictly related, but it's something I've had > sitting around for a long time. I actually posted it almost exactly two > years ago: > > https://lore.kernel.org/git/20180817190310.GA5360@xxxxxxxxxxxxxxxxxxxxx/ > > which ended with Jonathan saying: > > > - keeping this in-tree for the benefit of just one user is excessive, > > so removing it is probably the right thing > > > > - it would be nice if the commit removing this code from Git includes > > a note to help people find its new home > > > > Would you mind holding off until I'm able to arrange that last bit? > > So...I held off for a bit, but I'd really like to be done with this (and > I prefer dropping the code, because I have a few other tree-wide > cleanups for which I'd just as soon not have to deal with vcs-svn). > > [1/5]: Makefile: drop builtins from MSVC pdb list > [2/5]: make credential helpers builtins > [3/5]: make git-bugreport a builtin > [4/5]: make git-fast-import a builtin > [5/5]: drop vcs-svn experiment > > .gitignore | 1 - > Makefile | 45 +- > builtin.h | 5 + > bugreport.c => builtin/bugreport.c | 10 +- > .../credential-cache--daemon.c | 29 +- > .../credential-cache.c | 29 +- > .../credential-store.c | 6 +- > fast-import.c => builtin/fast-import.c | 3 +- > contrib/buildsystems/CMakeLists.txt | 52 +- > contrib/svn-fe/.gitignore | 4 - > contrib/svn-fe/Makefile | 105 -- > contrib/svn-fe/svn-fe.c | 18 - > contrib/svn-fe/svn-fe.txt | 71 -- > contrib/svn-fe/svnrdump_sim.py | 68 - > git.c | 5 + > remote-testsvn.c | 341 ----- > t/helper/.gitignore | 2 - > t/helper/test-line-buffer.c | 81 -- > t/helper/test-svn-fe.c | 52 - > t/t0081-line-buffer.sh | 90 -- > t/t9010-svn-fe.sh | 1105 ----------------- > t/t9011-svn-da.sh | 248 ---- > t/t9020-remote-svn.sh | 95 -- > t/test-lib-functions.sh | 2 +- > vcs-svn/LICENSE | 32 - > vcs-svn/fast_export.c | 365 ------ > vcs-svn/line_buffer.c | 126 -- > vcs-svn/line_buffer.txt | 77 -- > vcs-svn/sliding_window.c | 79 -- > vcs-svn/svndiff.c | 309 ----- > vcs-svn/svndump.c | 540 -------- > 31 files changed, 80 insertions(+), 3915 deletions(-) > rename bugreport.c => builtin/bugreport.c (96%) > rename credential-cache--daemon.c => builtin/credential-cache--daemon.c (91%) > rename credential-cache.c => builtin/credential-cache.c (83%) > rename credential-store.c => builtin/credential-store.c (96%) > rename fast-import.c => builtin/fast-import.c (99%) > delete mode 100644 contrib/svn-fe/.gitignore > delete mode 100644 contrib/svn-fe/Makefile > delete mode 100644 contrib/svn-fe/svn-fe.c > delete mode 100644 contrib/svn-fe/svn-fe.txt > delete mode 100755 contrib/svn-fe/svnrdump_sim.py > delete mode 100644 remote-testsvn.c > delete mode 100644 t/helper/test-line-buffer.c > delete mode 100644 t/helper/test-svn-fe.c > delete mode 100755 t/t0081-line-buffer.sh > delete mode 100755 t/t9010-svn-fe.sh > delete mode 100755 t/t9011-svn-da.sh > delete mode 100755 t/t9020-remote-svn.sh > delete mode 100644 vcs-svn/LICENSE > delete mode 100644 vcs-svn/fast_export.c > delete mode 100644 vcs-svn/line_buffer.c > delete mode 100644 vcs-svn/line_buffer.txt > delete mode 100644 vcs-svn/sliding_window.c > delete mode 100644 vcs-svn/svndiff.c > delete mode 100644 vcs-svn/svndump.c Looks all very good to me, and I'm certainly a fan of being able to drop the installation size down substantially as you have done here. I didn't comment on patches 3 and 4, since they are obviously correct and a good thing to be doing. It would be interesting to think about how we might prevent someone from introducing something that should be a builtin and has to link against libgit.a other than looking for it carefully in review, but that's definitely outside the scope of what you're trying to do here. Thanks. Taylor
