Re: [PATCH 3/4] build dashless "test-bin" directory similar to installed bindir

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 28, 2009, 19:44 UTC
Message-ID: <7vtywefn88.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1259433537-3963-4-git-send-email-mmogilvi_git@miniinfo.net>

Matthew Ogilvie <mmogilvi_git@miniinfo.net> writes:

> The new test-bin directory contains wrapper scripts for executables that
> will be installed into the standard bindir.  It explicitly does not
> contain most dashed-commands.  The scripts automatically set environment
> variables to run out of the source tree, not the installed directory.

Even though I haven't tried the series, I like most of the things I see in this series.

 - Patch #1 and #2 are good and are independent from the later patches, as
   without them running tests with GIT_TEST_INSTALLED would not work.

   By the way, 6720721 (test-lib.sh: Allow running the test suite against
   installed git, 2009-03-16) failed to document the feature in t/README.
   Could you please fix this while you are at it?

 - Running tests before installation in an environment slightly closer to
   the final installation (i.e. lacks dashed commands in the $PATH during
   the time tests are run) is a good direction to go.

 - I like the Makefile changes that uses these new BINDIR_PROGRAMS_NEED_X,
   TEST_PROGRAMS_NEED_X.  Parameterizing commands listed in the actions
   part is good.

 - It certainly is _possible_ to add $(pwd)/test-bin to $PATH instead of
   the established practice of using GIT_EXEC_PATH for every day/permanent
   use without installation, but I doubt we should _encourage_ it for a
   few reasons:

   . The set-up will force one extra exec due to the wrapper; this is good
     for the purpose of running tests, but unnecessary for a set-up for
     every day/permanent use by people, compared with the already working
     approach.  The user needs to change an environment variable _anyway_
     (either GIT_EXEC_PATH with the traditional approach, or PATH with
     your patch).

   . The new component to be added to $PATH shouldn't be named "test-bin/"
     if it is meant for every day/permanent use.

   . Advertising this forces the Makefile build test-bin/ contents from
     "all" target.  I think test-bin/ should only depend on "test" (iow,
     after "make all && make install" there shouldn't have to be "test-bin"
     directory.

   I would rather treat it an unintended side-effect that you can add that
   directory to the $PATH.  It is designed to work in such an environment
   (otherwise the tests won't exercise the version of git they are meant
   to test), but I do not think it is _meant_ to be _used_  by end users
   that way.  And an unintended side-effect does not have to be mentioned
   in INSTALL (especially with the directory name with "test" in it).