On Tue, Apr 27, 2010 at 12:15:52PM -0500, Jonathan Nieder wrote:
> 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.
> 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?
> 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.
> > --- 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@xxxxxxxxx>
Much appreciated!
Cheers,
--
Gary V. Vaughan (gary@xxxxxxxxxxxxxxxxxx)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html