Skip to content

Optionally skip linking/copying the built-ins #411

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed

Conversation

dscho
Copy link
Member

@dscho dscho commented Oct 20, 2019

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.

This addresses #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.

cc: SZEDER Gábor [email protected]
cc: Johannes Schindelin [email protected]

@dscho dscho added the needs-work These patches have pending issues that need to be resolved before submitting label Oct 20, 2019
@dscho dscho force-pushed the optionally-skip-dashed-built-ins branch from 4ce262d to 22764b2 Compare October 20, 2019 21:17
@dscho
Copy link
Member Author

dscho commented Jan 29, 2020

I also need to replace the hard-coded .exe by $(X), and probably we'll want to change the CI job linux-gcc so that it uses this build knob, just to make sure that it works as advertised (and keeps working that way).

@dscho dscho force-pushed the optionally-skip-dashed-built-ins branch from 0b8b449 to 1269d7a Compare July 16, 2020 13:53
@dscho dscho changed the title [Work-In-Progress] Optionally skip linking/copying the built-ins Optionally skip linking/copying the built-ins Jul 17, 2020
@dscho
Copy link
Member Author

dscho commented Aug 17, 2020

/preview

@gitgitgadget
Copy link

gitgitgadget bot commented Aug 17, 2020

Preview email sent as [email protected]

@dscho
Copy link
Member Author

dscho commented Aug 17, 2020

/submit

@gitgitgadget
Copy link

gitgitgadget bot commented Aug 17, 2020

Submitted as [email protected]

@gitgitgadget
Copy link

gitgitgadget bot commented Aug 17, 2020

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

  This message is in MIME format.  The first part should be readable text,
  while the remaining parts are likely unreadable without MIME-aware tools.

--8323328-1494576974-1597640142=:56
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

Hi,


On Mon, 17 Aug 2020, Johannes Schindelin wrote:

> The dashed form of the built-ins is so pass=C3=A9.
>
> Incidentally, this also handles the .pdb issue in MSVC's install Makefil=
e
> target that Peff pointed out in the context of the "slimming down" patch
> series
> [https://lore.kernel.org/git/[email protected]=
.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%2F=
dscho%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
>
>

--8323328-1494576974-1597640142=:56--

@@ -2899,20 +2899,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

@dscho dscho force-pushed the optionally-skip-dashed-built-ins branch from 1269d7a to 4a9d9e1 Compare August 17, 2020 09:40
@gitgitgadget
Copy link

gitgitgadget bot commented Aug 17, 2020

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

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

@gitgitgadget
Copy link

gitgitgadget bot commented Aug 24, 2020

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

Hi Junio,

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

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

@dscho dscho force-pushed the optionally-skip-dashed-built-ins branch from 4a9d9e1 to ea23ba5 Compare August 24, 2020 15:02
@dscho
Copy link
Member Author

dscho commented Aug 24, 2020

/submit

@gitgitgadget
Copy link

gitgitgadget bot commented Aug 24, 2020

Submitted as [email protected]

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-411/dscho/optionally-skip-dashed-built-ins-v2

To fetch this version to local tag pr-411/dscho/optionally-skip-dashed-built-ins-v2:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-411/dscho/optionally-skip-dashed-built-ins-v2

@gitgitgadget
Copy link

gitgitgadget bot commented Aug 24, 2020

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

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

@gitgitgadget
Copy link

gitgitgadget bot commented Aug 24, 2020

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

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

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

@gitgitgadget
Copy link

gitgitgadget bot commented Aug 24, 2020

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

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

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

@@ -10,7 +10,7 @@ windows*) cmd //c mklink //j t\\.prove "$(cygpath -aw "$cache_dir/.prove")";;
*) ln -s "$cache_dir/.prove" t/.prove;;
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]>
>
> 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 <[email protected]>
> ---
>  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

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:
>
> > From: Johannes Schindelin <[email protected]>
> >
> > Originally, all of Git's subcommands were implemented in their own
> > executable/script, using the naming scheme `git-<command-name>`. When
> > more and more functionality was turned into built-in commands (i.e. the
> > `git` executable could run them without spawning a separate process),
> > for backwards-compatibility, we hard-link the `git` executable to
> > `git-<built-in>` for every built-in.
> >
> > This backwards-compatibility was needed to support scripts that called
> > the dashed form, even if we deprecated that a _long_ time ago.
>
> 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 <[email protected]>
> > ---
> >  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
Copy link

gitgitgadget bot commented Aug 24, 2020

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

Jeff King <[email protected]> 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.

@gitgitgadget
Copy link

gitgitgadget bot commented Aug 24, 2020

This branch is now known as js/no-builtins-on-disk-option.

@gitgitgadget
Copy link

gitgitgadget bot commented Aug 24, 2020

This patch series was integrated into seen via git@f3e8b82.

@gitgitgadget gitgitgadget bot added the seen label Aug 24, 2020
@gitgitgadget
Copy link

gitgitgadget bot commented Aug 25, 2020

This patch series was integrated into seen via git@cfb875e.

@gitgitgadget
Copy link

gitgitgadget bot commented Sep 17, 2020

This patch series was integrated into seen via git@99cde25.

@gitgitgadget
Copy link

gitgitgadget bot commented Sep 18, 2020

This patch series was integrated into seen via git@46d8ad7.

@gitgitgadget
Copy link

gitgitgadget bot commented Sep 18, 2020

This patch series was integrated into seen via git@c583a73.

@gitgitgadget
Copy link

gitgitgadget bot commented Sep 19, 2020

This patch series was integrated into seen via git@81619bc.

@gitgitgadget
Copy link

gitgitgadget bot commented Sep 19, 2020

This patch series was integrated into seen via git@4a3b483.

@gitgitgadget
Copy link

gitgitgadget bot commented Sep 21, 2020

This patch series was integrated into seen via git@c65f9c6.

@dscho dscho force-pushed the optionally-skip-dashed-built-ins branch from 99a5328 to 84a5a50 Compare September 21, 2020 21:44
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 <[email protected]>
For a long time already, the non-dashed form of the built-ins is the
recommended way to write scripts, i.e. it is better to call `git merge
[...]` than to call `git-merge [...]`.

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

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

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

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

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

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

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

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

In addition to that, some programs that are regularly used to assess
disk usage fail to realize that those are hard-links, and heavily
overcount disk usage. Most notably, this was the case with Windows
Explorer up until the last couple of Windows 10 versions. See e.g.
msysgit/msysgit#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 <[email protected]>
Since e4597aa (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 <[email protected]>
@dscho dscho force-pushed the optionally-skip-dashed-built-ins branch from 84a5a50 to 1fdf24a Compare September 21, 2020 21:56
@dscho
Copy link
Member Author

dscho commented Sep 21, 2020

/submit

@gitgitgadget
Copy link

gitgitgadget bot commented Sep 21, 2020

Submitted as [email protected]

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-411/dscho/optionally-skip-dashed-built-ins-v4

To fetch this version to local tag pr-411/dscho/optionally-skip-dashed-built-ins-v4:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-411/dscho/optionally-skip-dashed-built-ins-v4

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

@gitgitgadget
Copy link

gitgitgadget bot commented Sep 21, 2020

This patch series was integrated into seen via git@bd10869.

@gitgitgadget
Copy link

gitgitgadget bot commented Sep 22, 2020

This patch series was integrated into seen via git@419f6c2.

@gitgitgadget
Copy link

gitgitgadget bot commented Sep 22, 2020

This patch series was integrated into seen via git@55c92eb.

@gitgitgadget
Copy link

gitgitgadget bot commented Sep 22, 2020

This patch series was integrated into seen via git@aebf6b2.

@gitgitgadget
Copy link

gitgitgadget bot commented Sep 25, 2020

This patch series was integrated into seen via git@127e5a2.

@gitgitgadget
Copy link

gitgitgadget bot commented Sep 25, 2020

This patch series was integrated into next via git@6b976da.

@gitgitgadget gitgitgadget bot added the next label Sep 25, 2020
@gitgitgadget
Copy link

gitgitgadget bot commented Sep 29, 2020

This patch series was integrated into seen via git@6d50998.

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 1, 2020

This patch series was integrated into seen via git@ee10dc5.

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 4, 2020

This patch series was integrated into seen via git@94de88c.

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 4, 2020

This patch series was integrated into next via git@94de88c.

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 4, 2020

This patch series was integrated into master via git@94de88c.

@gitgitgadget gitgitgadget bot added the master label Oct 4, 2020
@gitgitgadget gitgitgadget bot closed this Oct 4, 2020
@gitgitgadget
Copy link

gitgitgadget bot commented Oct 4, 2020

Closed via 94de88c.

@dscho dscho deleted the optionally-skip-dashed-built-ins branch October 5, 2020 07:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a build option to skip linking all those built-ins to their dashed form
1 participant