Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 39 additions & 27 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -348,6 +348,9 @@ all::
# Define NO_INSTALL_HARDLINKS if you prefer to use either symbolic links or
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Johannes Schindelin via GitGitGadget" <[email protected]>
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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Johannes Schindelin wrote (reply to this):

Hi Junio,

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

> "Johannes Schindelin via GitGitGadget" <[email protected]>
> 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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

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

> From: Johannes Schindelin <[email protected]>
>
> For a long time already, the non-dashed form of the built-ins is the
> recommended way to write scripts, i.e. it is better to call `git merge
> [...]` than to call `git-merge [...]`.
>
> While Git still supports the dashed form (by hard-linking the `git`
> executable to the dashed name in `libexec/git-core/`), in practice, it
> is probably almost irrelevant.

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.

# 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.
#
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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::
Expand Down Expand Up @@ -2921,15 +2934,8 @@ ifdef MSVC
# have already been rolled up into the exe's pdb file.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Jeff King wrote (reply to this):

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Johannes Schindelin wrote (reply to this):

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Jeff King wrote (reply to this):

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Johannes Schindelin wrote (reply to this):

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

# 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)'
Expand Down Expand Up @@ -2957,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) && \
Expand All @@ -2975,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 \
Expand Down Expand Up @@ -3083,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) \
Expand Down Expand Up @@ -3178,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
Expand Down Expand Up @@ -3218,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; \
Expand Down
1 change: 1 addition & 0 deletions ci/lib.sh
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,7 @@ fi
export DEVELOPER=1
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

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

> From: Johannes Schindelin <[email protected]>
>
> Since e4597aae6590 (run test suite without dashed git-commands in PATH,
> 2009-12-02), we stopped running our tests with `git-foo` binaries found
> at the top-level directory of a freshly built source tree; instead we
> have placed only `git` and selected `git-foo` commands that must be on
> `$PATH` in `bin-wrappers/` and prepended that `bin-wrappers/` to the
> `PATH` used in the test suite. We did that to catch the tests and
> scripted Git commands that still try to use the dashed form.
>
> Since CI jobs will not install the built Git to anywhere, and the
> hardlinks we make at the top-level of the source tree for `git-add` and
> friends are not even used during tests, they are pure waste of resources
> these days.

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.

export DEFAULT_TEST_TARGET=prove
export GIT_TEST_CLONE_2GB=true
export SKIP_DASHED_BUILT_INS=YesPlease

case "$jobname" in
linux-clang|linux-gcc)
Expand Down