Re: [PATCH v6 09/12] split-index: disable the fsmonitor extension when running the split index test

From: Jonathan Nieder <jrnieder@gmail.com>
To: Ben Peart <peartben@gmail.com>
Cc: Ben Peart <benpeart@microsoft.com>,
	David.Turner@twosigma.com, avarab@gmail.com,
	christian.couder@gmail.com, git@vger.kernel.org,
	gitster@pobox.com, johannes.schindelin@gmx.de, pclouds@gmail.com,
	peff@peff.net
Subject: Re: [PATCH v6 09/12] split-index: disable the fsmonitor extension when running the split index test
Date: Wed, 20 Sep 2017 10:46:13 -0700	[thread overview]
Message-ID: <20170920174613.GA27425@aiede.mtv.corp.google.com> (raw)
In-Reply-To: <43c9d3b2-2895-adcb-6f77-b6967aacf9c8@gmail.com>

Hi,

Ben Peart wrote:
> On 9/19/2017 4:43 PM, Jonathan Nieder wrote:

>> This feels to me like the wrong fix.  Wouldn't it be better for the
>> test not to depend on the precise object ids?  See the "Tips for
>> Writing Tests" section in t/README:
>
> I completely agree that a better fix would be to rewrite the test to
> not hard code the SHA values.  I'm sure this will come to bite us
> again as we discuss the migration to a different SHA algorithm.

nit: the kind of change I'm proposing does not entail a full rewrite. :)

The SHA migration aspect is true, but that's actually the least of my
worries.  I intend to introduce a SHA1 test prereq that crazy tests
which want to depend on the hash function can declare a dependency on.

My actual worry is that tests hard-coding object ids are (1) hard to
understand, as illustrated by my having no clue what these particular
object ids refer to and (2) very brittle, since an object id changes
whenever a timestamp or any of the history leading to an object
changes.  They create a trap for anyone wanting to change the test
later.  They are basically change detector tests, which is generally
accepted to be a bad practice.

> That said, I think fixing this correctly is outside the scope of
> this patch series.  It has been written this way since it was
> created back in 2014 (and patched in 2015 to hard code the V4 index
> SHA).

Fair enough.

> If desired, this patch can simply be dropped from the series
> entirely as I doubt anyone other than me will attempt to run it with
> the fsmonitor extension turned on.

*shrug*

My motivations in the context of the review were:

 * now that we noticed the problem, we have an opportunity to fix it!
   (i.e. a fix would not have to be part of this series and would not
   necessarily have to be written by you)

 * if we include this non-fix, the commit message really needs to say
   something about it.  Otherwise people are likely to cargo-cult it
   in other contexts and make the problem worse.

Thanks,
Jonathan