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

Commit Message

Derrick Stolee via GitGitGadget Aug. 26, 2020, 11:56 a.m. UTC

From: Johannes Schindelin <[e-mail]>

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 <[e-mail]>
---
 ci/run-build-and-tests.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

SZEDER Gábor Sept. 3, 2020, 10:45 a.m. UTC | #1

On Wed, Aug 26, 2020 at 11:56:52AM +0000, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <[e-mail]>
> 
> 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 <[e-mail]>
> ---
>  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

Johannes Schindelin Sept. 8, 2020, 11:32 a.m. UTC | #2

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 <[e-mail]>
> >
> > 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 <[e-mail]>
> > ---
> >  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
>

SZEDER Gábor Sept. 8, 2020, 11:48 a.m. UTC | #3

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 <[e-mail]>
> > >
> > > 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 <[e-mail]>
> > > ---
> > >  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.

Junio C Hamano Sept. 8, 2020, 5:18 p.m. UTC | #4

Johannes Schindelin <[e-mail]> 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

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