Re: [PATCH 2/2] test-lib: exhaustively insert non-alnum ASCII into the TRASH_DIRECTORY name

From: Jeff King <peff@peff.net>
Date: Apr 10, 2017, 13:38 UTC
Message-ID: <20170410133856.qbmcif2it2gc6kdw@sigill.intra.peff.net>
In-Reply-To: <CACBZZX7kMcTgKFkFN3OvVKVHU693PYhRFe6gyO4AirihNsUYmg@mail.gmail.com>

On Mon, Apr 10, 2017 at 01:40:13PM +0200, Ævar Arnfjörð Bjarmason wrote:

Show 10 quoted lines

> > A few other failures are triggered by the ':' in the trash directory's
> > name, breaking the following commonly used pattern:
> >
> >   export GIT_CEILING_DIRECTORIES="$TRASH_DIRECTORY" &&
> >   cd subdir &&
> >   test-git-pretending-it's-run-outside-of-a-repository
> 
> Does GIT_CEILING_DIRECTORIES support escaping somehow? E.g.
> "foo\:bar". If so maybe we could use a wrapper to set it, if not
> that's a bug in the ceiling dir feature, surely.

I don't think it does. But nor does $PATH (which is almost certainly another source of breakage). And neither does info/alternates allow pathnames with newline in them (that is the likely cause of a large number of failures, as it hits anywhere we use "clone -s").

Are those bugs? Maybe. Certainly they are limitations. But are they ones anybody _cares_ about? I think this may fall under "if it hurts, don't do it". If there are security bugs where a malicious input can cause us to do something bad, that's something to care about. But that's very different than asking "do these tests run to completion with a funny input".

  As an aside, I'd also question whether your patch might actually
  _hide_ bugs. It's applying a blanket change to the on-disk state that
  is obviously breaking some features. How many of those breakages are
  things that show up as a test failure, and how many of them quietly
  cause a test to do something else entirely, like failing its setup in
  a way that makes the rest of the test a noop?

So given the pain this will cause to people actually looking at tests, and given that it's not clear to me if it has or will find any actual bugs, it seems premature to flip it on by default. If somebody wants to actually dig into these cases and look for actual bugs, I'm all for it.

But flipping the default and marking a bunch of tests blindly as "well, this fails" hasn't made the world a better place. It's made it slightly worse.

-Peff

PS If you want to test various characters, I think the simplest way is
   just:

     make GIT_TEST_OPTS=--root=/tmp/look-a-col:on test