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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 9/19/2017 4:43 PM, Jonathan Nieder wrote:
Hi,

Ben Peart wrote:

The split index test t1700-split-index.sh has hard coded SHA values for
the index.  Currently it supports index V4 and V3 but assumes there are
no index extensions loaded.

When manually forcing the fsmonitor extension to be turned on when
running the test suite, the SHA values no longer match which causes the
test to fail.

The potential matrix of index extensions and index versions can is quite
large so instead disable the extension before attempting to run the test.

Thanks for finding and diagnosing this problem.

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.

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

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.

	                                                         And
	such drastic changes to the core GIT that even changes these
	otherwise supposedly stable object IDs should be accompanied by
	an update to t0000-basic.sh.

	However, other tests that simply rely on basic parts of the core
	GIT working properly should not have that level of intimate
	knowledge of the core GIT internals.  If all the test scripts
	hardcoded the object IDs like t0000-basic.sh does, that defeats
	the purpose of t0000-basic.sh, which is to isolate that level of
	validation in one place.  Your test also ends up needing
	updating when such a change to the internal happens, so do _not_
	do it and leave the low level of validation to t0000-basic.sh.

Worse, t1700-split-index.sh doesn't explain where the object ids it
uses comes from so it is not even obvious to a casual reader like me
how to fix it.

See t/diff-lib.sh for some examples of one way to avoid depending on
the object id computation.  Another way that is often preferable is to
come up with commands to compute the expected hash values, like
$(git rev-parse HEAD^{tree}), and use those instead of hard-coded
values.

Thanks and hope that helps,
Jonathan




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux