Re: [patch 06/16] diff-test_cmp.patch

From: Gary V. Vaughan <git@mlists.thewrittenword.com>
Date: Apr 28, 2010, 09:00 UTC
Message-ID: <20100428090045.GD36271@thor.il.thewrittenword.com>
In-Reply-To: <20100427171531.GA15553@progeny.tock>

On Tue, Apr 27, 2010 at 12:15:52PM -0500, Jonathan Nieder wrote:

Show 13 quoted lines

> Hi,
> 
> Gary V. Vaughan wrote:
> 
> > Subject: diff-test_cmp.patch
> >
> > In tests, call test_cmp rather than raw diff where possible (i.e. if
> > the output does not go to a pipe), to allow the use of, say, 'cmp'
> > when the default 'diff -u' is not compatible with a vendor diff.
> > 
> > When that is not possible, use $DIFF, as set in GIT-BUILD-OPTIONS.
> 
> Sign-off?  (See SubmittingPatches for what I am asking about here.)

Ah, okay.  I didn't read carefully enough. Sorry.

Can I add a Sign-off message to each patch subthread? Or do you need me to resubmit the entire series?

> And is it possible to change your mailing script to use more
> meaningful subject lines?

Sure. What is preferable? As short a sentence summarising the fixed issue as I can muster? (Like SuSE Linux, we use quilt to manage and submit our patch stacks... git seems to require hosting the entire history of each project which is too heavyweight for the 1000's of packages we build - if git provides the means to store just the head of an upstream release branch along with our patch stacks on local disk, I would love to be proven wrong here).

> This patch makes a good change, but I do not think your description
> captures it.  Most of the changes are from ???diff???, not from ???diff -u???.
> Is your bare ???diff??? really incapable of distinguishing between
> identical and differing files?

A good point, thanks. I think the scope has crept since I first started applying such a patch to earlier releases of git in our package tree.

Show 16 quoted lines

> But using test_cmp consistently would make debugging test scripts
> with -v much easier, since the output is more readable.
> 
> > --- a/t/t0000-basic.sh
> > +++ b/t/t0000-basic.sh
> > @@ -280,7 +280,7 @@ $expectfilter >expected <<\EOF
> >  EOF
> >  test_expect_success \
> >      'validate git diff-files output for a know cache/work tree state.' \
> > -    'git diff-files >current && diff >/dev/null -b current expected'
> > +    'git diff-files >current && test_cmp current expected >/dev/null'
> 
> The original ignores whitespace changes; this version does not.  It
> turns out that???s okay, but it???s worth mentioning in the commit
> message.  (I think we do guarantee that diff-files will not change the
> whitespace it produces without good reason.)

Okay, I'll rewrite the patch headers where they have bit-rotted. Maybe in combination with the missing Signed-off-by: headers and unsuitable Subject headers I need to amend and resubmit the whole patch series again?

Show 16 quoted lines

> The original suppressed its output, without any good reason to.  Could
> you remove the >/dev/null while at it, to make it easier to debug?
> 
> > --- a/t/t4124-apply-ws-rule.sh
> > +++ b/t/t4124-apply-ws-rule.sh
> > @@ -44,7 +44,7 @@ test_fix () {
> >  	apply_patch --whitespace=fix || return 1
> >  
> >  	# find touched lines
> > -	diff file target | sed -n -e "s/^> //p" >fixed
> > +	$DIFF file target | sed -n -e "s/^> //p" >fixed
> >  
> >  	# the changed lines are all expeced to change
> >  	fixed_cnt=$(wc -l <fixed)
> 
> Is this needed?  I don???t mind it, just curious.

Probably not, though I prefer the consistency rather than having to consider diff vs $DIFF at every occurence. libtool has had numerous silly bugs tickled by making the wrong choice between echo, $echo, $lt_echo and $ECHO in rarely exercised corners of the code.

> I hope some earlier patch takes care of setting DIFF in test-lib.sh.
> Tests need to be usable without running them through the makefile.

Unfortunately, that is not the case in 7.1.1.  This is it:

GIT_TEST_CMP=${GIT_TEST_CMP:-diff -u}

The obvious fix would be to use:

: ${DIFF=@DIFF@} : ${GIT_TEST_CMP=@DIFF@ -u}

And substitute at configure time. Makefile only builds will require additional support though.

Show 20 quoted lines

> > --- a/t/Makefile
> > +++ b/t/Makefile
> > @@ -6,10 +6,14 @@
> >  -include ../config.mak
> >  
> >  #GIT_TEST_OPTS=--verbose --debug
> > +GIT_TEST_CMP ?= $(DIFF)
> >  SHELL_PATH ?= $(SHELL)
> >  TAR ?= $(TAR)
> >  RM ?= rm -f
> >  
> > +# Make sure test-lib.sh uses make's value of GIT_TEST_CMP
> > +export GIT_TEST_CMP
> > +
> >  # Shell quote;
> >  SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))
> 
> If neither DIFF nor GIT_TEST_CMP is already set, this will export
> GIT_TEST_CMP as the empty string.  Will t/test-lib.sh treat that as
> asking for the default?  Yes --- phew.

That change was just added to my patch yesterday, to fix the massive testsuite failures I had previously experienced.

>   Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Much appreciated!

Cheers,

-- 
Gary V. Vaughan (gary@thewrittenword.com)