On 2020-07-11 at 00:43:08, Junio C Hamano wrote: > "brian m. carlson" <sandals@xxxxxxxxxxxxxxxxxxxx> writes: > > > Use test_oid instead of hard-coding a fixed size all-zeros object ID. > > > > Signed-off-by: brian m. carlson <sandals@xxxxxxxxxxxxxxxxxxxx> > > --- > > t/t7063-status-untracked-cache.sh | 155 ++++++++++++++++-------------- > > 1 file changed, 83 insertions(+), 72 deletions(-) > > The stated objective does make sense, but ... > > > > -info/exclude 13263c0978fb9fad16b2d580fb800b6d811c3ff0 > > -core.excludesfile 0000000000000000000000000000000000000000 > > +info/exclude $(test_oid exclude) > > +core.excludesfile $ZERO_OID > > exclude_per_dir .gitignore > > flags 00000006 > > -/ e6fcc8f2ee31bae321d66afd183fcb7237afae6e recurse > > -/done/ 0000000000000000000000000000000000000000 recurse valid > > -/dthree/ 0000000000000000000000000000000000000000 recurse check_only valid > > -/dtwo/ 0000000000000000000000000000000000000000 recurse check_only valid > > +/ $(test_oid root) recurse > > +/done/ $ZERO_OID recurse valid > > +/dthree/ $ZERO_OID recurse check_only valid > > +/dtwo/ $ZERO_OID recurse check_only valid > > two > > EOF > > - test_cmp ../expect ../actual > > + test_might_fail test_cmp ../expect ../actual > > Any "cmd" that is run under test_might_fail that is *not* used for > its side effect is suspect---e.g. "we would try to remove this file > as the test may have created it, but it is OK if the file does not > exist and removal fails" is sort-of understandable, but I am having > a hard time imagining in what situation it makes sense for a test to > say "these two files may have the same contents but it is OK if that > is not the case". There are a few others in this patch. > > Another topic in flight tightens the allowed usage of test_must_fail > and test_might_fail helpers, and that is how I found this (because > the tip of 'seen' does not pass the test), but regardless of that > tightening, I am not sure what this "these two files may or may not > be equal" is trying to achieve. I agree this is suspect, and I'm not sure what I intended there. The patch is unfortunately over two years old, so I don't recall exactly, but it may have been flaky (in general) at the time I was changing the test and I may have accidentally squashed that into the patch at the time. The test passes without it, so clearly the right thing is to remove the test_might_fail, which I agree is bizarre. Feel free to do that when picking it up, and I'll fix that in in the reroll. -- brian m. carlson: Houston, Texas, US
Attachment:
signature.asc
Description: PGP signature
