Re: [PATCH 1/3] tests: Prepare --textconv tests for correctly-failing conversion program

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Sep 20, 2010, 21:03 UTC
Message-ID: <vpq1v8o9lg8.fsf@bauges.imag.fr>
In-Reply-To: <20100920203501.GB23533@landau.phys.spbu.ru>

Kirill Smelkov <kirr@landau.phys.spbu.ru> writes:

> Matthieu, Jeff, Junio, thanks for your review and input.
>
> Comments below inline...

You've replied to several messages in one. That's OK here since the content of your message is essentially uncontroversial, but I'd suggest keeping separate threads for separate patches for next time. (for example, my reply below looks strange since I'm commenting on PATCH 2/3 in a message titled PATCH 1/3 ...). No big deal.

Show 11 quoted lines

>> > Subject: Re: [PATCH 2/3] blame,cat-file: Demonstrate --textconv is wrongly running converter on symlinks
>>
>> We try to keep the subject lines short (<80 chars, and as much as
>> possible less so that "git log --oneline" be pretty).
>>
>> How about
>>
>> blame,cat-file: add failing tests for --textconv on symlinks
>
> I'd like to shorten it, but "add failing tests" is not as descriptive as
> "Demonstrate --textconv misbehaves in such-and-such way",

Right, but your "such-and-such way" is not really precise either. One cannot know whether it's running, but the wrong way, whether it's running and should not, etc. The subject line is here to be a summary, you don't need details. And I actually find "add failing tests" more precise than "demonstrate that ... is behaving wrongly".

Show 5 quoted lines

> and I can't come up with a shorter subject without making it more
> cryptic. And btw, I've looked at log --oneline output, and
> (surprise, surprise)
>
> 479a56 4fccc04 etc ...

I don't understand what you mean. After applying your patches, I get:

6998f9a RFC: blame,cat-file --textconv: Don't assume mode is ``S_IFREF | 0664'' 025aaac blame,cat-file: Demonstrate --textconv is wrongly running converter on symlinks 3646401 tests: Prepare --textconv tests for correctly-failing conversion program

which do not fit on a 80-columns terminal.

> So if that's not a major obstacle, I'd leave it as is.

I don't consider it "major" (but other may have different opinions) though.

Show 11 quoted lines

>> These days, it's recommanded to put this kind of code within the
>> test_expect_success/test_expect_failure.
>
> I see, thanks.
>
> I've moved other >expected preparation inside test_expect_*, but this
> expect is special in that it is used in subsequent two tests. So I'd
> leave this one outside.
>
> And btw, I've originally copied in-style what was already there in t8006
> and t8007 which date to Jun 2010.

Yes, the guideline is new, and not widely applied (yet).

> Ah, yes, thanks. I forgot ':/text' means `commit which log is text' -

A patch to update the comment above follows.

> Thanks again to everyone.

That "everyone" includes you ;-). Thanks for the patch, good job both for testing and for the actual fix!

-- Matthieu Moy http://www-verimag.imag.fr/~moy/