diff mbox series

[2/5] tests: respect GIT_TEST_INSTALLED when initializing repositories

Message ID 948b3dc146fe353fbab6057c1376fa0e787a444f.1542030510.git.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series tests: various improvements to the GIT_TEST_INSTALLED feature | expand

Commit Message

Johannes Schindelin via GitGitGadget Nov. 12, 2018, 1:48 p.m. UTC 
From: Johannes Schindelin <johannes.schindelin@gmx.de>

It really makes very, very little sense to use a different git
executable than the one the caller indicated via setting the environment
variable GIT_TEST_INSTALLED.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/test-lib-functions.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Junio C Hamano Nov. 14, 2018, 4:55 a.m. UTC | #1 
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> It really makes very, very little sense to use a different git
> executable than the one the caller indicated via setting the environment
> variable GIT_TEST_INSTALLED.

Makes perfect sense.  Shouldn't we be asking where the template
directory of the installed version is and using it instead of the
freshly built one, no?

>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  t/test-lib-functions.sh | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 78d8c3783b..801cc9b2ef 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -900,7 +900,8 @@ test_create_repo () {
>  	mkdir -p "$repo"
>  	(
>  		cd "$repo" || error "Cannot setup test environment"
> -		"$GIT_EXEC_PATH/git-init" "--template=$GIT_BUILD_DIR/templates/blt/" >&3 2>&4 ||
> +		"${GIT_TEST_INSTALLED:-$GIT_EXEC_PATH}/git" init \
> +			"--template=$GIT_BUILD_DIR/templates/blt/" >&3 2>&4 ||
>  		error "cannot run git init -- have you built things yet?"
>  		mv .git/hooks .git/hooks-disabled
>  	) || exit
Johannes Schindelin Nov. 14, 2018, 1:16 p.m. UTC | #2 
Hi Junio,

On Wed, 14 Nov 2018, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
> 
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > It really makes very, very little sense to use a different git
> > executable than the one the caller indicated via setting the environment
> > variable GIT_TEST_INSTALLED.
> 
> Makes perfect sense.  Shouldn't we be asking where the template
> directory of the installed version is and using it instead of the
> freshly built one, no?

It would make sense, but we don't know how to get that information, do we?

$ git grep DEFAULT_GIT_TEMPLATE_DIR
Makefile:       -DDEFAULT_GIT_TEMPLATE_DIR='"$(template_dir_SQ)"'
builtin/init-db.c:#ifndef DEFAULT_GIT_TEMPLATE_DIR
builtin/init-db.c:#define DEFAULT_GIT_TEMPLATE_DIR "/usr/share/git-core/templates"
builtin/init-db.c:              template_dir = to_free = system_path(DEFAULT_GIT_TEMPLATE_DIR);
contrib/vscode/init.sh: '-DDEFAULT_GIT_TEMPLATE_DIR="$(template_dir_SQ)"' \

In other words, the Makefile defines the DEFAULT_GIT_TEMPLATE_DIR, and the
only user in the code is init-db.c which uses it in copy_templates().

And changing the code *now* to let us query Git where it thinks its
templates should be won't work, as this patch is about using the installed
Git (at whatever pre-compiled version that might be).

Ciao,
Dscho

> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  t/test-lib-functions.sh | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> > index 78d8c3783b..801cc9b2ef 100644
> > --- a/t/test-lib-functions.sh
> > +++ b/t/test-lib-functions.sh
> > @@ -900,7 +900,8 @@ test_create_repo () {
> >  	mkdir -p "$repo"
> >  	(
> >  		cd "$repo" || error "Cannot setup test environment"
> > -		"$GIT_EXEC_PATH/git-init" "--template=$GIT_BUILD_DIR/templates/blt/" >&3 2>&4 ||
> > +		"${GIT_TEST_INSTALLED:-$GIT_EXEC_PATH}/git" init \
> > +			"--template=$GIT_BUILD_DIR/templates/blt/" >&3 2>&4 ||
> >  		error "cannot run git init -- have you built things yet?"
> >  		mv .git/hooks .git/hooks-disabled
> >  	) || exit
>
Junio C Hamano Nov. 14, 2018, 2:59 p.m. UTC | #3 
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> It would make sense, but we don't know how to get that information, do we?
> ...
> And changing the code *now* to let us query Git where it thinks its
> templates should be won't work, as this patch is about using the installed
> Git (at whatever pre-compiled version that might be).

It won't work, but we can add something like "git var templatedir"
to help those who want to further improve the test-installed mode
next year, preparing for better future by sowing seeds now.

In the meantime, using the same temlate dir as before is probably
the best we can do.  Two and a half tangential thoughts are:

 - But then, we need to make sure $GIT_BUILD_DIR/templates/blt/
   is populated, if we do rely on them (i.e. we probably want to
   make sure we have built).

 - Yet, once installed, the contents of the templatedir can be
   arbitrarily munged by the end user, so anything that depends on
   what is in the template won't work as a reliable test piece.

 - Among what's in templates/blt/, we explicitly disable hooks at
   the beginning of the test repository creation to ensure no hooks
   interfere what we test by default, but we will get affected by
   what is in info/excludes.  The contents of freshly-built one is
   empty, so it is unlikely that this will cause problems, but if we
   use installed templates, we cannot control what's in there,
   letting the tests get affected to random things the end-user
   happens to have.

So after all, if we were to change anything, it might make better
sense not to install anything from any templatedir.

But of course, that is orthogonal to the test-install mode.  If we
want to make the test more robust by emptying the templates, we
should do that also for the test-freshly-baked mode, too.
Jeff King Nov. 14, 2018, 9:38 p.m. UTC | #4 
On Wed, Nov 14, 2018 at 02:16:37PM +0100, Johannes Schindelin wrote:

> > Makes perfect sense.  Shouldn't we be asking where the template
> > directory of the installed version is and using it instead of the
> > freshly built one, no?
> 
> It would make sense, but we don't know how to get that information, do we?
> 
> $ git grep DEFAULT_GIT_TEMPLATE_DIR
> Makefile:       -DDEFAULT_GIT_TEMPLATE_DIR='"$(template_dir_SQ)"'
> builtin/init-db.c:#ifndef DEFAULT_GIT_TEMPLATE_DIR
> builtin/init-db.c:#define DEFAULT_GIT_TEMPLATE_DIR "/usr/share/git-core/templates"
> builtin/init-db.c:              template_dir = to_free = system_path(DEFAULT_GIT_TEMPLATE_DIR);
> contrib/vscode/init.sh: '-DDEFAULT_GIT_TEMPLATE_DIR="$(template_dir_SQ)"' \
> 
> In other words, the Makefile defines the DEFAULT_GIT_TEMPLATE_DIR, and the
> only user in the code is init-db.c which uses it in copy_templates().
> 
> And changing the code *now* to let us query Git where it thinks its
> templates should be won't work, as this patch is about using the installed
> Git (at whatever pre-compiled version that might be).

Do we actually care where the templates are? I thought the point was to
override for the built Git to use the built templates instead of the
installed one. For an installed Git, shouldn't we not be overriding the
templates at all? I.e.:

  if test -n "$GIT_TEST_INSTALLED"
  then
	"$GIT_TEST_INSTALLED/git" init
  else
	"$GIT_ExEC_PATH/git" init --template="$GIT_BUILD_DIR/templates/blt"
  fi >&3 2>&4

(That's all leaving aside the question of whether we ought to be using a
clean template dir instead of this).

-Peff
Johannes Schindelin Nov. 15, 2018, 12:29 p.m. UTC | #5 
Hi Junio,

On Wed, 14 Nov 2018, Jeff King wrote:

> On Wed, Nov 14, 2018 at 02:16:37PM +0100, Johannes Schindelin wrote:
> 
> > > Makes perfect sense.  Shouldn't we be asking where the template
> > > directory of the installed version is and using it instead of the
> > > freshly built one, no?
> > 
> > It would make sense, but we don't know how to get that information, do we?
> > 
> > $ git grep DEFAULT_GIT_TEMPLATE_DIR
> > Makefile:       -DDEFAULT_GIT_TEMPLATE_DIR='"$(template_dir_SQ)"'
> > builtin/init-db.c:#ifndef DEFAULT_GIT_TEMPLATE_DIR
> > builtin/init-db.c:#define DEFAULT_GIT_TEMPLATE_DIR "/usr/share/git-core/templates"
> > builtin/init-db.c:              template_dir = to_free = system_path(DEFAULT_GIT_TEMPLATE_DIR);
> > contrib/vscode/init.sh: '-DDEFAULT_GIT_TEMPLATE_DIR="$(template_dir_SQ)"' \
> > 
> > In other words, the Makefile defines the DEFAULT_GIT_TEMPLATE_DIR, and the
> > only user in the code is init-db.c which uses it in copy_templates().
> > 
> > And changing the code *now* to let us query Git where it thinks its
> > templates should be won't work, as this patch is about using the installed
> > Git (at whatever pre-compiled version that might be).
> 
> Do we actually care where the templates are? I thought the point was to
> override for the built Git to use the built templates instead of the
> installed one. For an installed Git, shouldn't we not be overriding the
> templates at all? I.e.:
> 
>   if test -n "$GIT_TEST_INSTALLED"
>   then
> 	"$GIT_TEST_INSTALLED/git" init
>   else
> 	"$GIT_ExEC_PATH/git" init --template="$GIT_BUILD_DIR/templates/blt"
>   fi >&3 2>&4
> 
> (That's all leaving aside the question of whether we ought to be using a
> clean template dir instead of this).

I fear that that might buy us a ton of trouble. Just like we override the
system config, we should override the templates.

Ciao,
Dscho
Jeff King Nov. 15, 2018, 12:41 p.m. UTC | #6 
On Thu, Nov 15, 2018 at 01:29:58PM +0100, Johannes Schindelin wrote:

> > Do we actually care where the templates are? I thought the point was to
> > override for the built Git to use the built templates instead of the
> > installed one. For an installed Git, shouldn't we not be overriding the
> > templates at all? I.e.:
> > 
> >   if test -n "$GIT_TEST_INSTALLED"
> >   then
> > 	"$GIT_TEST_INSTALLED/git" init
> >   else
> > 	"$GIT_ExEC_PATH/git" init --template="$GIT_BUILD_DIR/templates/blt"
> >   fi >&3 2>&4
> > 
> > (That's all leaving aside the question of whether we ought to be using a
> > clean template dir instead of this).
> 
> I fear that that might buy us a ton of trouble. Just like we override the
> system config, we should override the templates.

Yes, it might. I guess it just seems plausible to me that somebody would
expect GIT_TEST_INSTALLED to be as close to the installed experience as
possible. I dunno. I do not use it myself.

At any rate, my point was that for GIT_TEST_INSTALLED, either:

  1. We can use a known-clean set of templates (either our local
     templates/blt, or an even-cleaner empty set).

or

  2. We do not need to specify any template, and it will just use
     whatever it came installed with.

And in either case, we do not have to worry about asking it "where are
your templates?".

-Peff
diff mbox series

Patch

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 78d8c3783b..801cc9b2ef 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -900,7 +900,8 @@  test_create_repo () {
 	mkdir -p "$repo"
 	(
 		cd "$repo" || error "Cannot setup test environment"
-		"$GIT_EXEC_PATH/git-init" "--template=$GIT_BUILD_DIR/templates/blt/" >&3 2>&4 ||
+		"${GIT_TEST_INSTALLED:-$GIT_EXEC_PATH}/git" init \
+			"--template=$GIT_BUILD_DIR/templates/blt/" >&3 2>&4 ||
 		error "cannot run git init -- have you built things yet?"
 		mv .git/hooks .git/hooks-disabled
 	) || exit