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

[Ben Peart <peartben@gmail.com>]

On 9/20/2017 1:46 PM, Jonathan Nieder wrote:
> 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.
> 

I'll update the commit message to indicate this is a temporary 
workaround until the underlying hard coded SHA issue is fixed.

> Thanks,
> Jonathan
>