Skip to contentSkip to conversation

SKIP_DASHED_BUILT_INS: do not skip the bin/ programs

PATCH In review Johannes Schindelin via GitGitGadget 3 participants 3 messages Oct 21, 2020 – Oct 22, 2020 Kbuild

lore
Share & tools
lore

Johannes Schindelin via GitGitGadget

author
#0 lore raw

PATCH 1/1 SKIP_DASHED_BUILT_INS: do not skip the bin/ programs

From: Johannes Schindelin <[email protected]>

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 <[email protected]>
Signed-off-by Johannes Schindelin <[email protected]>

---
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(-)
Makefile +6 -9
index 95571ee3fc..1fb0ec1705 100644
@@ -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" && \

base-commit: a5fa49ff0a8f3252c6bff49f92b85e7683868f8a

Signature · 1 line
gitgitgadget
Reviewed-by on v1 1/1
#1 lore raw
Quoted 35 lines · Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

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 <[email protected]>
Signed-off-by Johannes Schindelin <[email protected]>

---
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(-)
Makefile +0 -0
index 95571ee3fc..1fb0ec1705 100644

Makefile endif · line 2982

Quoted 2 lines
Makefile +0 -0
@@ -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.

Quoted 6 lines
Makefile +0 -5
 		$(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)" && \

Makefile endif · line 2989

Quoted 10 lines
Makefile +6 -4
-			  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.

Quoted 7 lines
Makefile +0 -0
 	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 <[email protected]>

Thanks,
Taylor

#2 lore raw

Hi Taylor,

Quoted 31 lines · Taylor Blau
Quoted 30 lines · Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

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 <[email protected]>
Signed-off-by Johannes Schindelin <[email protected]>

---
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(-)
Quoted 12 lines

Makefile endif · line 2982

Quoted 7 lines
Makefile +0 -0
index 95571ee3fc..1fb0ec1705 100644
@@ -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.

Quoted 26 lines
Quoted 6 lines
Makefile +0 -5
 		$(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)" && \

Makefile endif · line 2989

Quoted 10 lines
Makefile +6 -4
-			  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.

Quoted 4 lines
Makefile +0 -0
 	done && \
 	for p in $(BUILT_INS); do \
 		$(RM) "$$execdir/$$p" && \
Quoted 7 lines
Quoted 3 lines

base-commit: a5fa49ff0a8f3252c6bff49f92b85e7683868f8a --
gitgitgadget

Looks great, thanks for turning my thoughts into a real patch.

Reviewed-by Taylor Blau <[email protected]>

Awesome, thanks!
Dscho