diff mbox series

SKIP_DASHED_BUILT_INS: do not skip the bin/ programs

Message ID pull.768.git.1603293211428.gitgitgadget@gmail.com (mailing list archive)
State Accepted
Commit 907e6379d0af4c5412b73dd2f3799ecc634cf059
Headers show
Series SKIP_DASHED_BUILT_INS: do not skip the bin/ programs | expand

Commit Message

Johannes Schindelin Oct. 21, 2020, 3:13 p.m. UTC 
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(-)


base-commit: a5fa49ff0a8f3252c6bff49f92b85e7683868f8a

Comments

Taylor Blau Oct. 21, 2020, 5:25 p.m. UTC | #1 
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.

>  		$(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>

Thanks,
Taylor
Johannes Schindelin Oct. 22, 2020, 9:29 a.m. UTC | #2 
Hi Taylor,

On Wed, 21 Oct 2020, Taylor Blau wrote:

> 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`.

> >  		$(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
diff mbox series

Patch

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 \
 		$(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; }; \
 	done && \
 	for p in $(BUILT_INS); do \
 		$(RM) "$$execdir/$$p" && \