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

[Jeff King <peff@peff.net>]

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

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