Subject: Re: [PATCH 11/38] t7063: make hash size independent

From: "brian m. carlson" <[email-masked]>
Date: Sat, 11 Jul 2020 00:54:07 +0000

On 2020-07-11 at 00:43:08, Junio C Hamano wrote:
> "brian m. carlson" <[email-masked]> writes:
> 
> > Use test_oid instead of hard-coding a fixed size all-zeros object ID.
> >
> > Signed-off-by: brian m. carlson <[email-masked]>
> > ---
> >  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