[2/3] Optionally skip linking/copying the built-ins

Commit Message

Linus Arver via GitGitGadget Aug. 17, 2020, 9:07 a.m. UTC

The dashed form of the built-ins is so passé. To save on development
time, and to support the idea of eventually dropping the dashed form
altogether, let's introduce a Makefile knob to skip generating those
hard-links.

Signed-off-by: Johannes Schindelin <[e-mail]>
---
 Makefile | 53 ++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 36 insertions(+), 17 deletions(-)

Comments

Junio C Hamano Aug. 17, 2020, 6:19 p.m. UTC | #1

Johannes Schindelin <[e-mail]> writes:

> The dashed form of the built-ins is so passé. To save on development
> time, and to support the idea of eventually dropping the dashed form
> altogether, let's introduce a Makefile knob to skip generating those
> hard-links.
>
> Signed-off-by: Johannes Schindelin <[e-mail]>
> ---
>  Makefile | 53 ++++++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 36 insertions(+), 17 deletions(-)

I do not know passé is a good adjective to use for the past effort
of keeping the promise we made to our users, but I think in general
this as an optional installation knob is an excellent idea.

>  ### Check documentation
>  #
> -ALL_COMMANDS = $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS)
> +ALL_COMMANDS = $(ALL_PROGRAMS_AND_BUILT_INS) $(SCRIPT_LIB)
>  ALL_COMMANDS += git
>  ALL_COMMANDS += git-citool
>  ALL_COMMANDS += git-gui

This stops "make check-docs" from ensuring that the built-in
commands are documented when skip-dashed is requested, no?
The first action in check-docs target that runs lint-docs in the
Documentation directory may notice a missing documentation when
it is referenced by somebody else, but the check in the target
itself are told that these built-ins no longer exist and triggers
"removed but listed" errors.

A mistake clike the above an become harder to make if
ALL_PROGRAMS_AND_BUILT_INS is renamed to indicate what it really is
(which would also help its primary target, the installation step).
It obviously does NOT always include $(BUILT_INS), so it is not "all
programs and built-ins" but something else (perhaps "all programs
and built-ins that are installed on a filesystem as separate
executable files"?)

Thanks.

Johannes Schindelin Aug. 24, 2020, 2:58 p.m. UTC | #2

Hi Junio,

On Mon, 17 Aug 2020, Junio C Hamano wrote:

> Johannes Schindelin <[e-mail]> writes:
>
> > The dashed form of the built-ins is so passé. To save on development
> > time, and to support the idea of eventually dropping the dashed form
> > altogether, let's introduce a Makefile knob to skip generating those
> > hard-links.
> >
> > Signed-off-by: Johannes Schindelin <[e-mail]>
> > ---
> >  Makefile | 53 ++++++++++++++++++++++++++++++++++++-----------------
> >  1 file changed, 36 insertions(+), 17 deletions(-)
>
> I do not know passé is a good adjective to use for the past effort
> of keeping the promise we made to our users, but I think in general
> this as an optional installation knob is an excellent idea.

You're right. My frustration with related Git for Windows tickets got the
better of me. I hope that you'll like v2's commit message much better.

> >  ### Check documentation
> >  #
> > -ALL_COMMANDS = $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS)
> > +ALL_COMMANDS = $(ALL_PROGRAMS_AND_BUILT_INS) $(SCRIPT_LIB)
> >  ALL_COMMANDS += git
> >  ALL_COMMANDS += git-citool
> >  ALL_COMMANDS += git-gui
>
> This stops "make check-docs" from ensuring that the built-in
> commands are documented when skip-dashed is requested, no?
> The first action in check-docs target that runs lint-docs in the
> Documentation directory may notice a missing documentation when
> it is referenced by somebody else, but the check in the target
> itself are told that these built-ins no longer exist and triggers
> "removed but listed" errors.
>
> A mistake clike the above an become harder to make if
> ALL_PROGRAMS_AND_BUILT_INS is renamed to indicate what it really is
> (which would also help its primary target, the installation step).
> It obviously does NOT always include $(BUILT_INS), so it is not "all
> programs and built-ins" but something else (perhaps "all programs
> and built-ins that are installed on a filesystem as separate
> executable files"?)

Right, that's a very good point. I had assumed that `check-docs` would be
exercised by CI, but it wasn't... It's only exercised in the
`Documentation` job, which is run without Makefile knobs.

I fixed it in preparation for v2 of this patch series.

Ciao,
Dscho

Patch

diff --git a/Makefile b/Makefile
index 3f5ba97b70..2a355a4da8 100644
--- a/Makefile
+++ b/Makefile
@@ -348,6 +348,9 @@  all::
 # Define NO_INSTALL_HARDLINKS if you prefer to use either symbolic links or
 # copies to install built-in git commands e.g. git-cat-file.
 #
+# Define SKIP_DASHED_BUILT_INS if you do not need the dashed versions of the
+# built-ins to be linked/copied at all.
+#
 # Define USE_NED_ALLOCATOR if you want to replace the platforms default
 # memory allocators with the nedmalloc allocator written by Niall Douglas.
 #
@@ -775,6 +778,16 @@  BUILT_INS += git-whatchanged$X
 # what 'all' will build and 'install' will install in gitexecdir,
 # excluding programs for built-in commands
 ALL_PROGRAMS = $(PROGRAMS) $(SCRIPTS)
+ALL_PROGRAMS_AND_BUILT_INS = $(ALL_PROGRAMS)
+ifeq (,$(SKIP_DASHED_BUILT_INS))
+ALL_PROGRAMS_AND_BUILT_INS += $(BUILT_INS)
+else
+# git-upload-pack, git-receive-pack and git-upload-archive are special: they
+# are _expected_ to be present in the `bin/` directory in their dashed form.
+ALL_PROGRAMS_AND_BUILT_INS += git-receive-pack$(X)
+ALL_PROGRAMS_AND_BUILT_INS += git-upload-archive$(X)
+ALL_PROGRAMS_AND_BUILT_INS += git-upload-pack$(X)
+endif
 
 # what 'all' will build but not install in gitexecdir
 OTHER_PROGRAMS = git$X
@@ -2066,9 +2079,9 @@  profile-fast: profile-clean
 	$(MAKE) PROFILE=USE all
 
 
-all:: $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) $(OTHER_PROGRAMS) GIT-BUILD-OPTIONS
+all:: $(ALL_PROGRAMS_AND_BUILT_INS) $(SCRIPT_LIB) $(OTHER_PROGRAMS) GIT-BUILD-OPTIONS
 ifneq (,$X)
-	$(QUIET_BUILT_IN)$(foreach p,$(patsubst %$X,%,$(filter %$X,$(ALL_PROGRAMS) $(BUILT_INS) git$X)), test -d '$p' -o '$p' -ef '$p$X' || $(RM) '$p';)
+	$(QUIET_BUILT_IN)$(foreach p,$(patsubst %$X,%,$(filter %$X,$(ALL_PROGRAMS_AND_BUILT_INS) git$X)), test -d '$p' -o '$p' -ef '$p$X' || $(RM) '$p';)
 endif
 
 all::
@@ -2928,7 +2941,7 @@  ifndef NO_TCLTK
 	$(MAKE) -C git-gui gitexecdir='$(gitexec_instdir_SQ)' install
 endif
 ifneq (,$X)
-	$(foreach p,$(patsubst %$X,%,$(filter %$X,$(ALL_PROGRAMS) $(BUILT_INS) git$X)), test '$(DESTDIR_SQ)$(gitexec_instdir_SQ)/$p' -ef '$(DESTDIR_SQ)$(gitexec_instdir_SQ)/$p$X' || $(RM) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)/$p';)
+	$(foreach p,$(patsubst %$X,%,$(filter %$X,$(ALL_PROGRAMS_AND_BUILT_INS) git$X)), test '$(DESTDIR_SQ)$(gitexec_instdir_SQ)/$p' -ef '$(DESTDIR_SQ)$(gitexec_instdir_SQ)/$p$X' || $(RM) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)/$p';)
 endif
 
 	bindir=$$(cd '$(DESTDIR_SQ)$(bindir_SQ)' && pwd) && \
@@ -2946,21 +2959,27 @@  endif
 	} && \
 	for p in $(filter $(install_bindir_programs),$(BUILT_INS)); do \
 		$(RM) "$$bindir/$$p" && \
-		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; } \
+		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 \
 	done && \
 	for p in $(BUILT_INS); do \
 		$(RM) "$$execdir/$$p" && \
-		test -n "$(INSTALL_SYMLINKS)" && \
-		ln -s "$$destdir_from_execdir_SQ/$(bindir_relative_SQ)/git$X" "$$execdir/$$p" || \
-		{ test -z "$(NO_INSTALL_HARDLINKS)" && \
-		  ln "$$execdir/git$X" "$$execdir/$$p" 2>/dev/null || \
-		  ln -s "git$X" "$$execdir/$$p" 2>/dev/null || \
-		  cp "$$execdir/git$X" "$$execdir/$$p" || exit; } \
+		if test -z "$(SKIP_DASHED_BUILT_INS)"; \
+		then \
+			test -n "$(INSTALL_SYMLINKS)" && \
+			ln -s "$$destdir_from_execdir_SQ/$(bindir_relative_SQ)/git$X" "$$execdir/$$p" || \
+			{ test -z "$(NO_INSTALL_HARDLINKS)" && \
+			  ln "$$execdir/git$X" "$$execdir/$$p" 2>/dev/null || \
+			  ln -s "git$X" "$$execdir/$$p" 2>/dev/null || \
+			  cp "$$execdir/git$X" "$$execdir/$$p" || exit; }; \
+		fi \
 	done && \
 	remote_curl_aliases="$(REMOTE_CURL_ALIASES)" && \
 	for p in $$remote_curl_aliases; do \
@@ -3051,7 +3070,7 @@  ifneq ($(INCLUDE_DLLS_IN_ARTIFACTS),)
 OTHER_PROGRAMS += $(shell echo *.dll t/helper/*.dll)
 endif
 
-artifacts-tar:: $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) $(OTHER_PROGRAMS) \
+artifacts-tar:: $(ALL_PROGRAMS_AND_BUILT_INS) $(SCRIPT_LIB) $(OTHER_PROGRAMS) \
 		GIT-BUILD-OPTIONS $(TEST_PROGRAMS) $(test_bindir_programs) \
 		$(MOFILES)
 	$(QUIET_SUBDIR0)templates $(QUIET_SUBDIR1) \
@@ -3146,7 +3165,7 @@  endif
 
 ### Check documentation
 #
-ALL_COMMANDS = $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS)
+ALL_COMMANDS = $(ALL_PROGRAMS_AND_BUILT_INS) $(SCRIPT_LIB)
 ALL_COMMANDS += git
 ALL_COMMANDS += git-citool
 ALL_COMMANDS += git-gui