Re: [PATCH 0/3] Optionally skip linking/copying the built-ins

Re: [PATCH 0/3] Optionally skip linking/copying the built-ins

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1691 bytes --]

Hi,


On Mon, 17 Aug 2020, Johannes Schindelin wrote:

> 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/20200813145719.GA891370@coredump.intra.peff.net/]
> .
>
> This addresses https://github.com/gitgitgadget/git/issues/406

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

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

Re: [PATCH 1/3] msvc: copy the correct `.pdb` files in the Makefile target `install`

[Johannes Schindelin]

Hi Peff,

On Mon, 17 Aug 2020, Jeff King wrote:

> On Mon, Aug 17, 2020 at 09:07:51AM +0000, Johannes Schindelin wrote:
>
> > -	$(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)'
>
> Oh, this is much better than what my patch does. :)
>
> The rest of the series looks like a good direction to me, but is outside
> the scope of my series. I'd be happy to pick this first patch up for a
> re-roll of mine (which would require tweaking the rest of the patches on
> top to stop removing things from the .pdb list). Or we could just leave
> this as a separate topic and deal with the merge conflict (which would
> obviously resolve in favor of yours).

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

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

[Johannes Schindelin]

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/20200813145719.GA891370@coredump.intra.peff.net/]
.

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

[PATCH 1/3] msvc: copy the correct `.pdb` files in the Makefile target `install`

[Johannes Schindelin]

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Makefile | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/Makefile b/Makefile
index 372139f1f2..3f5ba97b70 100644
--- a/Makefile
+++ b/Makefile
@@ -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

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

[Johannes Schindelin]

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 <johannes.schindelin@gmx.de>
---
 Makefile | 53 ++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 36 insertions(+), 17 deletions(-)

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
-- 
gitgitgadget

[PATCH 3/3] ci: stop linking built-ins to the dashed versions

[Johannes Schindelin]

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 ci/run-build-and-tests.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
index 17e25aade9..b074db5c4b 100755
--- a/ci/run-build-and-tests.sh
+++ b/ci/run-build-and-tests.sh
@@ -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

Re: [PATCH 1/3] msvc: copy the correct `.pdb` files in the Makefile target `install`

[Jeff King]

On Mon, Aug 17, 2020 at 09:07:51AM +0000, Johannes Schindelin wrote:

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

Oh, this is much better than what my patch does. :)

The rest of the series looks like a good direction to me, but is outside
the scope of my series. I'd be happy to pick this first patch up for a
re-roll of mine (which would require tweaking the rest of the patches on
top to stop removing things from the .pdb list). Or we could just leave
this as a separate topic and deal with the merge conflict (which would
obviously resolve in favor of yours).

-Peff

Re: [PATCH 0/3] Optionally skip linking/copying the built-ins

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> This addresses https://github.com/gitgitgadget/git/issues/406
>
> 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.

It is close ;-) 

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

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

[Junio C Hamano]

Johannes Schindelin <gitgitgadget@gmail.com> 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 <johannes.schindelin@gmx.de>
> ---
>  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.

Re: [PATCH 3/3] ci: stop linking built-ins to the dashed versions

[Junio C Hamano]

Johannes Schindelin <gitgitgadget@gmail.com> writes:

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

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

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

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.

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  ci/run-build-and-tests.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
> index 17e25aade9..b074db5c4b 100755
> --- a/ci/run-build-and-tests.sh
> +++ b/ci/run-build-and-tests.sh
> @@ -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

Re: [PATCH 1/3] msvc: copy the correct `.pdb` files in the Makefile target `install`

[Jeff King]

On Mon, Aug 17, 2020 at 07:51:17AM +0200, Johannes Schindelin wrote:

> > The rest of the series looks like a good direction to me, but is outside
> > the scope of my series. I'd be happy to pick this first patch up for a
> > re-roll of mine (which would require tweaking the rest of the patches on
> > top to stop removing things from the .pdb list). Or we could just leave
> > this as a separate topic and deal with the merge conflict (which would
> > obviously resolve in favor of yours).
> 
> 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.

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

Re: [PATCH 1/3] msvc: copy the correct `.pdb` files in the Makefile target `install`

[Johannes Schindelin]

Hi Peff,

On Mon, 17 Aug 2020, Jeff King wrote:

> On Mon, Aug 17, 2020 at 07:51:17AM +0200, Johannes Schindelin wrote:
>
> > > The rest of the series looks like a good direction to me, but is outside
> > > the scope of my series. I'd be happy to pick this first patch up for a
> > > re-roll of mine (which would require tweaking the rest of the patches on
> > > top to stop removing things from the .pdb list). Or we could just leave
> > > this as a separate topic and deal with the merge conflict (which would
> > > obviously resolve in favor of yours).
> >
> > 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.
>
> 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).

Sounds good to me!

Ciao,
Dscho

Re: [PATCH 0/3] Optionally skip linking/copying the built-ins

[Johannes Schindelin]

Hi Junio,

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

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> >> This addresses https://github.com/gitgitgadget/git/issues/406
> >
> > 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.
>
> It is close ;-)
>
> The author name is correctly on "From:" but not the address.

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

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 2318 bytes --]

Hi Junio,

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

> Johannes Schindelin <gitgitgadget@gmail.com> 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 <johannes.schindelin@gmx.de>
> > ---
> >  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 v2 0/3] Optionally skip linking/copying the built-ins

[Johannes Schindelin via GitGitGadget]

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/20200813145719.GA891370@coredump.intra.peff.net/]
.

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 <johannes.schindelin@gmx.de>
      
     @@ 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.
      
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

-- 
gitgitgadget

[PATCH v2 1/3] msvc: copy the correct `.pdb` files in the Makefile target `install`

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Makefile | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/Makefile b/Makefile
index 65f8cfb236..66b6e076e2 100644
--- a/Makefile
+++ b/Makefile
@@ -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

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

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 Makefile | 55 +++++++++++++++++++++++++++++++++++++------------------
 1 file changed, 37 insertions(+), 18 deletions(-)

diff --git a/Makefile b/Makefile
index 66b6e076e2..0a09146fb3 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_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

[PATCH v2 3/3] ci: stop linking built-ins to the dashed versions

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 ci/run-build-and-tests.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
index 6c27b886b8..1df9402c3b 100755
--- a/ci/run-build-and-tests.sh
+++ b/ci/run-build-and-tests.sh
@@ -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

Re: [PATCH 0/3] Optionally skip linking/copying the built-ins

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> It is close ;-)
>>
>> The author name is correctly on "From:" but not the address.
>
> Yes, but the problem seems to be insurmountable, as I _think_ it is to
> prevent spammers from successfully sending "from abitrary email
> addresses".

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.

Re: [PATCH v2 0/3] Optionally skip linking/copying the built-ins

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> Johannes Schindelin (3):
>   msvc: copy the correct `.pdb` files in the Makefile target `install`

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.

Re: [PATCH v2 2/3] Optionally skip linking/copying the built-ins

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

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

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 ;-).

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

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.

Re: [PATCH v2 0/3] Optionally skip linking/copying the built-ins

[Jeff King]

On Mon, Aug 24, 2020 at 11:55:22AM -0700, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
> 
> > Johannes Schindelin (3):
> >   msvc: copy the correct `.pdb` files in the Makefile target `install`
> 
> 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

Re: [PATCH v2 3/3] ci: stop linking built-ins to the dashed versions

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> 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.

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

> For that reason, we just introduced a Makefile knob to skip linking
> them. TO make sure that this keeps working, teach the CI

s/TO/To/

> (and PR) builds to skip generating those hard-links.

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?

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  ci/run-build-and-tests.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
> index 6c27b886b8..1df9402c3b 100755
> --- a/ci/run-build-and-tests.sh
> +++ b/ci/run-build-and-tests.sh
> @@ -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

Re: [PATCH v2 0/3] Optionally skip linking/copying the built-ins

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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.

Re: [PATCH 0/3] Optionally skip linking/copying the built-ins

[Johannes Schindelin]

Hi Junio,

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

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> >> It is close ;-)
> >>
> >> The author name is correctly on "From:" but not the address.
> >
> > Yes, but the problem seems to be insurmountable, as I _think_ it is to
> > prevent spammers from successfully sending "from abitrary email
> > addresses".
>
> 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?

But if all you're interested in is the part before the actual email
address, isn't "Johannes Schindelin via GitGitGadget
<gitgitgadget@gmail.com>" almost identical to "Johannes Schindelin
<gitgitgadget@gmail.com>"?

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

Ciao,
Dscho

Re: [PATCH v2 2/3] Optionally skip linking/copying the built-ins

[Johannes Schindelin]

Hi Junio,

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

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > 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.
>
> 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.

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.

> 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 ;-).

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

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

Excellent!

Ciao,
Dscho

Re: [PATCH v2 3/3] ci: stop linking built-ins to the dashed versions

[Johannes Schindelin]

Hi Junio,

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

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > 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.
>
> 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).

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

> > For that reason, we just introduced a Makefile knob to skip linking
> > them. TO make sure that this keeps working, teach the CI
>
> s/TO/To/

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

> > (and PR) builds to skip generating those hard-links.
>
> 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?

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

> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  ci/run-build-and-tests.sh | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
> > index 6c27b886b8..1df9402c3b 100755
> > --- a/ci/run-build-and-tests.sh
> > +++ b/ci/run-build-and-tests.sh
> > @@ -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
>

Re: [PATCH v2 3/3] ci: stop linking built-ins to the dashed versions

[SZEDER Gábor]

On Mon, Aug 24, 2020 at 03:38:00PM +0000, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> 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.

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.

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  ci/run-build-and-tests.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
> index 6c27b886b8..1df9402c3b 100755
> --- a/ci/run-build-and-tests.sh
> +++ b/ci/run-build-and-tests.sh
> @@ -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

Note that the CI jobs executed in containers (Linux32 and linux-musl)
don't use this 'ci/run-build-and-tests.sh' script, so they won't set
SKIP_DASHED_BUILT_INS.  I suppose that's unintentional, because it
wasn't mentioned in the commit message.

>  case "$jobname" in
>  linux-gcc)
>  	make test
> -- 
> gitgitgadget

Re: [PATCH v2 3/3] ci: stop linking built-ins to the dashed versions

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> writes:

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

Re: [PATCH 0/3] Optionally skip linking/copying the built-ins

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> But if all you're interested in is the part before the actual email
> address, isn't "Johannes Schindelin via GitGitGadget
> <gitgitgadget@gmail.com>" almost identical to "Johannes Schindelin
> <gitgitgadget@gmail.com>"?

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.

Re: [PATCH v2 3/3] ci: stop linking built-ins to the dashed versions

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 2005 bytes --]

Hi,

On Tue, 25 Aug 2020, Junio C Hamano wrote:

> SZEDER Gábor <szeder.dev@gmail.com> writes:
>
> > 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.

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

[PATCH v3 0/3] Optionally skip linking/copying the built-ins

[Johannes Schindelin via GitGitGadget]

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/20200813145719.GA891370@coredump.intra.peff.net/]
.

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.
      
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
 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`).
     +
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
       ## ci/run-build-and-tests.sh ##

-- 
gitgitgadget

[PATCH v3 1/3] msvc: copy the correct `.pdb` files in the Makefile target `install`

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Makefile | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/Makefile b/Makefile
index 65f8cfb236..66b6e076e2 100644
--- a/Makefile
+++ b/Makefile
@@ -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

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

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Makefile | 55 +++++++++++++++++++++++++++++++++++++------------------
 1 file changed, 37 insertions(+), 18 deletions(-)

diff --git a/Makefile b/Makefile
index 66b6e076e2..0a09146fb3 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_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

[PATCH v3 3/3] ci: stop linking built-ins to the dashed versions

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 ci/run-build-and-tests.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
index 6c27b886b8..1df9402c3b 100755
--- a/ci/run-build-and-tests.sh
+++ b/ci/run-build-and-tests.sh
@@ -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

Re: [PATCH v2 3/3] ci: stop linking built-ins to the dashed versions

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Actually, `SKIP_DASHED_BUILT_INS` does not _only_ have an impact on `make
> install`:
> ...
> See how `git-add.exe` is linked in the first, but not in the second run?

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.

Re: [PATCH v3 2/3] Optionally skip linking/copying the built-ins

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> 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.

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.

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

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.

> Keeping this backwards-compatibility is not necessarily cheap, though:
> ...
> introduce a Makefile knob to skip generating them.

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.

Re: [PATCH v2 3/3] ci: stop linking built-ins to the dashed versions

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>> Actually, `SKIP_DASHED_BUILT_INS` does not _only_ have an impact on `make
>> install`:
>> ...
>> See how `git-add.exe` is linked in the first, but not in the second run?
>
> 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.

In other words, something like this squashed in.

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

diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
index 1df9402c3b..cfb841d981 100755
--- a/ci/run-build-and-tests.sh
+++ b/ci/run-build-and-tests.sh
@@ -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

Re: [PATCH v2 3/3] ci: stop linking built-ins to the dashed versions

[SZEDER Gábor]

On Wed, Aug 26, 2020 at 06:19:52AM +0200, Johannes Schindelin wrote:
> Hi,
> 
> On Tue, 25 Aug 2020, Junio C Hamano wrote:
> 
> > SZEDER Gábor <szeder.dev@gmail.com> writes:
> >
> > > 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.
> 
> 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?

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

Re: [PATCH v2 3/3] ci: stop linking built-ins to the dashed versions

[Johannes Schindelin]

Hi Junio,

On Wed, 26 Aug 2020, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > Actually, `SKIP_DASHED_BUILT_INS` does not _only_ have an impact on `make
> > install`:
> > ...
> > See how `git-add.exe` is linked in the first, but not in the second run?
>
> 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.

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

Re: [PATCH v2 3/3] ci: stop linking built-ins to the dashed versions

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

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

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.

Re: [PATCH v3 3/3] ci: stop linking built-ins to the dashed versions

[SZEDER Gábor]

On Wed, Aug 26, 2020 at 11:56:52AM +0000, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> 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`).
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  ci/run-build-and-tests.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
> index 6c27b886b8..1df9402c3b 100755
> --- a/ci/run-build-and-tests.sh
> +++ b/ci/run-build-and-tests.sh
> @@ -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

Please make sure that this Makefile knob is set in all jobs building
and testing Git, or justify in the commit message why it isn't.

>  case "$jobname" in
>  linux-gcc)
>  	make test
> -- 
> gitgitgadget

Re: [PATCH v3 3/3] ci: stop linking built-ins to the dashed versions

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 2760 bytes --]

Hi Gábor,

On Thu, 3 Sep 2020, SZEDER Gábor wrote:

> On Wed, Aug 26, 2020 at 11:56:52AM +0000, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > 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`).
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  ci/run-build-and-tests.sh | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
> > index 6c27b886b8..1df9402c3b 100755
> > --- a/ci/run-build-and-tests.sh
> > +++ b/ci/run-build-and-tests.sh
> > @@ -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
>
> Please make sure that this Makefile knob is set in all jobs building
> and testing Git, or justify in the commit message why it isn't.

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

>
> >  case "$jobname" in
> >  linux-gcc)
> >  	make test
> > --
> > gitgitgadget
>

Re: [PATCH v3 3/3] ci: stop linking built-ins to the dashed versions

[SZEDER Gábor]

On Tue, Sep 08, 2020 at 01:32:56PM +0200, Johannes Schindelin wrote:
> Hi Gábor,
> 
> On Thu, 3 Sep 2020, SZEDER Gábor wrote:
> 
> > On Wed, Aug 26, 2020 at 11:56:52AM +0000, Johannes Schindelin via GitGitGadget wrote:
> > > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> > >
> > > 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`).
> > >
> > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > > ---
> > >  ci/run-build-and-tests.sh | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
> > > index 6c27b886b8..1df9402c3b 100755
> > > --- a/ci/run-build-and-tests.sh
> > > +++ b/ci/run-build-and-tests.sh
> > > @@ -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
> >
> > Please make sure that this Makefile knob is set in all jobs building
> > and testing Git, or justify in the commit message why it isn't.
> 
> The intention was to set it in all jobs (but the jury, AKA Junio, is still
> out on that). Did I not do that?

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.

Re: [PATCH v3 3/3] ci: stop linking built-ins to the dashed versions

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> > diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
>> > index 6c27b886b8..1df9402c3b 100755
>> > --- a/ci/run-build-and-tests.sh
>> > +++ b/ci/run-build-and-tests.sh
>> > @@ -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
>>
>> Please make sure that this Makefile knob is set in all jobs building
>> and testing Git, or justify in the commit message why it isn't.
>
> The intention was to set it in all jobs (but the jury, AKA Junio, is still
> out on that). Did I not do that?

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.

[PATCH v4 0/3] Optionally skip linking/copying the built-ins

[Johannes Schindelin via GitGitGadget]

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/20200813145719.GA891370@coredump.intra.peff.net/]
.

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

[PATCH v4 1/3] msvc: copy the correct `.pdb` files in the Makefile target `install`

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Makefile | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/Makefile b/Makefile
index f1b1bc8aa0..ce072c4fb8 100644
--- a/Makefile
+++ b/Makefile
@@ -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

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

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Makefile | 55 +++++++++++++++++++++++++++++++++++++------------------
 1 file changed, 37 insertions(+), 18 deletions(-)

diff --git a/Makefile b/Makefile
index ce072c4fb8..6931ecd45e 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.
 #
@@ -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

[PATCH v4 3/3] ci: stop linking built-ins to the dashed versions

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 ci/lib.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ci/lib.sh b/ci/lib.sh
index 3eefec500d..821e3660d6 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -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

Re: [PATCH v4 3/3] ci: stop linking built-ins to the dashed versions

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> 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.

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

> diff --git a/ci/lib.sh b/ci/lib.sh
> index 3eefec500d..821e3660d6 100755
> --- a/ci/lib.sh
> +++ b/ci/lib.sh
> @@ -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

OK.  This would hopefully cover all the CI targets; we know it
covers everybody who uses DEVELOPER=1, which is a good sign ;-)

Thanks.