[prev in list] [next in list] [prev in thread] [next in thread] 

List:       git
Subject:    Re: [PATCH v3 06/30] subtree: t7900: use 'test' for string equality
From:       Luke Shumaker <lukeshu () lukeshu ! com>
Date:       2021-04-30 16:33:10
Message-ID: 87im43pwah.wl-lukeshu () lukeshu ! com
[Download RAW message or body]

On Fri, 30 Apr 2021 03:55:04 -0600,
Ævar Arnfjörð Bjarmason wrote:
> 
> 
> On Tue, Apr 27 2021, Luke Shumaker wrote:
> 
> > From: Luke Shumaker <lukeshu@datawire.io>
> >
> > t7900-subtree.sh defines its own `check_equal A B` function, instead of
> > just using `test A = B` like all of the other tests.  Don't be special,
> > get rid of `check_equal` in favor of `test`.
> >
> > Signed-off-by: Luke Shumaker <lukeshu@datawire.io>
> > ---
> >  contrib/subtree/t/t7900-subtree.sh | 60 ++++++++++++------------------
> >  1 file changed, 24 insertions(+), 36 deletions(-)
> >
> > diff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh
> > index 12b8cb03c7..76183153c9 100755
> > --- a/contrib/subtree/t/t7900-subtree.sh
> > +++ b/contrib/subtree/t/t7900-subtree.sh
> > @@ -28,18 +28,6 @@ create () {
> >  	git add "$1"
> >  }
> >  
> > -check_equal () {
> > -	test_debug 'echo'
> > -	test_debug "echo \"check a:\" \"{$1}\""
> > -	test_debug "echo \"      b:\" \"{$2}\""
> > -	if test "$1" = "$2"
> > -	then
> > -		return 0
> > -	else
> > -		return 1
> > -	fi
> > -}
> 
> It looks like the reason this was used because when this fails just
> having the "test" makes for bad debugging. I.e. if the values don't
> match the $1 and $2 are not aligned, so it's hard to eyeball what went
> wrong.

It's easy to make that assumption, but looking at the history it seems
the "actual" reason it exists is that it's vestigial from before the
subtree tests used test-lib.sh, and echoing it like that was the only
way you'd get feedback.

> These days this is more idiomatic:
> 
>     echo "Add [...]" >expected
>     last_commit_message >actual &&
>     test_cmp expected actual
> 
> So I think in this case a better narrower improvement would be to keep
> the check_equal function. I wonder if we shouldn't just in general in
> t/test-lib.sh have a test_cmp_str for this use-case. I.e. a trivial
> wrapper that echos the two strings to a file for you, before running
> diff(1).

But it's been my experience that the tests are already impossible to
debug without passing `-x`, so everything from `check_equal` ends up
being just noise.

And also I figured if `test` is good enough for t9350-fast-export.sh,
then it's good enough for t7900-subtree.sh... after working with
subtree, I'd accidentally written a couple of the checks in one of my
fast-export patchsets using `check_equal`.  Being special makes things
harder to hack on.

-- 
Happy hacking,
~ Luke Shumaker
[prev in list] [next in list] [prev in thread] [next in thread]