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
