mbox series

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

Message ID pull.411.v2.git.1598283480.gitgitgadget@gmail.com (mailing list archive)
Headers show
Series Optionally skip linking/copying the built-ins | expand

Message

Philippe Blain via GitGitGadget Aug. 24, 2020, 3:37 p.m. UTC 
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>

Comments

Junio C Hamano Aug. 24, 2020, 6:55 p.m. UTC | #1 
"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.
Jeff King Aug. 24, 2020, 7:03 p.m. UTC | #2 
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
Junio C Hamano Aug. 24, 2020, 7:51 p.m. UTC | #3 
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.