Re: [PATCH] SKIP_DASHED_BUILT_INS: do not skip the bin/ programs
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Oct 22, 2020, 09:29 UTC
- Message-ID
- <nycvar.QRO.7.76.6.2010221126240.56@tvgsbejvaqbjf.bet>
- In-Reply-To
- <20201021172516.GB1237181@nand.local>
Hi Taylor,
On Wed, 21 Oct 2020, Taylor Blau wrote:
Show 43 quoted lines
> On Wed, Oct 21, 2020 at 03:13:31PM +0000, Johannes Schindelin via GitGitGadget wrote: > > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > > > The idea of the `SKIP_DASHED_BUILT_INS` option is to stop hard-linking > > the built-in commands as separate executables. The patches to do that > > specifically excluded the three commands `receive-pack`, > > `upload-archive` and `upload-pack`, though: these commands are expected > > to be present in the `PATH` in their dashed form on the server side of > > any fetch/push. > > > > However, due to an oversight by myself, even if those commands were > > still hard-linked, they were not installed into `bin/`. > > > > Noticed-by: Michael Forney <mforney@mforney.org> > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > > --- > > SKIP_DASHED_BUILT_INS: do not skip the bin/ programs > > > > As reported by Michael in > > https://lore.kernel.org/git/CAGw6cBsEjOnh-ZqXCPfFha=NYEdy7JDddha=UzAau0Z1tBrWKg@mail.gmail.com > > , the SKIP_DASHED_BUILT_INS feature had a bug I wish we had caught in > > the -rc cycle. But at least SKIP_DASHED_BUILT_INS is still strictly > > opt-in, so the damage is somewhat contained. > > > > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-768%2Fdscho%2Fskip-dashed-built-ins-still-must-install-install_bindir_programs-v1 > > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-768/dscho/skip-dashed-built-ins-still-must-install-install_bindir_programs-v1 > > Pull-Request: https://github.com/gitgitgadget/git/pull/768 > > > > Makefile | 15 ++++++--------- > > 1 file changed, 6 insertions(+), 9 deletions(-) > > > > diff --git a/Makefile b/Makefile > > index 95571ee3fc..1fb0ec1705 100644 > > --- a/Makefile > > +++ b/Makefile > > @@ -2981,15 +2981,12 @@ endif > > } && \ > > for p in $(filter $(install_bindir_programs),$(BUILT_INS)); do \ > > This does still install 'git-shell' into my installation's 'bin' > directory. I'm not up-to-date whether or not we mean to still be > installing that, but, doing the same 'SKIP_DASHED_BUILT_INS=1 make > install' on 'master' also installs git-shell, so I'm fine with that.
Yes, `git-shell` is not a built-in, that's why.
It is added as `PROGRAM_OBJS` in the `Makefile`, and it has its own small `cmd_main()`. On purpose: the idea for `git-shell` is to serve as a pseudo login shell for `git fetch`/`git push` to do the minimum amount of work to ensure that only `git receive-pack`, `git upload-pack` and `git upload-archive` commands are allowed. Everything else is blocked.
So yes, it is installed into `bin/`, expressly so, and it is not a hard-linked copy of `bin/git`.
Show 33 quoted lines
> > $(RM) "$$bindir/$$p" && \
> > - if test -z "$(SKIP_DASHED_BUILT_INS)"; \
> > - then \
> > - test -n "$(INSTALL_SYMLINKS)" && \
> > - ln -s "git$X" "$$bindir/$$p" || \
> > - { test -z "$(NO_INSTALL_HARDLINKS)" && \
> > - ln "$$bindir/git$X" "$$bindir/$$p" 2>/dev/null || \
> > - ln -s "git$X" "$$bindir/$$p" 2>/dev/null || \
> > - cp "$$bindir/git$X" "$$bindir/$$p" || exit; }; \
> > - fi \
> > + test -n "$(INSTALL_SYMLINKS)" && \
> > + ln -s "git$X" "$$bindir/$$p" || \
> > + { test -z "$(NO_INSTALL_HARDLINKS)" && \
> > + ln "$$bindir/git$X" "$$bindir/$$p" 2>/dev/null || \
> > + ln -s "git$X" "$$bindir/$$p" 2>/dev/null || \
> > + cp "$$bindir/git$X" "$$bindir/$$p" || exit; }; \
>
> OK. $(install_bindir_programs) is the set of programs that we want to
> install; that makes sense. And for those, we definitely don't want to
> skip them because SKIP_DASHED_BUILT_INS was set. So removing that test
> makes sense to me.
>
> > done && \
> > for p in $(BUILT_INS); do \
> > $(RM) "$$execdir/$$p" && \
> >
> > base-commit: a5fa49ff0a8f3252c6bff49f92b85e7683868f8a
> > --
> > gitgitgadget
>
> Looks great, thanks for turning my thoughts into a real patch.
>
> Reviewed-by: Taylor Blau <me@ttaylorr.com>Awesome, thanks! Dscho