Optionally skip linking/copying the built-ins

The dashed form of the built-ins is so passé.

Incidentally, this also handles the .pdb issue in MSVC's install Makefile target that Peff pointed out in the context of the "slimming down" patch series
[https://lore.kernel.org/git/[email protected]/]
.

This addresses https://github.com/gitgitgadget/git/issues/406

Johannes Schindelin (3):
msvc: copy the correct .pdb files in the Makefile target install Optionally skip linking/copying the built-ins
ci: stop linking built-ins to the dashed versions

 Makefile                  | 69 +++++++++++++++++++++------------------
 ci/run-build-and-tests.sh |  2 +-
 2 files changed, 39 insertions(+), 32 deletions(-)

base-commit: b6a658bd00c9c29e07f833cabfc0ef12224e277a
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-411%2Fdscho%2Foptionally-skip-dashed-built-ins-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-411/dscho/optionally-skip-dashed-built-ins-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/411

gitgitgadget

There is a hard-coded list of .pdb files to copy. But we are about to introduce the SKIP_DASHED_BUILT_INS knob in the Makefile, which might make this hard-coded list incorrect.

Let's switch to a dynamically-generated list instead.

---
 Makefile | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

index 372139f1f2..3f5ba97b70 100644
@@ -2899,20 +2899,8 @@ ifdef MSVC
 	# have already been rolled up into the exe's pdb file.
 	# We DO NOT have pdb files for the builtin commands (like git-status.exe)
 	# because it is just a copy/hardlink of git.exe, rather than a unique binary.
-	$(INSTALL) git.pdb '$(DESTDIR_SQ)$(bindir_SQ)'
-	$(INSTALL) git-shell.pdb '$(DESTDIR_SQ)$(bindir_SQ)'
-	$(INSTALL) git-upload-pack.pdb '$(DESTDIR_SQ)$(bindir_SQ)'
-	$(INSTALL) git-credential-store.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
-	$(INSTALL) git-daemon.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
-	$(INSTALL) git-fast-import.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
-	$(INSTALL) git-http-backend.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
-	$(INSTALL) git-http-fetch.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
-	$(INSTALL) git-http-push.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
-	$(INSTALL) git-imap-send.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
-	$(INSTALL) git-remote-http.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
-	$(INSTALL) git-remote-testsvn.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
-	$(INSTALL) git-sh-i18n--envsubst.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
-	$(INSTALL) git-show-index.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
+	$(INSTALL) $(patsubst %.exe,%.pdb,$(filter-out $(BUILT_INS),$(patsubst %,%$X,$(BINDIR_PROGRAMS_NEED_X)))) '$(DESTDIR_SQ)$(bindir_SQ)'
+	$(INSTALL) $(patsubst %.exe,%.pdb,$(filter-out $(BUILT_INS) $(REMOTE_CURL_ALIASES),$(PROGRAMS))) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
 ifndef DEBUG
 	$(INSTALL) $(vcpkg_rel_bin)/*.dll '$(DESTDIR_SQ)$(bindir_SQ)'
 	$(INSTALL) $(vcpkg_rel_bin)/*.pdb '$(DESTDIR_SQ)$(bindir_SQ)'

gitgitgadget

Hi Peff,

Please feel totally free to cherry-pick my 1/3 as your 1/5 (but please do fix up the author email address, if you don't mind).

I have no problem with my patch series depending on yours, to make merging easier.

Ciao,
Dscho

It doesn't look like that series otherwise needs a re-roll, so if it's OK with you, let's just have yours come on top (or independent, as the conflict is pretty easy).

-Peff

Hi Peff,

Sounds good to me!

Ciao,
Dscho

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.

---
 Makefile | 53 ++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 36 insertions(+), 17 deletions(-)

index 3f5ba97b70..2a355a4da8 100644
@@ -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

gitgitgadget

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.

Hi Junio,

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.

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

Originally, all of Git's subcommands were implemented in their own executable/script, using the naming scheme git-<command-name>. When more and more functionality was turned into built-in commands (i.e. the git executable could run them without spawning a separate process), for backwards-compatibility, we hard-link the git executable to git-<built-in> for every built-in.

This backwards-compatibility was needed to support scripts that called the dashed form, even if we deprecated that a _long_ time ago.

In preparation for eventually dropping those hard-links, teach the CI (and PR) builds to skip generating those hard-links.

---
 ci/run-build-and-tests.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 17e25aade9..b074db5c4b 100755
@@ -10,7 +10,7 @@ windows*) cmd //c mklink //j t\\.prove "$(cygpath -aw "$cache_dir/.prove")";;
 *) ln -s "$cache_dir/.prove" t/.prove;;
 esac
 
-make
+make SKIP_DASHED_BUILT_INS=YesPlease
 case "$jobname" in
 linux-gcc)
 	make test

gitgitgadget

The other day, I found this amusing (yes, I am a fan of Emacs).

https://medium.com/@steve.yegge/dear-google-cloud-your-deprecation-policy-is-killing-you-ee7525dc05dc

You do not have to set the policy of "eventuall dropping" here. The presence of the choice of not installing added in step [2/3] alone is a very good justification to include this patch in the series. Otherwise, we won't know if our test suite and remaining scripted Porcelain rely on the age old promise we made and have kept to the end users, as [2/3] requires all our users accept the breakage of the promise.

It may be a good idea to test both configurations, with or without SKIP_DASHED, at least for now, though.

Thanks.

Hi,

Please note that this GitGitGadget run did not work as intended. The intention of https://github.com/gitgitgadget/gitgitgadget/pull/296 was to use the actual author in the From: headers of the sent emails, with GitGitGadget mentioned in the Sender: header, but apparently this did not work, and I will be reverting that PR for the time being.

In short: please do not apply these patches as-are, unless adjusting the author email to match my email address.

Thank you,
Dscho

It is close ;-)

The author name is correctly on "From:" but not the address.

Hi Junio,

Yes, but the problem seems to be insurmountable, as I _think_ it is to prevent spammers from successfully sending "from abitrary email addresses".

GMail adds an X-Google-Original-From: header with the original From: header, and drops the Sender: header.

There _might_ be other SMTP servers out there that might allow us to do this for GitGitGadget, but I am wary of undermining anti-spam measures that way.

Ciao,
Dscho

At least, even with only the name correction, the threads were easier to locate. Perhaps you can leave the in-body From: in to help "git am" but keep the half-successful attempt to give the human readable name to humans who are reading in their MUA?

Thanks.

Hi Junio,

But if all you're interested in is the part before the actual email address, isn't "Johannes Schindelin via GitGitGadget <[email protected]>" almost identical to "Johannes Schindelin <[email protected]>"?

Sorry, I seem to be slow understanding you :-(

Ciao,
Dscho

Here is what I often see in my MUA.

O  [  75: Johannes Schindelin    ] Re: [PATCH v2 3/3] ci: stop linking bu...
OA [  20: Johannes Schindelin    ] pw/add-p-allowed-options-fix, was Re: ...

O [ 106: Johannes Schindelin via] [PATCH] git-gui: accommodate for inten... O [ 66: Johannes Schindelin via] [PATCH] mingw: improve performance of ...

The "via ..." part may change its length depending on how long the real real name is, but it is irritating and more importantly conveys harmful information, as I am trying to be as neutral as I can when reviewing patches no matteer how they are delivered.

The dashed form of the built-ins is so passé.

Incidentally, this also handles the .pdb issue in MSVC's install Makefile target that Peff pointed out in the context of the "slimming down" patch series
[https://lore.kernel.org/git/[email protected]/]
.

This addresses https://github.com/gitgitgadget/git/issues/406

Changes since v1:

* Fixed check-docs under SKIP_DASHED_BUILT_INS
* Renamed ALL_PROGRAMS_AND_BUILT_INS to ALL_COMMANDS_TO_INSTALL to reflect its purpose better.
* Revamped the commit message of patch 2/3 and 3/3.

Johannes Schindelin (3):
msvc: copy the correct .pdb files in the Makefile target install Optionally skip linking/copying the built-ins
ci: stop linking built-ins to the dashed versions

 Makefile                  | 71 +++++++++++++++++++++------------------
 ci/run-build-and-tests.sh |  2 +-
 2 files changed, 40 insertions(+), 33 deletions(-)

base-commit: 878e727637ec5815ccb3301eb994a54df95b21b8
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-411%2Fdscho%2Foptionally-skip-dashed-built-ins-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-411/dscho/optionally-skip-dashed-built-ins-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/411

Range-diff vs v1:

 1:  120d2bb3e4 = 1:  1880a0e4bf msvc: copy the correct `.pdb` files in the Makefile target `install`
 2:  647f49d62e ! 2:  166bd0d8fb Optionally skip linking/copying the built-ins
     @@ Metadata
       ## Commit message ##
          Optionally skip linking/copying the built-ins
      
     -    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.
     +    For a long time already, the non-dashed form of the built-ins is the
     +    recommended way to write scripts, i.e. it is better to call `git merge
     +    [...]` than to call `git-merge [...]`.
     +
     +    While Git still supports the dashed form (by hard-linking the `git`
     +    executable to the dashed name in `libexec/git-core/`), in practice, it
     +    is probably almost irrelevant.
     +
     +    In fact, some platforms (such as Windows) only started gaining
     +    meaningful Git support _after_ the dashed form was deprecated, and
     +    therefore one would expect that all this hard-linking is unnecessary on
     +    those platforms.
     +
     +    In addition to that, some programs that are regularly used to assess
     +    disk usage fail to realize that those are hard-links, and heavily
     +    overcount disk usage. Most notably, this was the case with Windows
     +    Explorer up until the last couple of Windows 10 versions.
     +
     +    To save on the time needed to hard-link these dashed commands, and to
     +    eventually stop shipping with those hard-links on Windows, let's
     +    introduce a Makefile knob to skip generating them.
      
          Signed-off-by: Johannes Schindelin <[email protected]>
      
     @@ Makefile: 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)
     ++ALL_COMMANDS_TO_INSTALL = $(ALL_PROGRAMS)
      +ifeq (,$(SKIP_DASHED_BUILT_INS))
     -+ALL_PROGRAMS_AND_BUILT_INS += $(BUILT_INS)
     ++ALL_COMMANDS_TO_INSTALL += $(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)
     ++ALL_COMMANDS_TO_INSTALL += git-receive-pack$(X)
     ++ALL_COMMANDS_TO_INSTALL += git-upload-archive$(X)
     ++ALL_COMMANDS_TO_INSTALL += git-upload-pack$(X)
      +endif
       
       # what 'all' will build but not install in gitexecdir
     @@ Makefile: profile-fast: profile-clean
       
       
      -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
     ++all:: $(ALL_COMMANDS_TO_INSTALL) $(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';)
     ++	$(QUIET_BUILT_IN)$(foreach p,$(patsubst %$X,%,$(filter %$X,$(ALL_COMMANDS_TO_INSTALL) git$X)), test -d '$p' -o '$p' -ef '$p$X' || $(RM) '$p';)
       endif
       
       all::
     @@ Makefile: ifndef NO_TCLTK
       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';)
     ++	$(foreach p,$(patsubst %$X,%,$(filter %$X,$(ALL_COMMANDS_TO_INSTALL) 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) && \
     @@ Makefile: ifneq ($(INCLUDE_DLLS_IN_ARTIFACTS),)
       endif
       
      -artifacts-tar:: $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) $(OTHER_PROGRAMS) \
     -+artifacts-tar:: $(ALL_PROGRAMS_AND_BUILT_INS) $(SCRIPT_LIB) $(OTHER_PROGRAMS) \
     ++artifacts-tar:: $(ALL_COMMANDS_TO_INSTALL) $(SCRIPT_LIB) $(OTHER_PROGRAMS) \
       		GIT-BUILD-OPTIONS $(TEST_PROGRAMS) $(test_bindir_programs) \
       		$(MOFILES)
       	$(QUIET_SUBDIR0)templates $(QUIET_SUBDIR1) \
     @@ Makefile: endif
       ### Check documentation
       #
      -ALL_COMMANDS = $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS)
     -+ALL_COMMANDS = $(ALL_PROGRAMS_AND_BUILT_INS) $(SCRIPT_LIB)
     ++ALL_COMMANDS = $(ALL_COMMANDS_TO_INSTALL) $(SCRIPT_LIB)
       ALL_COMMANDS += git
       ALL_COMMANDS += git-citool
       ALL_COMMANDS += git-gui
     +@@ Makefile: check-docs::
     + 		    -e 's/\.txt//'; \
     + 	) | while read how cmd; \
     + 	do \
     +-		case " $(patsubst %$X,%,$(ALL_COMMANDS) $(EXCLUDED_PROGRAMS)) " in \
     ++		case " $(patsubst %$X,%,$(ALL_COMMANDS) $(BUILT_INS) $(EXCLUDED_PROGRAMS)) " in \
     + 		*" $$cmd "*)	;; \
     + 		*) echo "removed but $$how: $$cmd" ;; \
     + 		esac; \

3: 1269d7ace8 ! 3: ea23ba5e26 ci: stop linking built-ins to the dashed versions
@@ Commit message
This backwards-compatibility was needed to support scripts that called the dashed form, even if we deprecated that a _long_ time ago.

- In preparation for eventually dropping those hard-links, teach the CI
+ For that reason, we just introduced a Makefile knob to skip linking
+ them. TO make sure that this keeps working, teach the CI
(and PR) builds to skip generating those hard-links.

gitgitgadget

From: Johannes Schindelin <[email protected]>

There is a hard-coded list of .pdb files to copy. But we are about to introduce the SKIP_DASHED_BUILT_INS knob in the Makefile, which might make this hard-coded list incorrect.

Let's switch to a dynamically-generated list instead.

---
 Makefile | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

index 65f8cfb236..66b6e076e2 100644
@@ -2899,20 +2899,8 @@ ifdef MSVC
 	# have already been rolled up into the exe's pdb file.
 	# We DO NOT have pdb files for the builtin commands (like git-status.exe)
 	# because it is just a copy/hardlink of git.exe, rather than a unique binary.
-	$(INSTALL) git.pdb '$(DESTDIR_SQ)$(bindir_SQ)'
-	$(INSTALL) git-shell.pdb '$(DESTDIR_SQ)$(bindir_SQ)'
-	$(INSTALL) git-upload-pack.pdb '$(DESTDIR_SQ)$(bindir_SQ)'
-	$(INSTALL) git-credential-store.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
-	$(INSTALL) git-daemon.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
-	$(INSTALL) git-fast-import.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
-	$(INSTALL) git-http-backend.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
-	$(INSTALL) git-http-fetch.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
-	$(INSTALL) git-http-push.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
-	$(INSTALL) git-imap-send.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
-	$(INSTALL) git-remote-http.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
-	$(INSTALL) git-remote-testsvn.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
-	$(INSTALL) git-sh-i18n--envsubst.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
-	$(INSTALL) git-show-index.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
+	$(INSTALL) $(patsubst %.exe,%.pdb,$(filter-out $(BUILT_INS),$(patsubst %,%$X,$(BINDIR_PROGRAMS_NEED_X)))) '$(DESTDIR_SQ)$(bindir_SQ)'
+	$(INSTALL) $(patsubst %.exe,%.pdb,$(filter-out $(BUILT_INS) $(REMOTE_CURL_ALIASES),$(PROGRAMS))) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
 ifndef DEBUG
 	$(INSTALL) $(vcpkg_rel_bin)/*.dll '$(DESTDIR_SQ)$(bindir_SQ)'
 	$(INSTALL) $(vcpkg_rel_bin)/*.pdb '$(DESTDIR_SQ)$(bindir_SQ)'

gitgitgadget

From: Johannes Schindelin <[email protected]>

For a long time already, the non-dashed form of the built-ins is the recommended way to write scripts, i.e. it is better to call git merge [...] than to call git-merge [...].

While Git still supports the dashed form (by hard-linking the git executable to the dashed name in libexec/git-core/), in practice, it is probably almost irrelevant.

In fact, some platforms (such as Windows) only started gaining meaningful Git support _after_ the dashed form was deprecated, and therefore one would expect that all this hard-linking is unnecessary on those platforms.

In addition to that, some programs that are regularly used to assess disk usage fail to realize that those are hard-links, and heavily overcount disk usage. Most notably, this was the case with Windows Explorer up until the last couple of Windows 10 versions.

To save on the time needed to hard-link these dashed commands, and to eventually stop shipping with those hard-links on Windows, let's introduce a Makefile knob to skip generating them.

---
 Makefile | 55 +++++++++++++++++++++++++++++++++++++------------------
 1 file changed, 37 insertions(+), 18 deletions(-)

index 66b6e076e2..0a09146fb3 100644
@@ -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_COMMANDS_TO_INSTALL = $(ALL_PROGRAMS)
+ifeq (,$(SKIP_DASHED_BUILT_INS))
+ALL_COMMANDS_TO_INSTALL += $(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_COMMANDS_TO_INSTALL += git-receive-pack$(X)
+ALL_COMMANDS_TO_INSTALL += git-upload-archive$(X)
+ALL_COMMANDS_TO_INSTALL += 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_COMMANDS_TO_INSTALL) $(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_COMMANDS_TO_INSTALL) 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_COMMANDS_TO_INSTALL) 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_COMMANDS_TO_INSTALL) $(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_COMMANDS_TO_INSTALL) $(SCRIPT_LIB)
 ALL_COMMANDS += git
 ALL_COMMANDS += git-citool
 ALL_COMMANDS += git-gui

@@ -3186,7 +3205,7 @@ check-docs::
 		    -e 's/\.txt//'; \
 	) | while read how cmd; \
 	do \
-		case " $(patsubst %$X,%,$(ALL_COMMANDS) $(EXCLUDED_PROGRAMS)) " in \
+		case " $(patsubst %$X,%,$(ALL_COMMANDS) $(BUILT_INS) $(EXCLUDED_PROGRAMS)) " in \
 		*" $$cmd "*)	;; \
 		*) echo "removed but $$how: $$cmd" ;; \
 		esac; \

gitgitgadget

"Johannes Schindelin via GitGitGadget" <[email protected]>

It is irrelevant when you have to say "probably" without facts, and this paragraph is not necessary to justify this option. I'd omit it.

We do care about keeping people's scripts working (even if they were written before Windows folks started using Git---those people who started using Git before that still exist ;-).

However, the above two paragraphs I would suggest to keep, as they do matter---it is a good justification to have this configurable. Windows folks won't be able to copy and use POSIX shell scripts written by folks before the Windows port of Git was started to become widely used anyway.

Hi Junio,

I would like to gently request to keep the sentence in, as it will provide me with the context when I stumble across this commit the next time.

That, however, I totally understand, and I think you're right, I should add this sentence (in one form or another).

Excellent!

Ciao,
Dscho

From: Johannes Schindelin <[email protected]>

Originally, all of Git's subcommands were implemented in their own executable/script, using the naming scheme git-<command-name>. When more and more functionality was turned into built-in commands (i.e. the git executable could run them without spawning a separate process), for backwards-compatibility, we hard-link the git executable to git-<built-in> for every built-in.

This backwards-compatibility was needed to support scripts that called the dashed form, even if we deprecated that a _long_ time ago.

For that reason, we just introduced a Makefile knob to skip linking them. TO make sure that this keeps working, teach the CI (and PR) builds to skip generating those hard-links.

---
 ci/run-build-and-tests.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 6c27b886b8..1df9402c3b 100755
@@ -10,7 +10,7 @@ windows*) cmd //c mklink //j t\\.prove "$(cygpath -aw "$cache_dir/.prove")";;
 *) ln -s "$cache_dir/.prove" t/.prove;;
 esac
 
-make
+make SKIP_DASHED_BUILT_INS=YesPlease
 case "$jobname" in
 linux-gcc)
 	make test

gitgitgadget

"Johannes Schindelin via GitGitGadget" <[email protected]>

This paragraph is irrelevant. We are keeping the support for it and this topic is not newly deprecating or removing anything. We need to argue for a need to test an installation that lacks these builtin subcommands anywhere on disk under their own names, which you did succinctly below (and there is no need for "For that reason," there).

s/TO/To/

What is not justified enough is why we no longer test installations with dashed builtins on disk. If this topic is primarily about Windows (as 2/3 said), perhaps we can do this only for Windows tasks before we make a colletive decision to _DROP_ support for the on-disk builtin subcommands?

Hi Junio,

Could we please keep it? It will help me in the future when stumbling over this commit, to remember the context.

Thanks! I guess my keys got sticky or something ;-)

Oh, sorry, I will amend the commit message to clarify that the dashed form is actually not tested at all anymore. Specifically since e4597aae6590 (run test suite without dashed git-commands in PATH, 2009-12-02), in fact.

All this change does is to make it an even stronger committment to run the test suite without dashed Git commands.

Thanks,
Dscho

I'm afraid I don't understand this patch or the previous one (or both?). So this new Makefile knob stops hard-linking the dashed builtins _during 'make install'_, but it doesn't affect how Git is built by the default target. And our CI jobs only build Git by the default target, but don't run 'make install', so setting SKIP_DASHED_BUILT_INS wouldn't have any affect anyway.

Very very true. Let's drop 3/3 if it is not testing anything new.

I do understand the concern 2/3 wants to address, and it would be a real one to you especially if you come from Windows. People on the platform wouldn't be able to use shell scripts written in 12 years ago or written with the promise we made to our users 12 years ago, and unlike hardlink-capable platforms it incurs real cost to install these individual binaries on disk.

Hi,

Actually, SKIP_DASHED_BUILT_INS does not _only_ have an impact on make install:

	$ rm git-add.exe && make
	    BUILTIN git-add.exe
	    BUILTIN all
	    SUBDIR git-gui
	    SUBDIR gitk-git
	    SUBDIR templates

	$ rm git-add.exe && make SKIP_DASHED_BUILT_INS=1
	    BUILTIN all
	    SUBDIR git-gui
	    SUBDIR gitk-git
	    SUBDIR templates

See how git-add.exe is linked in the first, but not in the second run?

So the difference 3/3 has is that those hard-linked executables are not even generated. Now, _technically_ this should not result in any change because we run the test suite without --with-dashes.

Practically, it _does_ make a difference, though, as bin-wrappers/git _specifically_ sets GIT_EXEC_PATH to the top-level directory, i.e. git-add.exe _would_ be found if any core Git command that is still implemented as a script called git-add.

Therefore, 3/3 makes sure that we really, really, really do not use those dashed invocations ourselves.

Ciao,
Dscho

OK, that is one more reason why we do want to have 3/3 applied not for all tasks in the CI , but for subset of tasks that includes the Windows task. If we had multiple Windows tasks, it may even be better to have only to some tasks, and allow other tasks build git-add.exe, so that both can be tested for the primary intended platform.

Thanks.

In other words, something like this squashed in.

 ci/run-build-and-tests.sh | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

index 1df9402c3b..cfb841d981 100755
@@ -5,12 +5,16 @@
 
 . ${0%/*}/lib.sh
 
+BUILTINS_HOW=
 case "$CI_OS_NAME" in
-windows*) cmd //c mklink //j t\\.prove "$(cygpath -aw "$cache_dir/.prove")";;
-*) ln -s "$cache_dir/.prove" t/.prove;;
+windows*) 
+	BUILTINS_HOW=SKIP_DASHED_BUILT_INS=YesPlease
+	cmd //c mklink //j t\\.prove "$(cygpath -aw "$cache_dir/.prove")";;
+*)
+	ln -s "$cache_dir/.prove" t/.prove;;
 esac
 
-make SKIP_DASHED_BUILT_INS=YesPlease
+make $BUILTINS_HOW
 case "$jobname" in
 linux-gcc)
 	make test

Hi Junio,

If you want to skip this patch, that's fine with me.

But I would like to clarify what I perceive as a misunderstanding: this patch is not about testing whether it would install the necessary files or not.

What this patch does is simply to complete the mission of e4597aae6590 (run test suite without dashed git-commands in PATH, 2009-12-02): to make sure that our very own scripts do not use dashed invocations of built-in commands.

In that respect, I find it to make more sense to either do it, or not do it (even if I don't quite understand why we wouldn't do it), instead of doing it only for one platform.

Ciao,
Dscho

OK, then I totally misread the proposed log message, or the proposed log message didn't adequately describe what it was trying to solve.

With e4597aae (run test suite without dashed git-commands in PATH, 2009-12-02), we should not need git-foo in bin-wrappers/ for most of git subcommands, as long as our tests and scripted porcelains the test invokes never use the dashed form. And with this step, we come closer to that goal?

"git checkout next && make test" does not seem to populate anything extra in bin-wrappers but bare minimum that needs due to the protocol requirements, though, without this patch, so the above may not be the whole story.

Apparently, the patch does not achieve this goal.

For that reason, we just introduced a Makefile knob to skip linking them. TO make sure that this keeps working, teach the CI (and PR) builds to skip generating those hard-links.

because what matters to tests, when run without with-dash, which is the sensible modern default, is what is in bin-wrappers/ and we do not populate it with builtin git-add... at all even before this step. In other words, this change has no way to make sure "skip linking them" will keep working more than what we already have.

Rather,

Since e4597aae6590, we stopped running our tests with "git-foo" binaries found at the top-level of a freshly built source tree; instead we have placed only "git" and selected "git-foo" commands that must be on $PATH in bin-wrappers/ and they were what used by the tests. This is to catch the tests and scripted Porcelains that are tested will get caught if they try to use the dashed form.

Since CI jobs will not install the built Git to anywhere, and the links we make at the top-level of the source tree for "git-add" and friends are not even used during tests, they are pure waste of resources these days.

Thanks to the newly invented SKIP_DASHED_BUILT_INS knob, we can now skip creating these links in the source tree. Do so.

or something along the line, perhaps.

Thanks.

Ah, ok, so I did indeed misunderstand the previous patch. Thanks.

"Johannes Schindelin via GitGitGadget" <[email protected]>

Thanks---I was wondering what would happen to these files with Peff's "trimmed down" topic. My understanding is that we are still waiting for a reroll of that topic but without the "drop all the .pdb" step from it.

I hadn't planned to re-roll. My topic corrects the inaccuracies in the pdb list (both in the first patch, but also removing entries as they become builtins in the later patches). So it will conflict with Dscho's first patch here, but the resolution is easy (take his side, since it replaces the list entirely).

However, I don't mind re-rolling without touching the pdb list at all if you prefer. It makes the state after my series a little more broken, but it seems that nobody cares that much, and if we're pretty sure Dscho's patch will graduate, then it will be moot anyway.

-Peff

Yeah, that was what I was somehow expecting ;-)

Other than the .pdb thing, I think the rest of the topic was good to go already.

Thanks.

The dashed form of the built-ins is so passé.

Incidentally, this also handles the .pdb issue in MSVC's install Makefile target that Peff pointed out in the context of the "slimming down" patch series
[https://lore.kernel.org/git/[email protected]/]
.

This addresses https://github.com/gitgitgadget/git/issues/406

Changes since v2:

* Reworded and clarified the commit messages of the second and third patch.

Changes since v1:

* Fixed check-docs under SKIP_DASHED_BUILT_INS
* Renamed ALL_PROGRAMS_AND_BUILT_INS to ALL_COMMANDS_TO_INSTALL to reflect its purpose better.
* Revamped the commit message of patch 2/3 and 3/3.

Johannes Schindelin (3):
msvc: copy the correct .pdb files in the Makefile target install Optionally skip linking/copying the built-ins
ci: stop linking built-ins to the dashed versions

 Makefile                  | 71 +++++++++++++++++++++------------------
 ci/run-build-and-tests.sh |  2 +-
 2 files changed, 40 insertions(+), 33 deletions(-)

base-commit: 878e727637ec5815ccb3301eb994a54df95b21b8
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-411%2Fdscho%2Foptionally-skip-dashed-built-ins-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-411/dscho/optionally-skip-dashed-built-ins-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/411

Range-diff vs v2:

 1:  1880a0e4bf = 1:  1880a0e4bf msvc: copy the correct `.pdb` files in the Makefile target `install`
 2:  166bd0d8fb ! 2:  52deafded5 Optionally skip linking/copying the built-ins

@@ Commit message
executable to the dashed name in libexec/git-core/), in practice, it is probably almost irrelevant.

- In fact, some platforms (such as Windows) only started gaining
- meaningful Git support _after_ the dashed form was deprecated, and
- therefore one would expect that all this hard-linking is unnecessary on
- those platforms.
+ However, we *do* care about keeping people's scripts working (even if
+ they were written before the non-dashed form started to be recommended).
+
+ Keeping this backwards-compatibility is not necessarily cheap, though:
+ even so much as amending the tip commit in a git.git checkout will
+ require re-linking all of those dashed commands. On this developer's
+ laptop, this makes a noticeable difference:
+
+ $ touch version.c && time make
+ CC version.o
+ AR libgit.a
+ LINK git-bugreport.exe
+ [... 11 similar lines ...]
+ LN/CP git-remote-https.exe
+ LN/CP git-remote-ftp.exe
+ LN/CP git-remote-ftps.exe
+ LINK git.exe
+ BUILTIN git-add.exe
+ [... 123 similar lines ...]
+ BUILTIN all
+ SUBDIR git-gui
+ SUBDIR gitk-git
+ SUBDIR templates
+ LINK t/helper/test-fake-ssh.exe
+ LINK t/helper/test-line-buffer.exe
+ LINK t/helper/test-svn-fe.exe
+ LINK t/helper/test-tool.exe
+
+ real 0m36.633s
+ user 0m3.794s
+ sys 0m14.141s
+
+ $ touch version.c && time make SKIP_DASHED_BUILT_INS=1
+ CC version.o
+ AR libgit.a
+ LINK git-bugreport.exe
+ [... 11 similar lines ...]
+ LN/CP git-remote-https.exe
+ LN/CP git-remote-ftp.exe
+ LN/CP git-remote-ftps.exe
+ LINK git.exe
+ BUILTIN git-receive-pack.exe
+ BUILTIN git-upload-archive.exe
+ BUILTIN git-upload-pack.exe
+ BUILTIN all
+ SUBDIR git-gui
+ SUBDIR gitk-git
+ SUBDIR templates
+ LINK t/helper/test-fake-ssh.exe
+ LINK t/helper/test-line-buffer.exe
+ LINK t/helper/test-svn-fe.exe
+ LINK t/helper/test-tool.exe
+
+ real 0m23.717s
+ user 0m1.562s
+ sys 0m5.210s
+
+ Also, .zip files do not have any standardized support for hard-links,
+ therefore "zipping up" the executables will result in inflated disk
+ usage. (To keep down the size of the "MinGit" variant of Git for
+ Windows, which is distributed as a .zip file, the hard-links are
+ excluded specifically.)

In addition to that, some programs that are regularly used to assess disk usage fail to realize that those are hard-links, and heavily overcount disk usage. Most notably, this was the case with Windows
- Explorer up until the last couple of Windows 10 versions.
+ Explorer up until the last couple of Windows 10 versions. See e.g.
+ https://github.com/msysgit/msysgit/issues/58.

- To save on the time needed to hard-link these dashed commands, and to
- eventually stop shipping with those hard-links on Windows, let's
+ To save on the time needed to hard-link these dashed commands, with the
+ plan to eventually stop shipping with those hard-links on Windows, let's
introduce a Makefile knob to skip generating them.

3: ea23ba5e26 ! 3: 99a5328492 ci: stop linking built-ins to the dashed versions
@@ Commit message
the dashed form, even if we deprecated that a _long_ time ago.

For that reason, we just introduced a Makefile knob to skip linking
- them. TO make sure that this keeps working, teach the CI
+ them. To make sure that this keeps working, teach the CI
(and PR) builds to skip generating those hard-links.

+ This is actually not such a big change: e4597aae6590 (run test suite
+ without dashed git-commands in PATH, 2009-12-02) made sure that our test
+ suite does not require dashed commands. With this Makefile knob, the
+ commitment is just a little stronger (running tests with --with-dashes
+ would _still_ not see the dashed form of the built-ins).
+
+ There is a subtle change in behavior with this patch, though: as we no
+ longer even _build_ the dashed executables, running the test suite would
+ fail if any of Git's scripted commands (e.g. git-request-pull) still
+ This would have succeeded previously (and would have been unintentional,
+ of course) because bin-wrappers/git sets GIT_EXEC_PATH to the
+ top-level directory (which would still have contained, say,
+ git-rev-parse).
+

## ci/run-build-and-tests.sh ##

gitgitgadget

From: Johannes Schindelin <[email protected]>

There is a hard-coded list of .pdb files to copy. But we are about to introduce the SKIP_DASHED_BUILT_INS knob in the Makefile, which might make this hard-coded list incorrect.

Let's switch to a dynamically-generated list instead.

---
 Makefile | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

index 65f8cfb236..66b6e076e2 100644
@@ -2899,20 +2899,8 @@ ifdef MSVC
 	# have already been rolled up into the exe's pdb file.
 	# We DO NOT have pdb files for the builtin commands (like git-status.exe)
 	# because it is just a copy/hardlink of git.exe, rather than a unique binary.
-	$(INSTALL) git.pdb '$(DESTDIR_SQ)$(bindir_SQ)'
-	$(INSTALL) git-shell.pdb '$(DESTDIR_SQ)$(bindir_SQ)'
-	$(INSTALL) git-upload-pack.pdb '$(DESTDIR_SQ)$(bindir_SQ)'
-	$(INSTALL) git-credential-store.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
-	$(INSTALL) git-daemon.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
-	$(INSTALL) git-fast-import.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
-	$(INSTALL) git-http-backend.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
-	$(INSTALL) git-http-fetch.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
-	$(INSTALL) git-http-push.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
-	$(INSTALL) git-imap-send.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
-	$(INSTALL) git-remote-http.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
-	$(INSTALL) git-remote-testsvn.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
-	$(INSTALL) git-sh-i18n--envsubst.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
-	$(INSTALL) git-show-index.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
+	$(INSTALL) $(patsubst %.exe,%.pdb,$(filter-out $(BUILT_INS),$(patsubst %,%$X,$(BINDIR_PROGRAMS_NEED_X)))) '$(DESTDIR_SQ)$(bindir_SQ)'
+	$(INSTALL) $(patsubst %.exe,%.pdb,$(filter-out $(BUILT_INS) $(REMOTE_CURL_ALIASES),$(PROGRAMS))) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
 ifndef DEBUG
 	$(INSTALL) $(vcpkg_rel_bin)/*.dll '$(DESTDIR_SQ)$(bindir_SQ)'
 	$(INSTALL) $(vcpkg_rel_bin)/*.pdb '$(DESTDIR_SQ)$(bindir_SQ)'

gitgitgadget

From: Johannes Schindelin <[email protected]>

For a long time already, the non-dashed form of the built-ins is the recommended way to write scripts, i.e. it is better to call git merge [...] than to call git-merge [...].

While Git still supports the dashed form (by hard-linking the git executable to the dashed name in libexec/git-core/), in practice, it is probably almost irrelevant.

However, we *do* care about keeping people's scripts working (even if they were written before the non-dashed form started to be recommended).

Keeping this backwards-compatibility is not necessarily cheap, though:
even so much as amending the tip commit in a git.git checkout will require re-linking all of those dashed commands. On this developer's laptop, this makes a noticeable difference:

	$ touch version.c && time make
	    CC version.o
	    AR libgit.a
	    LINK git-bugreport.exe
	    [... 11 similar lines ...]
	    LN/CP git-remote-https.exe
	    LN/CP git-remote-ftp.exe
	    LN/CP git-remote-ftps.exe
	    LINK git.exe
	    BUILTIN git-add.exe
	    [... 123 similar lines ...]
	    BUILTIN all
	    SUBDIR git-gui
	    SUBDIR gitk-git
	    SUBDIR templates
	    LINK t/helper/test-fake-ssh.exe
	    LINK t/helper/test-line-buffer.exe
	    LINK t/helper/test-svn-fe.exe
	    LINK t/helper/test-tool.exe

	real    0m36.633s
	user    0m3.794s
	sys     0m14.141s

	$ touch version.c && time make SKIP_DASHED_BUILT_INS=1
	    CC version.o
	    AR libgit.a
	    LINK git-bugreport.exe
	    [... 11 similar lines ...]
	    LN/CP git-remote-https.exe
	    LN/CP git-remote-ftp.exe
	    LN/CP git-remote-ftps.exe
	    LINK git.exe
	    BUILTIN git-receive-pack.exe
	    BUILTIN git-upload-archive.exe
	    BUILTIN git-upload-pack.exe
	    BUILTIN all
	    SUBDIR git-gui
	    SUBDIR gitk-git
	    SUBDIR templates
	    LINK t/helper/test-fake-ssh.exe
	    LINK t/helper/test-line-buffer.exe
	    LINK t/helper/test-svn-fe.exe
	    LINK t/helper/test-tool.exe

	real    0m23.717s
	user    0m1.562s
	sys     0m5.210s

Also, .zip files do not have any standardized support for hard-links, therefore "zipping up" the executables will result in inflated disk usage. (To keep down the size of the "MinGit" variant of Git for Windows, which is distributed as a .zip file, the hard-links are excluded specifically.)

In addition to that, some programs that are regularly used to assess disk usage fail to realize that those are hard-links, and heavily overcount disk usage. Most notably, this was the case with Windows Explorer up until the last couple of Windows 10 versions. See e.g.
https://github.com/msysgit/msysgit/issues/58.

To save on the time needed to hard-link these dashed commands, with the plan to eventually stop shipping with those hard-links on Windows, let's introduce a Makefile knob to skip generating them.

---
 Makefile | 55 +++++++++++++++++++++++++++++++++++++------------------
 1 file changed, 37 insertions(+), 18 deletions(-)

index 66b6e076e2..0a09146fb3 100644
@@ -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_COMMANDS_TO_INSTALL = $(ALL_PROGRAMS)
+ifeq (,$(SKIP_DASHED_BUILT_INS))
+ALL_COMMANDS_TO_INSTALL += $(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_COMMANDS_TO_INSTALL += git-receive-pack$(X)
+ALL_COMMANDS_TO_INSTALL += git-upload-archive$(X)
+ALL_COMMANDS_TO_INSTALL += 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_COMMANDS_TO_INSTALL) $(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_COMMANDS_TO_INSTALL) 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_COMMANDS_TO_INSTALL) 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_COMMANDS_TO_INSTALL) $(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_COMMANDS_TO_INSTALL) $(SCRIPT_LIB)
 ALL_COMMANDS += git
 ALL_COMMANDS += git-citool
 ALL_COMMANDS += git-gui

@@ -3186,7 +3205,7 @@ check-docs::
 		    -e 's/\.txt//'; \
 	) | while read how cmd; \
 	do \
-		case " $(patsubst %$X,%,$(ALL_COMMANDS) $(EXCLUDED_PROGRAMS)) " in \
+		case " $(patsubst %$X,%,$(ALL_COMMANDS) $(BUILT_INS) $(EXCLUDED_PROGRAMS)) " in \
 		*" $$cmd "*)	;; \
 		*) echo "removed but $$how: $$cmd" ;; \
 		esac; \

gitgitgadget

"Johannes Schindelin via GitGitGadget" <[email protected]>

Let's drop this paragraph. We do not have to defend this new opt-in feature with "probably". Even if there are folks heavily depend on the old promise of having all binaries on disk, giving the rest of the world an option to have Git without that promise is a good thing as long as there is a good reason why the rest of the world would want to omit binaries for builtins. And we do have a good one below.

That misses the point. They were written in dashed form, and when we started recommending non-dashed form, they were TOLD to tweak it by adjusting PATH early in their script, and they did so to keep the script working. So it is not "even if". We do care because we promised them that we will *NOT* break them if they did such and such, and they followed that recommendation.

I do not have anything add to the good argument above (elided) to allow those who know they do not rely on the age old promise. Well written.

From: Johannes Schindelin <[email protected]>

Originally, all of Git's subcommands were implemented in their own executable/script, using the naming scheme git-<command-name>. When more and more functionality was turned into built-in commands (i.e. the git executable could run them without spawning a separate process), for backwards-compatibility, we hard-link the git executable to git-<built-in> for every built-in.

This backwards-compatibility was needed to support scripts that called the dashed form, even if we deprecated that a _long_ time ago.

For that reason, we just introduced a Makefile knob to skip linking them. To make sure that this keeps working, teach the CI (and PR) builds to skip generating those hard-links.

This is actually not such a big change: e4597aae6590 (run test suite without dashed git-commands in PATH, 2009-12-02) made sure that our test suite does not require dashed commands. With this Makefile knob, the commitment is just a little stronger (running tests with --with-dashes would _still_ not see the dashed form of the built-ins).

There is a subtle change in behavior with this patch, though: as we no longer even _build_ the dashed executables, running the test suite would fail if any of Git's scripted commands (e.g. git-request-pull) still This would have succeeded previously (and would have been unintentional, of course) because bin-wrappers/git sets GIT_EXEC_PATH to the top-level directory (which would still have contained, say, git-rev-parse).

---
 ci/run-build-and-tests.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 6c27b886b8..1df9402c3b 100755
@@ -10,7 +10,7 @@ windows*) cmd //c mklink //j t\\.prove "$(cygpath -aw "$cache_dir/.prove")";;
 *) ln -s "$cache_dir/.prove" t/.prove;;
 esac
 
-make
+make SKIP_DASHED_BUILT_INS=YesPlease
 case "$jobname" in
 linux-gcc)
 	make test

gitgitgadget

Hi Gábor,

The intention was to set it in all jobs (but the jury, AKA Junio, is still out on that). Did I not do that?

Ciao,
Dscho

I already said I understood and agreed with your reasoning why this should be done everywhere, and as far as I can see, the "make" invocation we see above is before the job specific case statement starts doing things differently, and applies to everybody.

If I were "still out on" anything, it is that the proposed log message of 3/3 does not explain well why this has a (good) effect on the running of tests, and caused both Szeder and I confusion. The log message needs a bit more polishing.

I think the primary cause of the confusion is that it is not clear to readers that the early three paragraphs refer to the building (hardlinking) of git-foo in the source tree. Because the primary goal of the Makefile change in 2/3 is to stop hardlinking git-foo in the installed location, it is easy for readers to mistakenly think that these paragraphs still talk about the git-foo binaries in the installed directory and miss the fact that we also make them in the source directory without SKIP_DASHED_BUILT_INS.

Thanks.

No; as mentioned earlier, the CI jobs using Docker containers don't use 'ci/run-build-and-tests.sh', but 'ci/run-docker-build.sh' instead.

The dashed invocation of Git commands (git-rev-parse instead of git rev-parse) is deprecated for a long time already. This patch series makes it possible to skip building (and installing) them.

Incidentally, these patches also handle the .pdb issue in MSVC's install Makefile target that Peff pointed out in the context of the "slimming down" patch series
[https://lore.kernel.org/git/[email protected]/]
.

This addresses https://github.com/gitgitgadget/git/issues/406

Changes since v3:

* We now skip linking the built-ins in all CI jobs, including the containerized builds.
* The commit message of the third patch was rewritten for clarity.
* Rebased on top of master to resolve merge conflicts with jk/slimmed-down.

Changes since v2:

* Reworded and clarified the commit messages of the second and the third patch.

Changes since v1:

* Fixed check-docs under SKIP_DASHED_BUILT_INS
* Renamed ALL_PROGRAMS_AND_BUILT_INS to ALL_COMMANDS_TO_INSTALL to reflect its purpose better.
* Revamped the commit message of patch 2/3 and 3/3.

Johannes Schindelin (3):
msvc: copy the correct .pdb files in the Makefile target install Optionally skip linking/copying the built-ins
ci: stop linking built-ins to the dashed versions

 Makefile  | 66 ++++++++++++++++++++++++++++++++-----------------------
 ci/lib.sh |  1 +
 2 files changed, 40 insertions(+), 27 deletions(-)

base-commit: 385c171a018f2747b329bcfa6be8eda1709e5abd
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-411%2Fdscho%2Foptionally-skip-dashed-built-ins-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-411/dscho/optionally-skip-dashed-built-ins-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/411

Range-diff vs v3:

1: 1880a0e4bf ! 1: 5df767c919 msvc: copy the correct .pdb files in the Makefile target install

     @@ Makefile: ifdef MSVC
       	# because it is just a copy/hardlink of git.exe, rather than a unique binary.
      -	$(INSTALL) git.pdb '$(DESTDIR_SQ)$(bindir_SQ)'
      -	$(INSTALL) git-shell.pdb '$(DESTDIR_SQ)$(bindir_SQ)'
     --	$(INSTALL) git-upload-pack.pdb '$(DESTDIR_SQ)$(bindir_SQ)'
     --	$(INSTALL) git-credential-store.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
      -	$(INSTALL) git-daemon.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
     --	$(INSTALL) git-fast-import.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
      -	$(INSTALL) git-http-backend.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
      -	$(INSTALL) git-http-fetch.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
      -	$(INSTALL) git-http-push.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
      -	$(INSTALL) git-imap-send.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
      -	$(INSTALL) git-remote-http.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
     --	$(INSTALL) git-remote-testsvn.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
      -	$(INSTALL) git-sh-i18n--envsubst.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
     --	$(INSTALL) git-show-index.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
      +	$(INSTALL) $(patsubst %.exe,%.pdb,$(filter-out $(BUILT_INS),$(patsubst %,%$X,$(BINDIR_PROGRAMS_NEED_X)))) '$(DESTDIR_SQ)$(bindir_SQ)'
      +	$(INSTALL) $(patsubst %.exe,%.pdb,$(filter-out $(BUILT_INS) $(REMOTE_CURL_ALIASES),$(PROGRAMS))) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
       ifndef DEBUG
 2:  52deafded5 = 2:  14d6eeefbc Optionally skip linking/copying the built-ins
 3:  99a5328492 < -:  ---------- ci: stop linking built-ins to the dashed versions
 -:  ---------- > 3:  1fdf24af36 ci: stop linking built-ins to the dashed versions

gitgitgadget

From: Johannes Schindelin <[email protected]>

There is a hard-coded list of .pdb files to copy. But we are about to introduce the SKIP_DASHED_BUILT_INS knob in the Makefile, which might make this hard-coded list incorrect.

Let's switch to a dynamically-generated list instead.

---
 Makefile | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

index f1b1bc8aa0..ce072c4fb8 100644
@@ -2921,15 +2921,8 @@ ifdef MSVC
 	# have already been rolled up into the exe's pdb file.
 	# We DO NOT have pdb files for the builtin commands (like git-status.exe)
 	# because it is just a copy/hardlink of git.exe, rather than a unique binary.
-	$(INSTALL) git.pdb '$(DESTDIR_SQ)$(bindir_SQ)'
-	$(INSTALL) git-shell.pdb '$(DESTDIR_SQ)$(bindir_SQ)'
-	$(INSTALL) git-daemon.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
-	$(INSTALL) git-http-backend.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
-	$(INSTALL) git-http-fetch.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
-	$(INSTALL) git-http-push.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
-	$(INSTALL) git-imap-send.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
-	$(INSTALL) git-remote-http.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
-	$(INSTALL) git-sh-i18n--envsubst.pdb '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
+	$(INSTALL) $(patsubst %.exe,%.pdb,$(filter-out $(BUILT_INS),$(patsubst %,%$X,$(BINDIR_PROGRAMS_NEED_X)))) '$(DESTDIR_SQ)$(bindir_SQ)'
+	$(INSTALL) $(patsubst %.exe,%.pdb,$(filter-out $(BUILT_INS) $(REMOTE_CURL_ALIASES),$(PROGRAMS))) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
 ifndef DEBUG
 	$(INSTALL) $(vcpkg_rel_bin)/*.dll '$(DESTDIR_SQ)$(bindir_SQ)'
 	$(INSTALL) $(vcpkg_rel_bin)/*.pdb '$(DESTDIR_SQ)$(bindir_SQ)'

gitgitgadget

From: Johannes Schindelin <[email protected]>

For a long time already, the non-dashed form of the built-ins is the recommended way to write scripts, i.e. it is better to call git merge [...] than to call git-merge [...].

While Git still supports the dashed form (by hard-linking the git executable to the dashed name in libexec/git-core/), in practice, it is probably almost irrelevant.

However, we *do* care about keeping people's scripts working (even if they were written before the non-dashed form started to be recommended).

Keeping this backwards-compatibility is not necessarily cheap, though:
even so much as amending the tip commit in a git.git checkout will require re-linking all of those dashed commands. On this developer's laptop, this makes a noticeable difference:

	$ touch version.c && time make
	    CC version.o
	    AR libgit.a
	    LINK git-bugreport.exe
	    [... 11 similar lines ...]
	    LN/CP git-remote-https.exe
	    LN/CP git-remote-ftp.exe
	    LN/CP git-remote-ftps.exe
	    LINK git.exe
	    BUILTIN git-add.exe
	    [... 123 similar lines ...]
	    BUILTIN all
	    SUBDIR git-gui
	    SUBDIR gitk-git
	    SUBDIR templates
	    LINK t/helper/test-fake-ssh.exe
	    LINK t/helper/test-line-buffer.exe
	    LINK t/helper/test-svn-fe.exe
	    LINK t/helper/test-tool.exe

	real    0m36.633s
	user    0m3.794s
	sys     0m14.141s

	$ touch version.c && time make SKIP_DASHED_BUILT_INS=1
	    CC version.o
	    AR libgit.a
	    LINK git-bugreport.exe
	    [... 11 similar lines ...]
	    LN/CP git-remote-https.exe
	    LN/CP git-remote-ftp.exe
	    LN/CP git-remote-ftps.exe
	    LINK git.exe
	    BUILTIN git-receive-pack.exe
	    BUILTIN git-upload-archive.exe
	    BUILTIN git-upload-pack.exe
	    BUILTIN all
	    SUBDIR git-gui
	    SUBDIR gitk-git
	    SUBDIR templates
	    LINK t/helper/test-fake-ssh.exe
	    LINK t/helper/test-line-buffer.exe
	    LINK t/helper/test-svn-fe.exe
	    LINK t/helper/test-tool.exe

	real    0m23.717s
	user    0m1.562s
	sys     0m5.210s

Also, .zip files do not have any standardized support for hard-links, therefore "zipping up" the executables will result in inflated disk usage. (To keep down the size of the "MinGit" variant of Git for Windows, which is distributed as a .zip file, the hard-links are excluded specifically.)

In addition to that, some programs that are regularly used to assess disk usage fail to realize that those are hard-links, and heavily overcount disk usage. Most notably, this was the case with Windows Explorer up until the last couple of Windows 10 versions. See e.g.
https://github.com/msysgit/msysgit/issues/58.

To save on the time needed to hard-link these dashed commands, with the plan to eventually stop shipping with those hard-links on Windows, let's introduce a Makefile knob to skip generating them.

---
 Makefile | 55 +++++++++++++++++++++++++++++++++++++------------------
 1 file changed, 37 insertions(+), 18 deletions(-)

index ce072c4fb8..6931ecd45e 100644
@@ -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.
 #

@@ -774,6 +777,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_COMMANDS_TO_INSTALL = $(ALL_PROGRAMS)
+ifeq (,$(SKIP_DASHED_BUILT_INS))
+ALL_COMMANDS_TO_INSTALL += $(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_COMMANDS_TO_INSTALL += git-receive-pack$(X)
+ALL_COMMANDS_TO_INSTALL += git-upload-archive$(X)
+ALL_COMMANDS_TO_INSTALL += git-upload-pack$(X)
+endif
 
 # what 'all' will build but not install in gitexecdir
 OTHER_PROGRAMS = git$X

@@ -2088,9 +2101,9 @@ profile-fast: profile-clean
 	$(MAKE) PROFILE=USE all
 
 
-all:: $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) $(OTHER_PROGRAMS) GIT-BUILD-OPTIONS
+all:: $(ALL_COMMANDS_TO_INSTALL) $(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_COMMANDS_TO_INSTALL) git$X)), test -d '$p' -o '$p' -ef '$p$X' || $(RM) '$p';)
 endif
 
 all::

@@ -2950,7 +2963,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_COMMANDS_TO_INSTALL) 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) && \

@@ -2968,21 +2981,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 \

@@ -3076,7 +3095,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_COMMANDS_TO_INSTALL) $(SCRIPT_LIB) $(OTHER_PROGRAMS) \
 		GIT-BUILD-OPTIONS $(TEST_PROGRAMS) $(test_bindir_programs) \
 		$(MOFILES)
 	$(QUIET_SUBDIR0)templates $(QUIET_SUBDIR1) \

@@ -3171,7 +3190,7 @@ endif
 
 ### Check documentation
 #
-ALL_COMMANDS = $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS)
+ALL_COMMANDS = $(ALL_COMMANDS_TO_INSTALL) $(SCRIPT_LIB)
 ALL_COMMANDS += git
 ALL_COMMANDS += git-citool
 ALL_COMMANDS += git-gui

@@ -3211,7 +3230,7 @@ check-docs::
 		    -e 's/\.txt//'; \
 	) | while read how cmd; \
 	do \
-		case " $(patsubst %$X,%,$(ALL_COMMANDS) $(EXCLUDED_PROGRAMS)) " in \
+		case " $(patsubst %$X,%,$(ALL_COMMANDS) $(BUILT_INS) $(EXCLUDED_PROGRAMS)) " in \
 		*" $$cmd "*)	;; \
 		*) echo "removed but $$how: $$cmd" ;; \
 		esac; \

gitgitgadget

From: Johannes Schindelin <[email protected]>

Since e4597aae6590 (run test suite without dashed git-commands in PATH, 2009-12-02), we stopped running our tests with git-foo binaries found at the top-level directory of a freshly built source tree; instead we have placed only git and selected git-foo commands that must be on $PATH in bin-wrappers/ and prepended that bin-wrappers/ to the PATH used in the test suite. We did that to catch the tests and scripted Git commands that still try to use the dashed form.

Since CI jobs will not install the built Git to anywhere, and the hardlinks we make at the top-level of the source tree for git-add and friends are not even used during tests, they are pure waste of resources these days.

Thanks to the newly invented SKIP_DASHED_BUILT_INS knob, we can now skip creating these links in the source tree. So let's do that.

Note that this change introduces a subtle change of behavior: when Git's cmd_main() calls setup_path(), it inserts the value of GIT_EXEC_PATH (defaulting to <prefix>/libexec/git-core) at the beginning of the environment variable PATH. This is necessary to find e.g. scripted commands that are installed in that location. For the purposes of Git's test suite, the bin-wrappers/ scripts override GIT_EXEC_PATH to point to the top-level directory of the source code.

In other words, if a scripted command had used a dashed invocation of a built-in Git command, it would not have been caught previously, which is fixed by this change.

---
 ci/lib.sh | 1 +
 1 file changed, 1 insertion(+)

index 3eefec500d..821e3660d6 100755
@@ -178,6 +178,7 @@ fi
 export DEVELOPER=1
 export DEFAULT_TEST_TARGET=prove
 export GIT_TEST_CLONE_2GB=true
+export SKIP_DASHED_BUILT_INS=YesPlease
 
 case "$jobname" in
 linux-clang|linux-gcc)

gitgitgadget

"Johannes Schindelin via GitGitGadget" <[email protected]>

Makes perfect sense, and I do not think readers will confused like they were by the previous round's corresponding step.